regression: Response does not accept a async-iterator anymore

[mcollina]

#48395 which shipped in Node v20.6.0 broke part of the fetch() spec and was caught by CITGM with some of the undici test failing everywhere.

This stopped working:

  const asyncIterable = {
    async * [Symbol.asyncIterator] () {
      yield 'a'
      yield 'b'
      yield 'c'
    }
  }

  const response = new Response(asyncIterable)
  await response.text()

I'm shipping a fix in undici in nodejs/undici#2251.

Ref. nodejs/citgm#959

(FWIW we should have caught this before release)

[mcollina]

cc @Trott

[nicolo-ribaudo]

Q (coming here from the citgm PR with zero background 😅): what is the expected behavior of that code? In browser, await response.text() returns [object Object] because Response does not have special handling for async iterables.

[lucacasonato]

@mcollina maybe your example is wrong, but you can't pass AsyncIterables as response bodies directly. The only valid streaming BodyInit is ReadableStream.

[mcollina]

maybe your example is wrong, but you can't pass AsyncIterables as response bodies directly. The only valid streaming BodyInit is ReadableStream.

Have you got a spec reference @lucacasonato?
That used to work before, and it broke.
I think it's part of our backward-compat with node-fetch (we lifted some of the tests there to minimize disruption).

@KhafraDev wdyt?

[lucacasonato]

• Response interface first argument is BodyInit
• BodyInit is either ReadableStream or XMLHttpRequestBodyInit
• XMLHttpRequestBodyInit is either Blob, BufferSource (Uint8Array, ArrayBuffer etc), FormData, URLSearchParams, or USVString

None of those types are IDL types that can be created directly from an async iterable / async iterator.

Your example should be new Response(ReadableStream.from(asyncIterator)); to be spec compliant.

[mcollina]

Thanks for the explanation!

@KhafraDev should we change this or leave as-is for backward compat with node-fetch?

[KhafraDev]

This was added so fetch could support node streams. We should definitely keep it.

[dnalborczyk]

doesn't seem to be great for interoperable code between runtimes and browsers. that said, how would users know about this? the docs are just mentioning Response as a browser compatible global: https://nodejs.org/api/globals.html#response

This was added so fetch could support node streams.

I haven't looked into this, but doesn't node provide a node stream/web api stream converter?

[lucacasonato]

This was added so fetch could support node streams. We should definitely keep it.

@KhafraDev Fetch can support node streams through new Response(ReadableStream.from(nodeStream)).

It is obviously not my call to make, but I want to advise you that keeping this behaviour is a dangerous hazard for future spec compatibility for Node.js. This is because if fetch gains support for other object types in the BodyInit position in the future, and these objects happen to already implement the async iterator protocol, and a developer is already passing these objects in a BodyInit position, then the behaviour that the developer experiences may breakingly change when Node is updated with that spec change.

If Node did not accept arbitrary async iterables, but rather only branded node streams in the BodyInit position, your usecase would be resolved without exposing Node to this dangerous future compatibility hazard. I think this would be a much safer alternative you could consider.

[KhafraDev]

that said, how would users know about this?

It's mentioned in the undici docs, there probably should be more extensive docs for fetch in node core.
https://github.com/nodejs/undici#requestbody

@lucacasonato

I agree with you and it wasn't my idea to support arbitrary async iterables, but I don't think it's as harmful as it's being made out to be. We've already implemented breaking changes that were made in the spec that broke users, so it would be the same here. For example, when a change was introduced that removes Authorization headers from cross-origin redirects or when a duplex: 'half' option started being required when passing a ReadableStream as a body. All environments choose to go against the spec in some capacity, and for node's use case it makes sense to support async iterables.

I couldn't find past discussions but @ronag added it so he might be able to give a better answer.

[ronag]

It was added so that node-fetch could be more easily be replaced with native fetch. The value was deemed high and any possible negative side effects were considered negligible.

Not supporting it is an unnecessary breaking change.

[ronag]

Ref: whatwg/fetch#1291
Ref: whatwg/fetch#809

[lucacasonato]

Folks now think this is standard behavior and are opening issues on the Deno repo about us not supporting this. This is why you don't do random willy nilly spec extensions folks 🙄

denoland/deno#21063

[KhafraDev]

