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

Full stream support FTW #413

Closed
wants to merge 22 commits into from
Closed

Conversation

jimmywarting
Copy link

@jimmywarting jimmywarting commented Oct 19, 2016

I don't know how happy you are on depending on other lib... but this is easier then making streaming optional and only support it when the web-stream-polyfill is included sence everything is now based on streams

This is a fork of #404

It brings full streaming support to the table with moz-chunked-array buffer and almost ms-stream (couldn't test that) and it also uses text as last fallback to bring streaming support to older browsers

It also removed some private property out of the body class as it's now all based on ReadableStream
It have lead to text() blob() arrayBuffer() and json() becomeing a litle more manageable/readable

the fetch now resolves a response earlier on stage 2 (header recived) so you know a litle bit more about the kind of response you are going to get instead of waiting for the hole response to finish

it also adds support for posting arrayBuffer and typed arrays
It also support reading a FormData 2.0 (meaning it's readable)
So you could now do new Response(formData).blob() or even new Response(formData).body.getReader()

it also
resolves #411
resolves #404
resolves #198
resolves #351
resolves #350
resolves #344
resolves #343
resolves #335

It may break some sence it now depends on more stuff like textEncoder/decoder and ofc web-stream-polyfill and typed arrays

@jimmywarting
Copy link
Author

Any feedback?

@mislav
Copy link
Contributor

mislav commented Oct 22, 2016

I'm impressed by your gargantuan effort in putting this PR together, and I like this as a continuation of the existing conversations around streaming and ArrayBuffer, but I hope that you understand that we can't accept a PR of this size, even if it's technically correct and all tests pass.

We maintain this project mainly for the purpose of using it on GitHub.com. Its purpose was never to be a 100%-complete implementation of the spec, but rather a simplest possible implementation of the main interfaces that are enough to make requests in most cases. A diffstat +258 -118 proposed here goes against our core principle by changing most aspects of our implementation, adding additional implementations of low-level concepts such as multipart/form-data encoding, all in the service of adding support for features we at GitHub (and likely most of our users too) don't really need bad enough to warrant this volume of modifications and added code.

If we are going to add streaming support and improve our ArrayBuffer support, I think we would need to approach it in the following manner:

  1. Make those strictly isolated efforts (i.e. implement in different PRs);
  2. Try to gain the most functionality with fewest possible edits to the code;
  3. Give up on features that rely on proprietary browser behaviors and/or need too much code to implement.
  4. Consider the fact that if we implement said features, we will have to support them for a long time by answering bug reports, maintaining the test suite around these features, and keeping up to date with the spec changes.

This is why maintainers are generally conservative when it comes to accepting large changes: a “no” might be only temporary but a “yes” is forever in open source.

@jimmywarting
Copy link
Author

Thanks @mislav It was kind of expected. I know it's a huge change. This can bring up the size up a lot from dependencies (from web-stream-polyfill to those es6 shim you added and also typed arrays to who knows what else)
Thinking if i should just fork everything and build my own fetch polyfill to try to be 100% compatible with the spec...

@mislav
Copy link
Contributor

mislav commented Nov 12, 2016

Closing this out due to the reasons above, and in an effort to clean the issue/PR backlog. @jimmywarting: we still want streaming, especially since the fetch spec seems to be written lately with streaming as first-class citizen whenever it talks about request/response bodies, but I want us to add support incrementaly and with least disruption to the stability of this project.

@mislav mislav closed this Nov 12, 2016
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants