Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6 by gwicke · Pull Request #140 · node-fetch/node-fetch

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

 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

lib/body.js

class Body {

constructor(body, opts) {

opts = opts || {};

opts = opts || {};

Copy link

Contributor

dandv Sep 24, 2016 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 ← click

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

Uh oh!

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

Web streams (ReadableStream) support, blob & arrayBuffer accessors, ES6#140
gwicke wants to merge 13 commits intonode-fetch:masterfrom
gwicke:blob_and_streams

gwicke commented Aug 3, 2016

Uh oh!

coveralls commented Aug 3, 2016 •

edited

Loading

Uh oh!

bitinn commented Aug 3, 2016

Uh oh!

coveralls commented Aug 3, 2016 •

edited

Loading

Uh oh!

jimmywarting commented Aug 27, 2016 •

edited

Loading

Uh oh!

dandv Sep 24, 2016 •

edited

Loading

Uh oh!

TimothyGu left a comment

Uh oh!

TimothyGu Oct 4, 2016 •

edited

Loading

Uh oh!

TimothyGu Oct 4, 2016

Uh oh!

TimothyGu Oct 4, 2016 •

edited

Loading

Uh oh!

TimothyGu Oct 4, 2016

Uh oh!

gwicke commented Oct 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

gwicke commented Aug 3, 2016

Uh oh!

coveralls commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bitinn commented Aug 3, 2016

Uh oh!

coveralls commented Aug 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jimmywarting commented Aug 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dandv Sep 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu Oct 4, 2016

Choose a reason for hiding this comment

Uh oh!

gwicke commented Oct 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coveralls commented Aug 3, 2016 •

edited

Loading

coveralls commented Aug 3, 2016 •

edited

Loading

jimmywarting commented Aug 27, 2016 •

edited

Loading

dandv Sep 24, 2016 •

edited

Loading

TimothyGu Oct 4, 2016 •

edited

Loading

TimothyGu Oct 4, 2016 •

edited

Loading