fix: use ReadableStream for response object if deno

[satyarohith]

This patch allows astro to run in node-compat mode in Deno. Deno doesn't support construction of response from async iterables in node-compat mode so we need to fallback to use ReadableStream.

[changeset-bot]

🦋 Changeset detected

Latest commit: d2538dc

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Princesseuh]

Is there an upstream issue on Deno for supporting the initial feature? I don't necessarily feel super good about having a hack for it when it's really a missing feature on their end.

[lucacasonato]

@Princesseuh This is not a missing feature in Deno. This is a spec deviation from Node (and now Bun), that has no specified behaviour in the Fetch spec. The feature that Astro uses would not work in browsers for example - because it is a non standard extension of Fetch. Deno is spec compliant in not accepting async iterables as body arguments to Request/Response.

There is discussion about if this behaviour should be removed from Node in lieu of a spec: nodejs/node#49551. This is the issue discussing possible addition of this behaviour to the Fetch spec whatwg/fetch#1291, but it has not had any traction.

[Princesseuh]

@Princesseuh This is not a missing feature in Deno. This is a spec deviation from Node (and now Bun), that has no specified behaviour in the Fetch spec. The feature that Astro uses would not work in browsers for example - because it is a non standard extension of Fetch. Deno is spec compliant in not accepting async iterables as body arguments to Request/Response.

There is discussion about if this behaviour should be removed from Node in lieu of a spec: nodejs/node#49551. This is the issue discussing possible addition of this behaviour to the Fetch spec whatwg/fetch#1291, but it has not had any traction.

Oh, interesting. Apologies, I didn't know the context, figured that since it worked in Node and Bun added it for compatibility with it, it was a standard thing, should've known better.

Is there anything Astro could do to better respect the spec and at the same time not regress to the previous state? We added that code for performance reasons previously, as ReadableStream is slow in Node. cc @matthewp

[lucacasonato]

@Princesseuh No worries at all - I suggest you feature test for this specific behaviour, like so: #10495 (comment). You are then shielded from Node un-shipping this behaviour, and if it is added to the spec in the future, this will also just work in runtimes that support this behaviour. If you want to clean up the code here, you can always create the async iterator, and then pass it to Response directly if the feature check succeeds, and if not pass ReadableStream.from(asyncIterator) instead.

[bluwy]

The reason we use async iterables for node is because we're suggested that it's faster in node (#9614). We're not using it because the platform supports it. Is it faster in Deno too?

Otherwise I think the original plan that checks for Deno globally to avoid it in Deno is more correct, but it is unfortunate that we can't check if we're really really running in node.

[matthewp]

Love that this uses feature detection, thanks! Just need a changeset pnpm changeset, a patch seems appropriate here.

[matthewp]

@bluwy I don't understand, what problem do you see with the feature detection mechanism in this PR?

[bluwy]

I think it isn't the intention we use async iterables in the first place. It's non-standard for node, so we'd only allow to use it there. We wouldn't want to start using async iterables if other runtimes also implement this IMO. The feature detection also has a slightly higher startup cost.

[satyarohith]

@bluwy I'm happy to revert back to Deno specific check if you prefer it.

[bluwy]

I'd wait for Matthew to respond first before doing that 😅 Maybe I'm missing something from his point of view and we could iron that out first.

[lucacasonato]

You could also always wrap the async iterable using ReadableStream.from() before passing it to Response. According to Matteo from the Node project, this has identical performance characteristics: nodejs/node#49551 (comment)

[bluwy]

Thanks for the link, I remember Matteo (on Twitter) suggested using async iterables because it was faster, but if it's not, then maybe we should remove the async iterable code altogether 🤔

About ReadableStream.from(), since we already have a renderToReadableStream method, I think it would be better to use that directly instead. We could keep the async iterable code and use ReadableStream.from(), but we don't have a use for async iterators then.

[lucacasonato]

I'm not saying a ReadableStream with custom "start" and "pull" hooks is identical in performance to the new Response(asyncIterable). I'm saying Matteo suggests that new Response(asyncIterable) is identical in performance to new Response(ReadableStream.from(asyncIterable)). Doing the latter is additionally spec compliant.

[matthewp]

I'd wait for Matthew to respond first before doing that 😅 Maybe I'm missing something from his point of view and we could iron that out first.

No, I understand your point of view. Basically let's not assume that it's always better to use AsyncIterator and be very deliberate about it. +1 from me on that idea.

[lilnasy]

Doing the latter is additionally spec compliant.

It is also exactly what undici does with the async iterable in the Response constructor - it gets turned into a ReadableStream first order of business. I would expect performance on node to be identical.

[bluwy]

In that case, let's stick with the explicit deno check for now then. It looks like ReadableSteam.from is only supported from node 20.6 so we can't quite use it yet.

We could check if the API exists and use it, so renderToAsyncIterable + ReadableStream.from where possible, but I don't think we have to implement it in this PR (but I don't mind if we do).

[satyarohith]

@bluwy I appreciate if you can review the PR. I reverted back to the original Deno check.

[satyarohith]

@bluwy are there any issues blocking this PR? I'm happy to address them. It would be great if we can land this PR :)

[bluwy]

Sorry I've been away last week. LGTM 👍