Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 #140

gwicke · 2016-08-03T04:23:41Z

Expose body as WhatWG ReadableStream, in line with the spec & browser implementations.
Remove charset detection, in line with the spec & browser implementations.
Add missing blob & arrayBuffer accessors in the response.
Migrate to a ES6 subset compatible with Node 4.
- Use ES6 accessors to lazily construct a ReadableStream only when needed.
Drop Node 0.10 & 0.12 support, and test Node 4 (LTS) & 6.

- Return a Node `Buffer` from the Response.blob() constructor. The `Buffer` signature is pretty much the same as `Blob`, so for now this might be a good compromise (until there is better `Blob` support on Node). - Return an `ArrayBuffer` from `Response.arrayBuffer()`. This is cheap, as Node's `Buffer` is implemented in terms of `ArrayBuffer` already. - Clean up `Body` state a bit: - Move _decode related vars to the closure, so that they are GC'ed as soon as possible. This is especially interesting for the _raw response chunks.

The fetch spec prescribes the use of a ReadableStream (from the streams spec) as response & request bodies. With ReadableStream exposed in recent Chrome, client-side code is starting to take advantage of streaming content transformations. To more faithfully implement the fetch spec & support isomorphic stream processing code, this patch adds support for ReadableStream, using the whatwg reference implementation: - `Response.body` is now a ReadableStream. - The `Body` constructor dynamically converts Node streams to `ReadableStream`, but still accepts FormData. - Requests accept ReadableStream, Node stream, or FormData bodies.

Apart from being cleaner, this will let us leverage ES6 getters & setters for lazy ReadableStream construction & type conversion.

- Remove charset detection, as this differs from the spec & browser implementations. Resolves #1. - Use ES6 getters for Request.body / Response.body access. This lets us avoid constructing a ReadableStream in the large number of cases where the full body is consumed with `.text()`, `.json()` or `.blob()`. - Add a fast path avoiding stream construction for request bodies in index.js.

- Convert to basic ES6. - Convert `fetch` into a regular function. This is in line with the spec & browser implementations, where calling `fetch` as a constructor is explicitly not supported. (Example: Chrome returns "TypeError: fetch is not a constructor".) Additionally, constructors masquerading as functions is no longer supported in ES6.

coveralls · 2016-08-03T04:24:53Z

Coverage decreased (-1.8%) to 98.208% when pulling f11ad21 on gwicke:blob_and_streams into 952aa2a on bitinn:master.

bitinn · 2016-08-03T04:27:28Z

Thx for the PR, but as great as it might be, I won't be merging it due to multiple breaking change. I will however keep it open for future reference.

coveralls · 2016-08-03T04:29:13Z

Coverage decreased (-1.8%) to 98.208% when pulling d459135 on gwicke:blob_and_streams into 952aa2a on bitinn:master.

