workerd
workerd copied to clipboard
cloudflare
Node.js compat — Request body/Response can not be initialized with a Readable
Repro:
import { Readable } from 'node:stream';
export default {
async fetch() {
const rNode = new Request('https://example.com', {
method: 'POST',
body: Readable.from([...'node']),
});
for await (const l of rNode.body!) {
console.log(l);
}
const rWeb = new Request('https://example.com', {
method: 'POST',
body: ReadableStream.from([...'web']),
});
for await (const l of rWeb.body!) {
console.log(l);
}
return new Response('done');
},
};
Logs:
Uint8Array(15) [
91, 111, 98, 106, 101,
99, 116, 32, 79, 98,
106, 101, 99, 116, 93
] // "[object Object]"
w
e
b
Node (/undici) have extra support for AsyncIterable<Uint8Array>, Iterable<Uint8Array>, and null for BodyInit
Response can also be initialized with a BodyInit, i.e. return new Response(bodyInit); so we should also update the response in the same way
Just a comment to say I think we should only do this in node compat mode. That's all we need it for – and that reduces the surface area for issues – especially if ppl start relying on it in non-Node contexts
Also, while we're at it – should we throw for any non-supported type passed as the body? Seems we're calling .toString on it if we don't recognise it (would be my guess?). Not sure if that's the spec – if not, might help people track down issues a bit easier. Would need a compat flag though in case anyone was relying on that.
Keep in mind that supporting AsyncIterable<Uint8Array> and Iterable<Uint8Array> as the body are non-standard extensions that I really wish undici hadn't implemented unilaterally but they do make sense. Given that they are non-standard, I don't think we should support these yet, even in Node.js compat mode but should instead first go to WHATWG with a proposal to add AsyncIterable and Iterable to the spec.
... while we're at it -- should we throw for any non-supported type passed as the body? Seems we're calling .toString on it if we don't recognise
Coercing to a string is required by the spec. We could potentially warn folks but our current behavior here matches the standard and other runtimes.
Can you explain “even in Node.js compat mode”? That’s all the ask is here.
Sent from Gmail Mobile
On Thu, 3 Oct 2024 at 2:25 AM, James M Snell @.***> wrote:
Keep in mind that supporting AsyncIterable<Uint8Array> and Iterable<Uint8Array> as the body are non-standard extensions that I really wish undici hadn't implemented unilaterally but they do make sense. Given that they are non-standard, I don't think we should support these yet, even in Node.js compat mode but should instead first go to WHATWG with a proposal to add AsyncIterable and Iterable to the spec.
... while we're at it -- should we throw for any non-supported type passed as the body? Seems we're calling .toString on it if we don't recognise
Coercing to a string is required by the spec. We could potentially warn folks but our current behavior here matches the standard and other runtimes.
— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/2746#issuecomment-2389096326, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACZ2QG7OUDW7EOLULEOZDLZZQNBDAVCNFSM6AAAAABOPFQ6BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBZGA4TMMZSGY . You are receiving this because you commented.Message ID: @.***>
Request and Response are web platform standard APIs. Even in Node.js extending the behavior of web standard APIs in non-standard ways is strongly frowned upon and typically we wouldn't have accepted this kind of variance but it just kind of snuck in. Given that adding this for nodejs_compat would introduce a non-standard variance in behavior of an otherwise mostly standard API, not to mention complicating the implementation of the API a bit, I'd prefer to hold off and see if we can get it added to the standard first.
What’s the recommended route to take in the meantime?
Without this we need some way of implementing this behaviour – otherwise certain packages will just not run. Is the suggestion to monkey-patch Request/Response? Do we have a nice/recommended way of doing that?
On 3 Oct 2024, at 7:46 AM, James M Snell @.***> wrote:
Request and Response are web platform standard APIs. Even in Node.js extending the behavior of web standard APIs in non-standard ways is strongly frowned upon and typically we wouldn't have accepted this kind of variance but it just kind of snuck in. Given that adding this for nodejs_compat would introduce a non-standard variance in behavior of an otherwise mostly standard API, not to mention complicating the implementation of the API a bit, I'd prefer to hold off and see if we can get it added to the standard first.
— Reply to this email directly, view it on GitHub https://github.com/cloudflare/workerd/issues/2746#issuecomment-2389748249, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACZ2QGULW4PM2VGX4PAHELZZRSSFAVCNFSM6AAAAABOPFQ6BSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOBZG42DQMRUHE. You are receiving this because you commented.
not to mention complicating the implementation of the API a bit
Can't we re-use ReadableStream.from implementation for this as it accepts an (Async)Iterable and Request/Response already have support for ReadableStream.
Do we have a nice/recommended way of doing that?
That's something we could bake into unenv. But then users opting out of unenv would see a different behavior
... Can't we re-use
ReadableStream.fromimplementation for this ...
Yes, absolutely. Instead of passing the Node.js stream.Readable directly there are two choices that are valid per the spec:
- Using
ReadableStream.from(streamReadable), leveraging the fact that Node.jsstream.Readableis an async iterator, or - Using
streamReadable.toWeb(), leveraging the Node.js adapter API
I would likely choose the first option as it carries the least overhead.
@jasnell is this something you can take on? I agree that this should be gated behind nodejs_compat flag. The first option sounds good to me, but I don't have strong opinions here except for that without any compat flags we should stick to standards.
From @pi0, this seems relevant: https://github.com/denoland/deno/issues/24849
Not quite sure what you're asking for @IgorMinar ... the runtime already supports using either the ReadableStream.from(...) or nodeReadable.toWeb(...) options. Neither are great because they both carry a fair amount of additional overhead. There's already a discussion happening in WHATWG around updating the spec to allow async iterables as the body. Assuming that is accepted then we can absolutely update our implementation of Request and Response to match.
I think the question is:
Does the runtime team plan to implement AsyncIterable<Uint8Array>, Iterable<Uint8Array> and if yes when.
Regardless of WHATWG this is implemented in Node and must be supported in nodejs_compat mode.
If there is no concrete plan, it can be done in unenv but we need to take a decision to avoid duplicating the effort here.
Thanks!
Does the runtime team plan to implement AsyncIterable<Uint8Array>, Iterable<Uint8Array> and if yes when.
If it is added to the standard, yes. When it is added. I would prefer not to implement it prior to then.
If it is added to the standard, yes. When it is added. I would prefer not to implement it prior to then.
That's quite clear - we should implement this in unenv.
One last question for my understanding: let's imagine AsyncIterable<Uint8Array>, Iterable<Uint8Array> never make their way to WHATWG (which would surprise me because it's a sensible API), I think it's pretty safe to say that those will never be dropped from Node.js at is would break BC. So we will end-up with nodejs_compat never being really Node.js compatible?
There will always be limitations to workerd being really Node.js compatible which is why we've gone with the hybrid polyfill model... favoring the use of polyfills to fill in gaps where necessary.