Deprecating support for new Response(asyncIterator)

[mcollina]

Given nodejs/node#49551 (comment), let's consider deprecating new Response(asyncIterator) and adding some kind of utility to do the same.

Response.fromAsyncIterator(asyncIterator) or something similar.

This would be easier to detect/polyfill than new Response(asyncIterator) and increase spec adherence.

Ideally, we could runtime deprecate this in Node.js v23, then remove it in Node.js v24.

cc @KhafraDev @lucacasonato

[lucacasonato]

That sounds great. A helper is not necessary I think, because you can do new Response(ReadableStream.from(asyncIterator))

[mcollina]

The helper is necessary for us to avoid the overhead of ReadableStream.from(), as there is a significant perf difference between:

new Response(ReadableStream.from(fs.createReadStream('myfile')))

and (currently)

new Response(fs.createReadStream('myfile'))

[KhafraDev]

@lucacasonato why does the webidl async iterable type fallback to using Symbol.iterator? Wouldn't the issue be solved by not doing this - even if it goes against precedent?

Right now node handles this exactly how the fetch spec will once workarounds are added:

new Response(new String("hello world")).text().then(console.log) // 'hello world'

We also seem to have a stalemate as to its replacement:

• We need(ed) support for using node streams in Response, hence adding async iterable support.
• We can't use ReadableStream.from because of perf.
• Adding a new static method means we're still not spec compliant, just in a different way.

e: I was not the one who added it. When I implemented the webidl spec I accidentally got rid of it, which lead to nodejs/node#49551 being opened.

Once async iterable support officially lands in the spec, I'll update our implementation to match the spec, but getting rid of something we know is widely used...

[mcollina]

Adding a new static method means we're still not spec compliant, just in a different way.

Part of the problem of new Response(fs.createReadStream('myfile')) is that it's not easy to detect support for the caller. Having an explicit function could be easily detected: if (Response.fromAsyncIterator(...)) { ... }.

[mcollina]

I was not the one who added it.

This was added long long ago, as we ported the node-fetch test. We have done this to provide the easiest upgrade path from node-fetch.

[KhafraDev]

It's already possible to support async iterables in a spec compliant and cross-platform way as mentioned above, via ReadableStream.from. Adding in a new, non-spec api for better performance is a different issue.

This was added long long ago

I thought it was important to mention that I didn't implement it as I'm defending it. :)

[lucacasonato]

@mcollina @KhafraDev I don't know why you keep saying ReadableStream.from is slower - @mcollina already debunked this in March (nodejs/node#49551 (comment)). I checked now, and you are still using ReadableStream.from internally so there is no performance difference:

undici/lib/web/fetch/body.js

Line 207 in b043a5f

object instanceof ReadableStream ? object : ReadableStreamFrom(object)

So: it is not slower right now. Also you can do any optimization you could do (but do not do right now) for new Response(asyncIterable) with new Response(ReadableStream.from(asyncIterable)) too? ReadableStreams are opaque objects so if a user doesn't read before passing it to readable, you could just extact the inner async iterable out again for optimizations. Deno has optimizations like this for new Response(res.body) for example.

Why does the webidl async iterable type fallback to using Symbol.iterator?

@KhafraDev Because that is what browsers do for all other APIs that take async iterables, both in JS and in Web specs. We are not going to be able to ship this to browsers unless we also handle this case.

Considering that the only stated usecase for not using ReadableStream.from() is perf (which is debunked to not be correct), maybe it is time to reasses?

I am stuggling to find motiviation to continue on the spec change here. It is difficult to get right, and will require a lot of baby sitting to ensure we do not break web compat. If we can not ship this in browsers because of web compat, Node will have to figure out what to do instead anyway. The proposal from Matteo to just get rid of this non standard behaviour in Node would probably be the simplest path forward. I am not planning to stop working on the spec change right now, but I can not guaruantee I will finish it.

[KhafraDev]

I was following Matteo wrt performance, but there is still a rather large issue not being mentioned.

• node does not benefit from removing async iterable support.
• webidl limitations make adding official support difficult.
• ReadableStream.from was added in node v20.6.0. The one you're referring to is from undici - undici doesn't use primordials (side note: does not appear to support Symbol.iterator). undici needs to support node v18 since it's LTS, those users wouldn't be able to use async iterables under any capacity if we removed it.

I don't understand Deno's position to be staunchly opposed to adding async iterator support without it being supported by the spec. This has not been a problem in the past with redirect: 'manual', forbidden headers, etc. There's a simple workaround for those who want interoperability.

