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

WebStream API support #387

Open
martinheidegger opened this issue Dec 30, 2017 · 24 comments
Open

WebStream API support #387

martinheidegger opened this issue Dec 30, 2017 · 24 comments
Labels

Comments

@martinheidegger
Copy link

This has been sort of worked on in #140 but then was continued in another branch #140 (comment).

Is the WebStream API, that is published in FF & Chrome, like getReader() etc. too experimental for node-fetch?

@jimmywarting
Copy link
Collaborator

jimmywarting commented Feb 2, 2018

Not sure it would be useful in a nodejs enviorment where everything is written with node streams.
the WebStream api isn't small and would add quite alot to node-fetch dependency.

There is a grate package called node-web-streams for conversion between those two

const webStreams = require('node-web-streams');
const readableStream = webStreams.toWebReadableStream(nodeReadable);

if we where to implement ReadableStream in node-fetch I'm guessing most ppl would just to the opposite and try to convert it back to a node stream

It would be a breaking change since Response.prototype.body is now a node stream and some are already using it.

@ThisIsMissEm
Copy link

I've recently been implementing the cloudflare-workers API as a local server for development, and support for WebStreams is something we need. Would it be possible to support passing in a set Stream constructors for node-fetch to use?

That way if people want the web-streams API, they can opt-in to it.

@TimothyGu
Copy link
Collaborator

@ThisIsMissEm There are nontrivial differences in the stream.Readable API vs. the web ReadableStream API, and using the constructors of the two classes the same way unfortunately doesn't work. See some examples for Web Streams vs. Node.js Streams.

