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

regression: Response does not accept a async-iterator anymore #49551

Closed
mcollina opened this issue Sep 8, 2023 · 23 comments
Closed

regression: Response does not accept a async-iterator anymore #49551

mcollina opened this issue Sep 8, 2023 · 23 comments
Labels
fetch Issues and PRs related to the Fetch API tsc-agenda Issues and PRs to discuss during the meetings of the TSC.

Comments

@mcollina
Copy link
Member

mcollina commented Sep 8, 2023

#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
Copy link
Member Author

mcollina commented Sep 8, 2023

cc @Trott

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 8, 2023

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
Copy link

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

@mcollina
Copy link
Member Author

mcollina commented Sep 8, 2023

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
Copy link

lucacasonato commented Sep 8, 2023

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
Copy link
Member Author

mcollina commented Sep 8, 2023

Thanks for the explanation!

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

@KhafraDev
Copy link
Member

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

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Sep 8, 2023

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
Copy link

lucacasonato commented Sep 8, 2023

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
Copy link
Member

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
Copy link
Member

ronag commented Sep 8, 2023

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
Copy link
Member

ronag commented Sep 8, 2023

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

@VoltrexKeyva VoltrexKeyva added the fetch Issues and PRs related to the Fetch API label Sep 9, 2023
@lucacasonato
Copy link

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
Copy link
Member

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
Copy link

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
Copy link
Member

KhafraDev commented Nov 3, 2023

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
Copy link

lucacasonato commented Nov 3, 2023

@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
Copy link

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 mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Mar 19, 2024
@mcollina
Copy link
Member Author

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
Copy link

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
Copy link
Member

targos commented Mar 20, 2024

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
Copy link
Member Author

mcollina commented Mar 20, 2024

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
Copy link

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetch Issues and PRs related to the Fetch API tsc-agenda Issues and PRs to discuss during the meetings of the TSC.
Projects
None yet
Development

No branches or pull requests

8 participants