[WIP] Response now accepts ReadableStream as body #404

jimmywarting · 2016-10-05T21:55:38Z

This is a part necessary to be able to start supporting streaming response (#198)

here is just an example of how to use it

var rs = new ReadableStream({
  start(controller) {
    var chunk = new Uint8Array(3).fill(97)
    controller.enqueue(chunk)
    controller.close()
  }
})

var res = new Response(rs)

res.text().then(console.log) // aaa
res.arrayBuffer().then(console.log) // ArrayBuffer [97, 97, 97]
res.blob().then(console.log) // Blob {size: 3}
var reader = res.body.getReader()

This PR don't make use of ReadableStream it just allows end user to be able to construct a Response with a ReadableStream as body as you should be able to do

mislav

Thanks, this looks like a good start! Will users of streaming need any polyfills to be able to make use of this? Can you show off some example usage scenario in code?

mislav · 2016-10-05T23:32:24Z

package.json

@@ -6,7 +6,7 @@
  "repository": "github/fetch",
  "license": "MIT",
  "devDependencies": {
-    "bower": "1.3.8",
+    "bower": "^1.7.9",


Is this change necessary?

Maybe not. But I got a warning saying one of Bowers depends was deprecated

Deprecation warnings are fine. They don't affect us at all.

Please undo the commit that made the change to bower version. It doesn't belong in this PR and was never necessary. As I said, deprecation warnings are acceptable.

mislav · 2016-10-05T23:33:18Z

fetch.js

+            return reader.read().then(function(result){
+              if(!result.done){
+                chunks.push(result.value)
+                return pump()


Is it possible to create a while loop rather than recursion? I guess not, because of async.

Don't think so

This is just for turning a stream into a blob/text/arraybuffer

We don't know in advance if they want a stream/blob/text or a arraybuffer. So I thought the best default responseType should be ms-stream or moz-chunked-arraybuffer instead of blob so we build all our Response method based on streams first

mislav · 2016-10-05T23:33:47Z

fetch.js

+          var pump = function(){
+            return reader.read().then(function(result){
+              if(!result.done){
+                chunks.push(result.value)


Doesn't it defeat the point of streaming if we've just read the entire response into memory at once?

Mm, no? You call blob, text or arraybuffer so you need to acembly all the chunks into one result.

Fair point!

jimmywarting · 2016-10-06T01:25:22Z

Thanks, this looks like a good start! Will users of streaming need any polyfills to be able to make use of this?

They need web-stream-polyfill if they want to use streams. ATM it's like an optional dependency.

Can you show off some example usage scenario in code?

Can show some example tomorrow. Just realized ’_bodyStream’ should just be named ’body’ to follow the spec

mislav · 2016-10-06T13:00:44Z

fetch.js

@@ -177,6 +177,8 @@
        this._bodyText = body.toString()
      } else if (!body) {
        this._bodyText = ''
+      } else if (body.getReader) {
+        this.body = body


Are we now allowing people to interface with response.body directly? Is this in the spec?

Yep, body is suppose to be a ReadableStream. You can test this directly in chrome's console log

res = new Response(new ReadableStream({})) reader = res.body.getReader() console.log(reader) // even this works: res = new Response(new Blob([])) reader = res.body.getReader() console.log(reader)

It's also suppose to solve abortable request by calling res.body.cancel() So literally every thing you pass into new Response should be transformed to ReadableStream IMO.

But I don't want to rush into things and change the hole polyfill

mislav · 2016-10-06T13:00:58Z

fetch.js

+          var pump = function(){
+            return reader.read().then(function(result){
+              if(!result.done){
+                chunks.push(result.value)


Fair point!

mislav · 2016-10-06T13:03:21Z

package.json

@@ -6,7 +6,7 @@
  "repository": "github/fetch",
  "license": "MIT",
  "devDependencies": {
-    "bower": "1.3.8",
+    "bower": "^1.7.9",


Deprecation warnings are fine. They don't affect us at all.

mislav

Thanks, this looks pretty good! One final thing we would need is tests for this. It's fine if the test suite imports a polyfill for streams. Ideally these tests would work in PhantomJS as well as a real browser.

mislav · 2016-10-06T14:32:46Z

fetch.js

@@ -204,6 +206,21 @@

        if (this._bodyBlob) {
          return Promise.resolve(this._bodyBlob)
+        } else if (this.body) {
+          var reader = this.body.getReader();


Minor: we don't use semicolons

mislav · 2016-10-06T14:33:21Z

fetch.js

+        } else if (this.body) {
+          var reader = this.body.getReader();
+          var chunks = [];
+          var pump = function(){


Minor: we usually put a space in between ) and { (same applies to next line)

mislav · 2016-10-06T14:33:36Z

fetch.js

+          var chunks = [];
+          var pump = function(){
+            return reader.read().then(function(result){
+              if(!result.done){


Minor: we usually put a space after if

mislav · 2016-10-06T14:36:25Z

fetch.js

@@ -216,6 +233,10 @@
      }

      this.text = function() {
+        if (this.body) {
+          return this.blob().then(readBlobAsText)


We have some checks in place (see consumed() below) to prevent reading the body data multiple times. Should the same restrictions also apply to readable streams?

jimmywarting · 2016-10-06T14:43:08Z

Will try to add some test and fix the minor issues.
The stream api already has some sort of consumed restriction in a way that you can not call body.getReader() twice...

jimmywarting · 2016-10-06T15:14:03Z

Just discovered this:

rs = new ReadableStream()
res = new Response(rs)
reader = res.body.getReader()
console.log(res.bodyUsed) // false
reader.read()
console.log(res.bodyUsed) // true

jimmywarting · 2016-10-06T15:29:39Z

Think I'm going to need a little bit of help with the test. Got a error:
Uncaught Error: Out of skills VM397:1

mislav · 2016-10-10T14:18:46Z

Not sure what that error is about, but PhantomJS is definitely going to need some ES shims before it can load the streams polyfill. PhantomJS has neither Symbol nor Object.assign support. Your test, however, runs but fails in Chrome with a different error. Perhaps you should work on making the test work in a real browser, and we can handle eventually getting it to run in PhantomJS.

mislav · 2016-10-10T14:49:54Z

@jimmywarting Here are my polyfills for PhantomJS. It still fails with a cryptic exception that's different than yours.

jimmywarting · 2016-10-10T17:08:19Z

Works in the browser now...

jimmywarting · 2016-10-10T21:12:17Z

Weird, running make test locally yields

  91 passing (196ms)
  8 pending
  0 failure

even the browser succeeded

EDIT script don't exit properly
Can't figure it out...
If the only thing that is included in the test is new ReadableStream() then the test won't exit...
However running $ PhantomJS hello-world.js with the polyfills necessary and a simple new ReadableStream() do cuz the script to exit without errors
Could it be that there is something mocha inject into all code like code-coverage do to test if all code has been executed?

I know Node runs until all event queues are empty could something like this be the cause?

jimmywarting · 2016-10-14T13:54:01Z

Do you have any idea how to tackle this? The test passes in browser but not in phantomjs

mislav · 2016-10-14T17:46:41Z

Not sure. We could just exclude this test from PhantomJS then. But that's a shame, since this test won't ever get exercised in CI, and we risk this functionality falling out of date during future changes.

…etch

jimmywarting · 2016-10-14T22:19:36Z

This looks good to me now...

mislav

There are several reasons why I feel this PR is not ready for merge yet, in addition to my individual comments below:

It adds the response.body property conditionally, meaning it won't be present unless the response is streamable. I have to check the spec about this, but I kind of feel that this property should always be available.
The tests are currently not confirming that the behavior is indeed streaming, just that response.body was present. Can we add something that asserts that we can use the streaming functionality?

mislav · 2016-10-14T23:18:00Z

fetch.js

@@ -177,6 +194,14 @@
        this._bodyText = body.toString()
      } else if (!body) {
        this._bodyText = ''
+      } else if (body.getReader) {
+        this.body = body
+        Object.defineProperty(this, 'bodyUsed', {


I'm not a fan of the more complex bodyUsed implementation, but I guess if the value of this property needs to be dependent on the status of body, and body object can be accessed and manipulated directly, then we can't avoid having the dynamic accessor. 🤔

I wasn't a big fan of this either, I even asked the community if it was okey to do this: whatwg/fetch#396

But it wasn't... We should maybe implement a new TypeError message that checks the locked property.
It's a bit more complicated to set the bodyUsed property since it needs set it upon the first getReader().read call and spying on this is not that easy...

mislav · 2016-10-14T23:19:55Z

fetch.js

+    var clone
+
+    if (this.bodyUsed) {
+      throw new TypeError('Failed to execute \'clone\' on \'Response\': Response body is already used')


If you used double quotes for the string, you wouldn't have to escape every single quote inside the string

I used it first but then the linter complained about using 2 different quotes...

Could as well fall thought and use tee()'s error message that already checks if stream is locked

mislav · 2016-10-14T23:22:04Z

test/test-worker.html

@@ -8,6 +8,8 @@
 <body>
  <div id="mocha"></div>
  <script src="/node_modules/url-search-params/build/url-search-params.js"></script>
+  <script src="/test/es6-shims.js"></script>


You can remove the commits that add es6-shims since that was intended for PhantomJS and we couldn't make it run in the end anyway

mislav · 2016-10-14T23:23:35Z

test/test.js

+  // Firefox & Edge partially support fetch (but don't have stream support)
+  // However the polyfilled Response do support it (With web-streams-polyfill)
+  // PhantomJS can use the polyfill but failes togheter with mocha
+  featureDependent(test, !isPhantomJS && support.stream, 'Response should accept ReadableStream', function() {


Instead of doing two separate checks !isPhantomJS && support.stream, support.stream should be false in the first place if the browser is PhantomJS. That simplifies this check.

mislav · 2016-10-14T23:25:15Z

package.json

@@ -6,7 +6,7 @@
  "repository": "github/fetch",
  "license": "MIT",
  "devDependencies": {
-    "bower": "1.3.8",
+    "bower": "^1.7.9",


Please undo the commit that made the change to bower version. It doesn't belong in this PR and was never necessary. As I said, deprecation warnings are acceptable.

jimmywarting · 2016-10-16T19:58:52Z

It adds the response.body property conditionally, meaning it won't be present unless the response is streamable. I have to check the spec about this, but I kind of feel that this property should always be available.

I would like response.body to always be there, But that would mean that this fetch polyfill will need to be dependent on some ReadableStream polyfill - I would be fine with that cuz that is what I'm after and why I'm trying to help you implement this.
If it was included then we could get rid of _bodyText, _bodyBlob, _bodyFormData and everything else since the bodyInit would always have to be transformed into a ReadableStream

Maybe we could implement some super-duper simple ReadableStream class that adds the minimal requirements with enqueue, getReader, close, cancel, read leaving everything like back pressure, piping and queuing strategy out

The logic around res.arrayBuffer(), res.blob() & res.text() could be implemented way more simpler if everything where built on top of ReadableStream (that is what i like most about jonnyreeves version that he is starting of from ReadableStream)
jonnyreeves/fetch-readablestream#3

https://greenkeeper.io/

eko24 · 2016-12-27T13:26:39Z

@mislav any chances to see this changes in master ?

mislav · 2016-12-27T14:48:14Z

@eko24 Not at the present moment. I'm mostly away from my computer around the New Year and I have to re-review this once I get back. We definitely want to support this, but I need to study the spec around this myself and see if we can minimize the diff further. The soonest I might be able to get back to this is 2 weeks from now.

eko24 · 2016-12-27T14:50:28Z

Ok, thank you for update ! вт, 27 дек. 2016 г. в 16:48, Mislav Marohnić <notifications@github.com>:

…

@eko24 <https://github.com/eko24> Not at the present moment. I'm mostly away from my computer around the New Year and I have to re-review this once I get back. We definitely want to support this, but I need to study the spec around this myself and see if we can minimize the diff further. The soonest I might be able to get back to this is 2 weeks from now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#404 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAuRAca-7o-g3BtqcpKbO1miHdNSM9x7ks5rMSUygaJpZM4KPVQq> .

Update dependencies to enable Greenkeeper 🌴

boczeratul · 2017-07-04T17:25:58Z

hello!
any update on this one?

Closes #2

…-1.1.0 chore(package): update url-search-params to version 1.1.0

mislav · 2019-05-21T12:02:19Z

@jimmywarting Thank you for your contribution and sorry that we have never reached a resolution regarding this! I have felt uneasy about all the code added to support this feature, which might prove to be tricky to maintain in the future and also I don't really see it being widely used. I'm calling this polyfill basically feature-frozen now #198 (comment)

jimmywarting · 2019-05-21T12:15:15Z

roger that

jimmywarting added 2 commits October 5, 2016 23:46

updated bower

083a49b

Response now accepts ReadableStream as body

d52b724

mislav reviewed Oct 5, 2016

View reviewed changes

jimmywarting added 2 commits October 6, 2016 03:49

Body should be a stream

94fef18

Forgot to rename this

186c7cf

mislav reviewed Oct 6, 2016

View reviewed changes

jimmywarting added 2 commits October 6, 2016 17:25

minor issues

30e0cc0

added 1 test for constructing a Response with a ReadableStream

cd3c6ff

mislav added 2 commits October 10, 2016 16:45

Add ES6 shims for streams polyfill in PhantomJS

372b7d3

Isolate the logic for reading a stream into a blob

d9d6c8e

jimmywarting added 2 commits October 10, 2016 19:07

Shouldn't be allowed to clone a used Response.

0521615

Set bodyUsed when stream is locked

66dc0e1

jimmywarting added 2 commits October 10, 2016 20:23

Less complex test

c420619

Made test pass successfully

857f064

Explicit exclude for phantomjs and for browser who partial supports f…

bf3db73

…etch

jimmywarting mentioned this pull request Oct 14, 2016

Possibility to enforce polyfill #170

Closed

mislav suggested changes Oct 16, 2016

View reviewed changes

jimmywarting added 2 commits October 16, 2016 22:34

removed PhantomJs tests

04ec1db

revert bower version

1aa6956

jimmywarting changed the title ~~Response now accepts ReadableStream as body~~ [WIP] Response now accepts ReadableStream as body Oct 16, 2016

jimmywarting added 3 commits October 17, 2016 15:12

Removed web-stream-polyfill for phantoms

4d48f89

removed more web-streams-polypill

64651a5

Other rejection

9f2af93

jimmywarting mentioned this pull request Oct 19, 2016

Full stream support FTW #413

Closed

chore(package): update dependencies

b133421

https://greenkeeper.io/

Merge pull request #1 from jimmywarting/greenkeeper/update-all

ef26149

Update dependencies to enable Greenkeeper 🌴

greenkeeper bot and others added 2 commits September 10, 2018 10:59

chore(package): update url-search-params to version 1.1.0

d69298d

Closes #2

Merge pull request #7 from jimmywarting/greenkeeper/url-search-params…

9746208

…-1.1.0 chore(package): update url-search-params to version 1.1.0

mislav closed this May 21, 2019

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2020

ghost deleted a comment Oct 21, 2022

[WIP] Response now accepts ReadableStream as body #404

[WIP] Response now accepts ReadableStream as body #404

Conversation

jimmywarting commented Oct 5, 2016 • edited Loading

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting commented Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

jimmywarting Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting commented Oct 6, 2016

jimmywarting commented Oct 6, 2016

jimmywarting commented Oct 6, 2016 • edited Loading

mislav commented Oct 10, 2016

mislav commented Oct 10, 2016

jimmywarting commented Oct 10, 2016

jimmywarting commented Oct 10, 2016 • edited Loading

jimmywarting commented Oct 14, 2016

mislav commented Oct 14, 2016

jimmywarting commented Oct 14, 2016

mislav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmywarting commented Oct 16, 2016 • edited Loading

eko24 commented Dec 27, 2016

mislav commented Dec 27, 2016

eko24 commented Dec 27, 2016 via email

boczeratul commented Jul 4, 2017

mislav commented May 21, 2019

jimmywarting commented May 21, 2019

jimmywarting commented Oct 5, 2016 •

edited

Loading

jimmywarting commented Oct 6, 2016 •

edited

Loading

jimmywarting Oct 6, 2016 •

edited

Loading

jimmywarting commented Oct 6, 2016 •

edited

Loading

jimmywarting commented Oct 10, 2016 •

edited

Loading

jimmywarting commented Oct 16, 2016 •

edited

Loading