Deno doesn't support forbidden headers, is that not also a "willy nilly spec extension"? Node supported them at one point, and removed it to match Deno's behavior (and we've had multiple issues regarding it).

[lucacasonato]

We discussed that issue at length in WinterCG. Also it was a spec extension because it obviously was never going to make sense in browsers so the only path forward was a spec extension. It was as such not "willy nilly"

[KhafraDev]

deno's handling of forbidden headers existed before wintercg's inception. If not that, Deno had special handling for set-cookie headers before getSetCookie. I don't see these as any different from node/undici supporting async iterable bodies.

[lucacasonato]

@KhafraDev Sorry, I was a bit tired yesterday when I wrote this message and it wasn't very nice. I apologize for my wording - I shouldn't have been so confrontational.

The extensions that we made to the spec were all born out of necessity - both of the cases you mentioned where not solvable without spec extensions. We also put in significant effort in either case to align with other runtimes, and coordinate upstreaming the extensions where possible.

In the case of the forbidden headers, we had no path to upstreaming, and so we did the next best thing which was to deviate from spec, but to align to other existing implementations (Cloudflare Workers).

In the case of set-cookie, we had a path to upstream so we quickly engaged with WHATWG to upstream the relevant changes. We shipped them prior to the spec change landing, because we were confident the design wouldn't change, but had it changed we would have done the whole "implement new thing, deprecate, unship" dance for the old non-compliant version.

I was just a bit annoyed at the async iterables as body deviation because it was not at all driven by spec necessity (ie no other way to do this), because trivial workarounds are possible. For example, using node:stream's Readable.toWeb() (or since more recently ReadableStream.from()).

Anyway, I don't want to argue with you about this, I just wanted to show that this creates ecosystem confusion and portability hazards that may not have been considered when originally shipping this, and may be a good trigger to re-evaluate whether this deviation should exist. I'd be happy to talk this through thoroughly at the next WinterCG call if you are interested.

[lucacasonato]

Another user was confused, thinking this was standard behaviour: withastro/astro#10495 (comment).

I want to once again suggest that you unship this behaviour as long as there is no written specification for the behaviour, or consensus for including this behaviour in the Fetch spec via WHATWG. I want to reiterate that users relying on this behaviour have a trivial workaround using ReadableStream.from() or Readable.prototype.toWeb().

[mcollina]

This specific issue can be closed, I think, because we ship the non-spec compliant behavior.

I think we should keep shipping it as it massively simplifies interoperability in a performant way. The proposed workaround is significantly slower because creating WHATWG streams is so much slower for us.

[lucacasonato]

I disagree strongly @mcollina. This significantly hurts interop with code that should run in both browsers, Deno and Node. You can special case new Response(ReadableStream.from(asynReadable)) to behave in a more performant way, in a completely spec compliant manner.

It is very unfortunate that instead of speeding up ReadableStream.from for this use case (which is absolutely doable), Node has decided to ship this spec incompatible API that is actively hurting cross runtime interoperability.

[targos]

I think what @mcollina means is that another issue should be opened wrt the interop question. The regression that this issue was about isn't here anymore.

[mcollina]

I was wrong about the performance impact, as right now there is no difference. Currently, we just call ReadableStream.from(). However:

1. new Response(asyncIterator) was supported before Node.js had ReadableStream.from(), which was added in Node.js v20.6.0. Note that we have supported this since the very beginning of fetch() in Node.js
2. undici had this since we shipped fetch(), and it was added to ease the transition from node-fetch, which we lifted the test suite from. IMHO this is expected behavior on the Node.js platform, mostly due to the great work done by node-fetch, which is still downloaded 55 million times per week.
3. This is a pure addition, and it does not prevent cross-platform compatibility. Unfortunately, it seems we are strong-armin other runtimes to implement it (Bun implements it). I apologize for this and it wasn't our intention.

I think it's way too late to remove this for Node.js. This is definitely a breaking change and it would cause significant breakage for v18 and v20 users.

As @targos said, this issue was about a regression for something that has already been there for quite some time and it regressed. @lucacasonato feel free to open a fresh issue about this, here or in the undici repo.

My 2 cents is that the best solution for this is try to standardize this behavior and re-assess.

[lucacasonato]

Just want to ping here again that Node shipping this without standards discussion is super frustrating, because it turns out standardization of this feature is not that easy because Node was not following prior precedent in ECMAScript and Web APIs by not handling [Symbol.iterator] at all in your implementation. See whatwg/fetch#1291 (comment)