feat: web streams based encoding/csv - #1993
Conversation
| comment?: string; | ||
| } | ||
|
|
||
| export class CSVStream { |
There was a problem hiding this comment.
You could rather make CSVStream extend TransformStream, then the only thing you have to do is super() with the transformstream options; and the variables you defined can just be private properties.
This isnt necessary, but imo just a lot cleaner & direct
| @@ -0,0 +1,51 @@ | |||
| import { Buffer, BufReader } from "../../io/buffer.ts"; | |||
There was a problem hiding this comment.
Now, this is going to be the problem: any of the new streams based APIs are not allowed to depend on any Deno.Reader/Writer APIs. You'll have to wait for #1970 to land
There was a problem hiding this comment.
Rewrote CSVStream not to depend on Deno.Reader in c573f87
TextProtoReader will no longer be needed to implement encoding/csv/stream 🙂
lucacasonato
left a comment
lucacasonato
left a comment
There was a problem hiding this comment.
I think the CSVStream should be a TransformStream<string, string[]> rather than TransformStream<Uint8Array, string[]>. The text decoding can be done with TextDecoderStream by the user.
crowlKats
left a comment
crowlKats
left a comment
There was a problem hiding this comment.
LGTM, thanks! the PR is still a draft; do you still have things to add? if not, i'd like to get this landed soon
|
@crowlKats Thanks for the review! I'd like to add more tests later in a separate PR, since the tests are a little low |
3 participants
This PR tries to implement web streams based
encoding/csvmodule.I'm not very familiar with web streams, so any feedback would be appreciated!
Part of #1986
Changes
encoding/csv/stream.tsencoding/csv/_io.tsto remove duplicate codes betweenencoding/csv.tsandencoding/csv/stream.ts