There have some exciting developments over on the Node.js side (nodejs/node#22352), and if Web Streams eventually get in Node core we can certainly add support for it.

(Does this answer your question or am I missing something you are trying to say?)

@ThisIsMissEm
Copy link

Yeah, we realised this was the case, so we've forked node-fetch to use ReadableStream API. You can see the start of our attempt here: master...titel-media:whatwg-readablestreams

@ThisIsMissEm
Copy link

Whilst my usage of node-fetch is for reimplementing the API of CloudFlare Workers, I think it’s worth noting that several modules that depend on node-fetch claim that they expose the whatwg-fetch spec, which isn’t true, as that’d mean they expose whatwg streams; They’ve possibly missed the note about difference in stream APIs

@ThisIsMissEm
Copy link

@TimothyGu We actually have pretty much full tests passing — I removed a bunch of stuff and can do a PR, but in order for my team to move forward we'll be publishing a scoped fork. It's up to you if you want to take my changes into node-fetch, I'm happy to have a fork for now.

@jimmywarting
Copy link
Collaborator

jimmywarting commented Aug 30, 2018

Found out something that is going to make this cross browser/node chunk reading much simpler! Async iterator!!!

Node implemented Symbol.asyncIterator into there stream specification (v10) and whatwg are planing on adding them too, until then here is a polyfill for the browser:

if (!ReadableStream.prototype[Symbol.asyncIterator]) {
  ReadableStream.prototype[Symbol.asyncIterator] = async function* () {
    const reader = this.getReader()
    while (1) {
      const r = await reader.read()
      if (r.done) return value
      yield r.value
    }
  }
}

now what you can do in both the browser and Node is:

const res = await fetch('https://httpbin.org/bytes/500000')
for await (const chunk of res.body) {
  console.log(chunk)
}

Both yields a Uint8Array array (well, a buffer in node, but they are inherited by Uint8array) so you can treat them the same

Hopefully this will make the WebStream api for node-fetch more irrelevant
(still think the node-web-streams is to much as a dependency)


this is something i would very much like to see in the readme 😛

@TimothyGu
Copy link
Collaborator

TimothyGu commented Aug 30, 2018

@jimmywarting Unfortunately, async iterators don't provide a full replacement for streams with regards to things like backpressure. This is obviously a step in the right direction but there is still value in exposing Web streams.

For context, a PR that adds async iterators to web streams is whatwg/streams#950.

@ThisIsMissEm
Copy link

@jimmywarting wow! That makes it SO much nicer than using the reader / while loop interface.

@ThisIsMissEm
Copy link

ThisIsMissEm commented Aug 31, 2018

@TimothyGu wouldn't backpressure with async iterator just be handled via something like:

const res = await fetch('https://httpbin.org/bytes/500000')
for await (const chunk of res.body) {
  await doSomework(chunk)
}

So because we're delaying in each iteration backpressure applies?

@TimothyGu
Copy link
Collaborator

Hm, I guess I was more referring to the highWatermark-based backpressure. That does indeed work, yes.

@ThisIsMissEm
Copy link

Okay, so, an update from my end: We're publishing a fork with ReadableStreams as @titelmedia/node-fetch, such that we can use it in our Cloudflare Worker development server. It's up to you @TimothyGu if you choose to adopt the changes I've made.

@jimmywarting
Copy link
Collaborator

Think you should have highlighted the differences between your fork and node-fetch in the readme a little better

@ThisIsMissEm
Copy link

Oh, yes.. I forgot about that. The package description has it though.

@bitinn
Copy link
Collaborator

bitinn commented Oct 24, 2018

@TimothyGu not sure your take on this but the nodejs core web stream PR didn't land; it seems the only viable solution is to use https://github.com/MattiasBuelens/web-streams-polyfill for now.

@bitinn
Copy link
Collaborator

bitinn commented Nov 6, 2018

Some good news:

https://github.com/nodejs/whatwg-stream

@jimmywarting
Copy link
Collaborator

it might be ok to leave them as a buffer internally, as long as res.body getter is guaranteed to be a stream (the debate is then whether it should be a web stream or nodejs stream...) - #547 (comment)

So what is the verdict? should we implement web-streams-polyfill for v3?

I guessing some ppl will complain about the switch from node stream to whatwg streams

@bitinn
Copy link
Collaborator

bitinn commented Jan 16, 2019

@jimmywarting don't think there is a verdict yet, stream is a different spec which I believe isn't quite finished yet, and browser support isn't quite on-par the last time I checked.

I recently thought about what if Fetch options allow users to pass in a stream implementation (node or web), and use that instead, just like how we handled AbortController support.

And since there is a lack of man-power on this issue, another likely outcome is we wait for nodejs to implement it, which will be a long while.

See also: MattiasBuelens/web-streams-polyfill#1

@jimmywarting
Copy link
Collaborator

I kinda feel like it's the right step forward to implement whatwg streams (to be isomorphic compatible cross node and browser) while at the same time i also feel like node-fetch is not quite ready for it yet.

here is some other ideas i thought of to keep the users that still uses node-streams events and pipes happy. We could implement a non standard function body.stream() that returns the internal node stream and let body.body be a whatwg stream

Or make some utility function something like

const asNodeStream = require('fetch/as-node-stream') // or
const { asNodeStream } = require('fetch')

fetch(url).then(res => {
  asNodeStream(res.body) // returns the internal body stream
   .pipe(dest)
})

Another idea is the use of symbol.

const { internalNodeStreamSymbol } = require('fetch')

fetch(url).then(res => {
  res[internalNodeStreamSymbol].pipe(dest)
})

@josephrocca
Copy link

josephrocca commented Jun 29, 2019

@ThisIsMissEm I can't thank you enough for sharing that fork! After an hour or two of trying to mash polyfills together unsuccessfully I thought I was going to have to implement something myself.

(sorry for the noise)

@loynoir
Copy link

loynoir commented Dec 10, 2021

Recently I found https://github.com/nodejs/undici

  • whatwg readablestream
  • have getReader
  • but need node16 to have stream/web, AbortController, ...

@jimmywarting
Copy link
Collaborator

We got plans to bring in whatwg streams polyfill and change the response body to whatwg streams for v4, but it's going to be a breaking change... would like to fix some issues before making that release

@RangerMauve
Copy link

I've been having great success with web-streams-polygill inside the make-fetch library.

Wasn't too hard to migrate to it from node streams.

@BasixKOR
Copy link

BasixKOR commented Feb 8, 2022

By breaking change, do you mean that v4 no longer supports Node.js stream? I'm a bit confused.

danielbankhead pushed a commit to googleapis/gaxios that referenced this issue Sep 11, 2024
`node-fetch` does not yet support webstreams, which is required for `.body` node-fetch/node-fetch#387
danielbankhead added a commit to googleapis/gaxios that referenced this issue Oct 25, 2024
* feat!: Upgrade to `node-fetch` v3

* refactor: Use native `FormData` in testing

* feat!: Improve spec compliance

* test(temp): Run 18+

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat: Improve types, streamline, and standardize

* test: Adopt `Headers` type from merge

* feat: Introduce `GaxiosOptionsPrepared` for improved type guarantees

* feat: Ensure `GaxiosOptionsPrepared.url` is always a `URL`

* refactor: Clean-up Types & Resolve lint warnings

* refactor: streamline `.data` for `Response`

* docs: Simplify example

* refactor: streamline, no error case

* feat: Basic `GET` Support for `Request` objects

`node-fetch` does not yet support webstreams, which is required for `.body` node-fetch/node-fetch#387

* test: remove `.only`

* refactor: simplify `httpMethodsToRetry`

* chore: update `nock`

* test: cleanup

* fix: `File` in Node 18

* docs: clarification for node-fetch `.data`

* chore: fix webpack for node-fetch v3

* docs: clarifications

* fix: Types for Node.js-only environments

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
danielbankhead added a commit to googleapis/gaxios that referenced this issue Oct 29, 2024
* feat!: Upgrade to `node-fetch` v3

* refactor: Use native `FormData` in testing

* feat!: Improve spec compliance

* test(temp): Run 18+

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat: Improve types, streamline, and standardize

* test: Adopt `Headers` type from merge

* feat: Introduce `GaxiosOptionsPrepared` for improved type guarantees

* feat: Ensure `GaxiosOptionsPrepared.url` is always a `URL`

* refactor: Clean-up Types & Resolve lint warnings

* refactor: streamline `.data` for `Response`

* docs: Simplify example

* refactor: streamline, no error case

* feat: Basic `GET` Support for `Request` objects

`node-fetch` does not yet support webstreams, which is required for `.body` node-fetch/node-fetch#387

* test: remove `.only`

* refactor: simplify `httpMethodsToRetry`

* chore: update `nock`

* test: cleanup

* feat!: Headers should be `Headers`

* fix: Multipart Headers

* refactor: clean-up and optimize

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
danielbankhead added a commit to googleapis/gaxios that referenced this issue Oct 30, 2024
* feat!: Upgrade to `node-fetch` v3

* refactor: Use native `FormData` in testing

* feat!: Improve spec compliance

* test(temp): Run 18+

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

* feat: Improve types, streamline, and standardize

* feat!: Do Not Treat Buffers as JSON By Default

* test: Adopt `Headers` type from merge

* feat: Introduce `GaxiosOptionsPrepared` for improved type guarantees

* feat: Ensure `GaxiosOptionsPrepared.url` is always a `URL`

* refactor: Clean-up Types & Resolve lint warnings

* refactor: streamline `.data` for `Response`

* docs: Simplify example

* refactor: streamline, no error case

* docs: clarification

* feat: Basic `GET` Support for `Request` objects

`node-fetch` does not yet support webstreams, which is required for `.body` node-fetch/node-fetch#387

* test: remove `.only`

* refactor: simplify `httpMethodsToRetry`

* chore: update `nock`

* test: cleanup

* test: add test for asserting no default `content-type`

* docs: Add detail for `code` property

* docs: uniform

---------

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants