-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use web streams instead of Node.js streams #61
base: master
Are you sure you want to change the base?
Conversation
c578701
to
226a50a
Compare
this.disableDerivesFromReferences = | ||
args.disableDerivesFromReferences || false | ||
|
||
// number of lines to buffer | ||
this.bufferSize = args.bufferSize === undefined ? 1000 : args.bufferSize | ||
this.bufferSize = args.bufferSize === undefined ? 50000 : args.bufferSize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do want to call out this change, I have run into buffer size being not enough and it is a maddening error. I think it is actually a potential source of serious mysterious bugs and it may be worth removing it entirely, making it infinity (similar to chunk size error or similar...just something the user shouldnt have to configure)
This PR migrates the streaming portions of the library to use web streams instead of Node.js streams API. From the Node.js overview:
The motivation for this is 1) to provide a consistent way to handle streams with the library in the browser or Node.js and 2) to remove any Node.js dependencies from the library so that no polyfilling is necessary in the browser.
The streaming functionality is exported as the
GFFTransformer
andGFFFormattingTransformer
classes. The synchronous functionality remains unchanged, and the new streaming functionality should be easily tree-shaken out by bundlers if it isn't used.Here is an example from the updated README of how the stream parsing can be used in Node.js and the browser:
Node.js example
Browser example
This would be a major version bump, and a couple other breaking change are included:
parseAll
andencoding
parse options have been removed.parseAll
could be difficult to reason about if a use tried to use it at the same time as e.g.parseFeatures
encoding
was unused, and the only spec-compliant encoding of GFF3 is UTF8parseStream
andformatStream
functions were removed since they now didn't add any functionalityOne remaining functionality that would be nice to add is better TypeScript inference with the streaming functionality. For example, with
the type of
result
is(GFF3Feature | GFF3Comment)[]
, since it's able to reason about what is being parsed. I haven't been able to figure out how to do that with the Transformers, though, so withthe type of
returnStream
isTransformStream<Uint8Array, GFF3Item>
, when I'd like it to beTransformStream<Uint8Array, GFF3Feature | GFF3Comment>
.