Upstream [is not keen on incorporating breaking changes](node-fetch#140) like the removal of charset detection & the introduction of ReadableStream. In the meantime, we need a spec conforming `fetch` compatible polyfill that can be used in ServiceWorker code. So, for now, the path of least resistance seems to be to rename the project to node-fetch-polyfill & bump the version to 2.0.0 to signal breaking API changes.

jimmywarting · 2016-08-27T16:20:34Z

I think this would be a good starting point of v 2.0 or at least some of it

Could maybe wish for a v2.0 brach that we can push to instead and also that this PR was broken down to a few smaller pull requests instead of one big. or perhaps just keep this as a reference like you said

dandv · 2016-09-24T00:23:00Z

lib/body.js

@@ -15,37 +15,37 @@ const webStreams = require('node-web-streams');
 */
 class Body {
    constructor(body, opts) {
-		opts = opts || {};
+        opts = opts || {};


👍 ← click

TimothyGu

I just did some very preliminary review on Body.

Was there a reason why you changed the code formatting? It makes finding the difference in code a lot harder, when there are unrelated changes mixed in.

TimothyGu · 2016-10-04T09:02:51Z

lib/body.js

+
+    set body(val) {
+        this._rawBody = val;
+    }


This goes against the WHATWG spec, which specifies that

readonly attribute ReadableStream? body;

The readonly annotation means that the setter should be left blank.

TimothyGu · 2016-10-04T09:09:23Z

lib/body.js

+    /**
+     * Accumulate the body & return a Buffer.
+     *
+     * @return  Promise


Make clear what the type of the encapsulated data in the Promise is as well.

TimothyGu · 2016-10-04T09:13:20Z

lib/body.js

+    constructor(body, opts) {
+        opts = opts || {};
+        this._rawBody = body;
+        this.bodyUsed = false;


Since the Fetch spec declares bodyUsed as readonly you might want to do the getter/setter dance similar to rawBody and body.

TimothyGu · 2016-10-04T09:13:54Z

lib/body.js

+class Body {
+    constructor(body, opts) {
+        opts = opts || {};
+        this._rawBody = body;


You could make rawBody a Symbol

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

Elements of this commit come from #140 by Gabriel Wicke <gwicke@wikimedia.org>.

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

@gwicke

Elements of this commit come from #140 by @gwicke.

gwicke · 2016-10-09T05:22:50Z

This PR is pretty old btw. Current work is in the master branch of https://github.com/gwicke/node-fetch-polyfill.

@gwicke

Elements of this commit come from #140 by @gwicke.

@gwicke

Elements of this commit come from #140 by @gwicke.

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

gwicke added 12 commits July 28, 2016 15:03

Test node 4 & 6, and drop 0.10 / 0.12

a563ea4

Use node-web-streams package

7d8c8db

Small bug fix in blob()

2070763

Slightly smarter text() handling

54ca3fd

Convert body, request & response to ES6 syntax

00ae892

Apart from being cleaner, this will let us leverage ES6 getters & setters for lazy ReadableStream construction & type conversion.

Re-enable full charset detection

9e3a3e1

Add a basic .jshintrc

54552da

De-tabify body.js

f11ad21

Update README to reflect changes

d459135

dandv reviewed Sep 24, 2016

View reviewed changes

TimothyGu requested changes Oct 4, 2016

View reviewed changes

TimothyGu mentioned this pull request Oct 4, 2016

Work in progress: node-fetch v2 #177

Merged

8 tasks

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Body: store fewer things in the class

2ddfdfe

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Body: store fewer things in the class

4d89228

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Convert all files to ES2015

34136ee

Elements of this commit come from #140 by Gabriel Wicke <gwicke@wikimedia.org>.

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Body: store fewer things in the class

69aa182

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Convert all files to ES2015

97a1118

Elements of this commit come from #140 by @gwicke.

TimothyGu mentioned this pull request Oct 9, 2016

Convert all files to ES2015 #182

Merged

gwicke closed this Oct 9, 2016

TimothyGu added a commit that referenced this pull request Oct 9, 2016

Convert all files to ES2015

d50dce0

Elements of this commit come from #140 by @gwicke.

TimothyGu added a commit that referenced this pull request Oct 10, 2016

Convert all files to ES2015 (#182)

8380712

Elements of this commit come from #140 by @gwicke.

TimothyGu added a commit that referenced this pull request Oct 10, 2016

Body: store fewer things in the class

67dc74d

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

TimothyGu mentioned this pull request Oct 10, 2016

Miscellaneous improvements over #182 #183

Merged

TimothyGu added a commit that referenced this pull request Oct 13, 2016

Body: store fewer things in the class

9d3cc52

Incorporates some changes from #140, by Gabriel Wicke <gwicke@wikimedia.org>.

martinheidegger mentioned this pull request Dec 30, 2017

WebStream API support #387

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 #140

Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 #140

gwicke commented Aug 3, 2016

coveralls commented Aug 3, 2016 •

edited

bitinn commented Aug 3, 2016

coveralls commented Aug 3, 2016 •

edited

jimmywarting commented Aug 27, 2016 •

edited

dandv Sep 24, 2016 •

edited

TimothyGu left a comment

TimothyGu Oct 4, 2016 •

edited

TimothyGu Oct 4, 2016

TimothyGu Oct 4, 2016 •

edited

TimothyGu Oct 4, 2016

gwicke commented Oct 9, 2016

Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 #140

Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 #140

Conversation

gwicke commented Aug 3, 2016

coveralls commented Aug 3, 2016 • edited

bitinn commented Aug 3, 2016

coveralls commented Aug 3, 2016 • edited

jimmywarting commented Aug 27, 2016 • edited

dandv Sep 24, 2016 • edited

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Oct 4, 2016 • edited

Choose a reason for hiding this comment

TimothyGu Oct 4, 2016

Choose a reason for hiding this comment

TimothyGu Oct 4, 2016 • edited

Choose a reason for hiding this comment

TimothyGu Oct 4, 2016

Choose a reason for hiding this comment

gwicke commented Oct 9, 2016

coveralls commented Aug 3, 2016 •

edited

coveralls commented Aug 3, 2016 •

edited

jimmywarting commented Aug 27, 2016 •

edited

dandv Sep 24, 2016 •

edited

TimothyGu Oct 4, 2016 •

edited

TimothyGu Oct 4, 2016 •

edited