[KhafraDev]

I have actively been working on reducing our fetch's incompatibilities with the spec, but I cannot see any benefit for our users by removing this right now - once we drop v18 support I wouldn't be totally against it. It's unfortunate that precedents might kill the proposal.

[lucacasonato]

webidl limitations make adding official support difficult.

I don't know what you are referring to - there is no WebIDL limitation. I am the one writing the WebIDL spec part for async iterables. The Symbol.asyncIterator to Symbol.iterator fallback has been engrained everywhere in the JS ecosystem for many many years. JS has language syntax that does this for example: for await (const x of syncIterator) {} has been supported for many years. That is why we would not ship anything on the web that does not follow this precedent. If we regretted this precedent maybe the story would be different - but we don't. The ecosystem keeps doing this fallback, including in new JS built in APIs, like the upcoming AsyncIterator.from() static method from async iterator helpers.

The odd one out here in regards to Symbol.iterator handling is unfortunately Node.

I don't understand Deno's position to be staunchly opposed to adding async iterator support without it being supported by the spec. This has not been a problem in the past with redirect: 'manual', forbidden headers, etc. There's a simple workaround for those who want interoperability.

Because there are good spec compliant alternatives. There were no good alternatives for the things you mentioned. We are working on standardizing this, because we think there can be some value. But unless browsers agree to shipping this (and they will not if we can not convincingly show that this wont break existing websites), this will not be standardized and we will likely not attempt to ship it again in Deno either because we do not want to break our users.

(We attempted to ship the PR experimentally in the last release, but had to revert because it was incompatible for existing users, like Hono users: denoland/deno#25278)

Right now Node is unfortunately propagating the incompatibility here, because people are writing new code that works in Node, and then does not work in other environments that are spec compliant. If Node at least warned and said "Don't do this, use 'ReadableStream.from' instead" if you use an async iterable, people would at least stop writing new code that relies on this feature.

[mcollina]

I 100% forgot about it nodejs/node#49551 (comment), my bad.

I'm good to deprecate this if a spec change is not viable.

[KhafraDev]

(We attempted to ship the PR experimentally in the last release, but had to revert because it was incompatible for existing users, like Hono users: denoland/deno#25278)

It was compatible with webidl, but not node's implementation. Node's implementation does not have the same issues because there's no Symbol.iterator support.

If Node at least warned and said "Don't do this, use 'ReadableStream.from' instead" if you use an async iterable, people would at least stop writing new code that relies on this feature.

#2588 (comment)
"I don't think the warning would be ok for Node.js itself on the released lines."

We already mention it in the docs that this behavior isn't present in the fetch spec - https://github.com/nodejs/undici?tab=readme-ov-file#requestbody. People choose to use it because it has value, as you mentioned. We cannot remove it entirely for node v18 users, nor should be drop support for a version that's in maintenance mode IMO.

Right now Node is unfortunately propagating the incompatibility here, because people are writing new code that works in Node, and then does not work in other environments that are spec compliant.

Every server environment does this for the aforementioned incompatibilities (redirect: 'manual' & forbidden headers). In node there are alternatives to these (undici itself is an alternative); there is still no alternative for v18 users regarding async iterable support. I don't count polyfilling ReadableStream.from a viable alternative.

[mcollina]

I think we are all in agreement, so in undici v7 (ideally Node.js v23) are we:

1. emitting a warning
2. throwing an error

I'm ok either way.

[KhafraDev]

1. We would be warning people using node 18 not to use something that has no alternative (unless we consider polyfilling ReadableStream.from an alternative...). Would we only be warning v20+ users?
2. We can't throw an error for the same reason.

I see no reason to drop v18 support over this. I am not convinced about the other reasons either, tbh.

It also puts us at a disadvantage: we would be removing something that might get re-added if the spec PR is ever merged. Depending on the spec PR, it's entirely possible that a) undici v6 works with async iterables; b) a v7 release doesn't/warns about it; and c) a v7/v8(?) release supports async iterables again.

We know that people are using it - in fact, I think it was someone(s) reporting bugs/feature requests in Deno that triggered the initial comments in the node core issue. We know that it has value (as admitted above), and we know that there is a spec update in progress officially supporting async iterables. We would be hurting our users over...?

It should have never landed, but I don't think there's benefit for us in removing it. Out of all the other spec incompatibilities we have, this one has never created security vulnerabilities (removing forbidden headers), has never caused bugs (same one), and has never been a burden to maintain (same one, supporting a dispatcher option (two level deep parenthesis: Deno has a client option IIRC)).