Skip to content
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

Feature/streams #20

Closed
wants to merge 3 commits into from
Closed

Feature/streams #20

wants to merge 3 commits into from

Conversation

jonnyreeves
Copy link
Owner

WIP implementation of stream-based parsers, see #16.

@jonnyreeves jonnyreeves added this to the 0.6 milestone Sep 7, 2016
@jonnyreeves jonnyreeves self-assigned this Sep 7, 2016
@jonnyreeves
Copy link
Owner Author

cc @ariutta - sorry for the delay; hoping to start getting this nailed down now. I've gone for a slightly different API for the parser basing it on a trimmed down implementation of the Response object provided by the fetch API. This will give the parser access to the Headers, statusCode and provide the ability to cancel the response stream (#19)

Thanks to @jimmywarting for the inspiration; the `xhrTransport` now makes use of the `web-streams-polyfill` to return a `ReadableStream` which allows us to emulate the `Response.body()` method from the `fetch` standard.

Took this oppertunity to unify the xhr based implementations and make the transport detection be based on features rather than user-agent detection.
@jonnyreeves
Copy link
Owner Author

This branch now incorperate's @jimmywarting's ideas on using a ReadableStream polyfill for the xhr-based transports; we can now provide a 'fetch-like' API to the parser function.

One question that arises from this change is does providing a parser make sense, or should the library just become a fetch-binary-stream which focuses on allowing you to make http requests from the browser for streamed binary data from the server?

This was referenced Sep 17, 2016
@jimmywarting
Copy link

jimmywarting commented Sep 17, 2016

You are kind of polyfilling the fetch api now? 😆
Which has already been done: here
But they don't seem to plan on adding streaming support JakeChampion/fetch#198 👎

You should maybe make a PR to them :P

@jimmywarting
Copy link

jimmywarting commented Sep 17, 2016

I don't think you should parse the arrayBuffer to string. If it's going to work like fetch ReadableStream then parsing the response to text is, well kind of unexpected. The Stream api have something like a binary stream that is more friendly with memory allocation. Other libraries could take advantage of this such as jszip where they could "read into a ArrayBuffer allocation"

The TextDecoder api will eventually have some Transform stream
Then you just have to .pipeThrogh(new TextDecoder) something like that

You can create another module that's more like a polyfill out of TextDecoder transform. I almost did it with web-byline
You can take out most of the logic from index.js and only keep the TextDecoder part

Beside you don't want to parse a binary and you only get one ReadableStream back, so how do you know if you should parse it? you don't want to do it with a image... and it don't look good with an option such as createReadableStream({parse: boolean})

Think it's just better to pipe it
So i say remove the parser!

@jonnyreeves
Copy link
Owner Author

OK - taken a very different approach over the last few days and have pulled the guts of this library out into a new library: https://github.com/jonnyreeves/fetch-readablestream

I am unsure what to do about this library - it feels a touch redundant now as it just provides a defaultParser implementation which is trivial to re-create in user code, eg: https://github.com/jonnyreeves/fetch-readablestream/blob/release/v0.1.0/test/integ/util.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants