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

Implement response.body as a stream #746

Closed
pcowgill opened this issue Jan 9, 2020 · 18 comments
Closed

Implement response.body as a stream #746

pcowgill opened this issue Jan 9, 2020 · 18 comments

Comments

@pcowgill
Copy link

@pcowgill pcowgill commented Jan 9, 2020

Response has no .body property available which is needed as a getter to expose a ReadableStream of the body contents.

https://developer.mozilla.org/en-US/docs/Web/API/Body/body

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 10, 2020

@mislav @josh @dgraham Would you be open to a PR for this if I can get it implemented in a fork?

Since React Native uses this code to implement fetch, it would be nice if the fetch API in React Native supported streams.

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 10, 2020

I'm also curious to hear what the React Native folks think of this idea @arthuralee @janicduplessis @cpojer

@pcowgill pcowgill changed the title Implement response.body Implement response.body as a stream Jan 10, 2020
@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 10, 2020

Finally, I thought I'd rope in some of the people contributing to the streams standard. @domenic @ricea @MattiasBuelens

Thanks!

@MattiasBuelens

This comment has been minimized.

Copy link

@MattiasBuelens MattiasBuelens commented Jan 10, 2020

From this comment on #198, it seems like there is no interest in adding streaming support to this polyfill.

There are other fetch polyfills that do support streaming, such as fetch-readablestream or @stardazed/streams-polyfill. (I also made my own, but it's not very well maintained. 😅) If possible, you may want to consider switching to one of those.

@MattiasBuelens

This comment has been minimized.

Copy link

@MattiasBuelens MattiasBuelens commented Jan 10, 2020

Since React Native uses this code to implement fetch, it would be nice if the fetch API in React Native supported streams.

Note that if the underlying platform does not support streaming responses, then the only thing a polyfill can do is create a ReadableStream that contains a single chunk with the final response body. So yes, while you can use a polyfill to add Response.body, it would behave just the same as using Response.arrayBuffer() directly.

If you want actual streaming support in React Native, then React Native would need to implement it itself.

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 10, 2020

@MattiasBuelens Thanks for the prompt and thoughtful response!

There are other fetch polyfills that do support streaming, such as fetch-readablestream

fetch-readablestream passes through res.body from the underlying fetch implementation, so that won't help with having .body.getReader() be defined, will it?

If you want actual streaming support in React Native, then React Native would need to implement it itself.

Since React Native uses this package as its implementation of fetch, wouldn't landing it here address that the next time React Native core upgrades the dep? Or is there something lower level that would need to change?

@MattiasBuelens

This comment has been minimized.

Copy link

@MattiasBuelens MattiasBuelens commented Jan 10, 2020

fetch-readablestream passes through res.body from the underlying fetch implementation, so that won't help with having .body.getReader() be defined, will it?

It only does that if the underlying fetch implementation actually supports Response.body. Otherwise, it will use either XHR with response type moz-chunked-arraybuffer (for "true" streaming support on older Firefox versions), or XHR with responseText (for "pseudo-streaming" support on all browsers, but with fairly bad performance due to string concatenation). Check the source code for more details.

Since React Native uses this package as its implementation of fetch, wouldn't landing it here address that the next time React Native core upgrades the dep? Or is there something lower level that would need to change?

A fetch polyfill is merely a wrapper around XMLHttpRequest. It can't "magically" add streaming support: the underlying platform needs to provide some low-level primitive that can be used to stream a response (such as moz-chunked-arraybuffer or responseText). React Native only implements XMLHttpRequest, so if that does not have any way to do streaming, then a fetch polyfill built on top of that won't be able to do streaming either.

If React Native wanted to support streaming, they would need to add something to their XMLHttpRequest implementation that a fetch polyfill can pick up. However, since there is no standardized API for streaming in XHR, this would require a non-standard extension to their XHR API, which is unlikely to be integrated into a general-purpose fetch polyfill like this one.

The "proper" way would be for React Native to implement fetch themselves on top of native APIs, rather than using a fetch polyfill built on top of XHR.

That said, I had a quick look at the source code for their XHR, and it would seem like they should support "pseudo-streaming" with responseText. So perhaps fetch-readablestream could automatically use that to polyfill Response.body when run inside React Native? It wouldn't be very efficient though, I think. 🤷‍♂

@cpojer

This comment has been minimized.

Copy link

@cpojer cpojer commented Jan 13, 2020

Could this be added separate from fetch as an additional plugin similar to the then/promise module? See https://github.com/then/promise

Then people could do something like:

require('fetch');
require('fetch/body');

where the latter would patch the getter into the former. What do you think?

@hugomrdias

This comment has been minimized.

Copy link

@hugomrdias hugomrdias commented Jan 13, 2020

Note that if the underlying platform does not support streaming responses, then the only thing a polyfill can do is create a ReadableStream that contains a single chunk with the final response body. So yes, while you can use a polyfill to add Response.body, it would behave just the same as using Response.arrayBuffer() directly.

This would be enough for a first step.

@cpojer do you think something like the above could be added to react native? if yes whats the best approach? PR this repo? make a new package patching whatwg-fetch like you said ? directly to react-native network code?

@mislav

This comment has been minimized.

Copy link
Member

@mislav mislav commented Jan 13, 2020

This polyfill was originally intended to be used in web browsers that had no support for the fetch standard. Our target is basically pre-2015 web browsers, and every feature that we add to the polyfill had to be reasonably implementable using the capabilities of those browsers, which was mostly reliant on building on top of XMLHttpRequest.

React Native runs under JavaScriptCore, which I have no familiarity with, but I would like to assume that it's a more modern environment than a pre-2015 web browser. I don't think that including this polyfill in React Native was a great call, since we never targeted this environment, and if React Native chose to maintain their own implementation (or use another implementation that targets JavaScriptCore), I think implementing more modern features such as streaming would be more feasible.

TL;DR; we likely won't implement streaming here because we don't want to find out what what would be involved in making this functionality work seamlessly in older browsers.

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 13, 2020

So it sounds like in the long run we need React Native core to implement fetch, and in the short run the React Native core team ought to fork this repo so we can make PRs against the fork without fear of breaking older browsers?

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 13, 2020

It appears that fetch-readablestream won't work well if you're using the fetch API via ky, because ky assumes response.clone() will be defined, and it's not when using fetch-readablestream in a React Native env.

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 14, 2020

@stardazed/streams-polyfill has clone() implemented, and it seems to be working

@cpojer

This comment has been minimized.

Copy link

@cpojer cpojer commented Jan 14, 2020

fetch is not implemented at the VM level, it's implemented by the host platform (browsers). React Native uses this polyfill directly on top of XMLHttpRequest which itself is implemented using Networking (android implementation, ios implementation), a JS module that exposes native networking functionality for each platform.

It is likely that React Native's networking stack will need changes to support newer features and even better would be native support for fetch, but we are not currently working on that. Is the question here whether we'd accept contributions? If so, yes, definitely! Especially if it unblocks progress for this repo, if people are still interested in maintaining this polyfill.

@hugomrdias

This comment has been minimized.

Copy link

@hugomrdias hugomrdias commented Jan 14, 2020

@cpojer from the comments above it's gonna be hard to add more stuff to this repo.
I'll move this discussion to react-native repo.

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 14, 2020

@hugomrdias @cpojer React Native GitHub issues tend to get closed to comments (totally understandable, but perhaps not ideal for a long-term topic like this). Maybe if we could set up a fork of this repo under the facebook GitHub org with a more liberal issue-closing policy, that would be better? Thanks!

@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 22, 2020

@hugomrdias @cpojer React Native GitHub issues tend to get closed to comments (totally understandable, but perhaps not ideal for a long-term topic like this). Maybe if we could set up a fork of this repo under the facebook GitHub org with a more liberal issue-closing policy, that would be better? Thanks!

@cpojer What do you think about this idea? Thanks!

@dgraham dgraham closed this Jan 24, 2020
@pcowgill

This comment has been minimized.

Copy link
Author

@pcowgill pcowgill commented Jan 24, 2020

Although this issue is closed, it's still an open question where to resume the conversation. So please keep commenting enabled for this issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.