-
Notifications
You must be signed in to change notification settings - Fork 565
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
Deprecating support for new Response(asyncIterator) #3540
Comments
That sounds great. A helper is not necessary I think, because you can do |
The helper is necessary for us to avoid the overhead of new Response(ReadableStream.from(fs.createReadStream('myfile'))) and (currently) new Response(fs.createReadStream('myfile')) |
@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:
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... |
Part of the problem of |
This was added long long ago, as we ported the |
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.
I thought it was important to mention that I didn't implement it as I'm defending it. :) |
@mcollina @KhafraDev I don't know why you keep saying Line 207 in b043a5f
So: it is not slower right now. Also you can do any optimization you could do (but do not do right now) for
@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 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. |
I was following Matteo wrt performance, but there is still a rather large issue not being mentioned.
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 |
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. |
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 The odd one out here in regards to
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. |
I 100% forgot about it nodejs/node#49551 (comment), my bad. I'm good to deprecate this if a spec change is not viable. |
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.
#2588 (comment) 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.
Every server environment does this for the aforementioned incompatibilities ( |
I think we are all in agreement, so in undici v7 (ideally Node.js v23) are we:
I'm ok either way. |
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 |
Deno has re-added support for async iterables. I think we can close this now? Please reopen if there's more we need to discuss here. |
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
The text was updated successfully, but these errors were encountered: