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

Node.js compat — Request body/Response can not be initialized with a Readable #2746

Open
vicb opened this issue Sep 19, 2024 · 14 comments
Open
Labels
feature request Request for Workers team to add a feature nodejs compat

Comments

@vicb
Copy link
Contributor

vicb commented Sep 19, 2024

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

@mhart
Copy link
Collaborator

mhart commented Sep 19, 2024

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

@mhart
Copy link
Collaborator

mhart commented Sep 19, 2024

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.

@vicb vicb changed the title 🐛 Bug Report — Request body can not be initialized with a Readable 🐛 Bug Report — Request body/Response can not be initialized with a Readable Sep 30, 2024
@jasnell
Copy link
Member

jasnell commented Oct 2, 2024

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.

@jasnell jasnell changed the title 🐛 Bug Report — Request body/Response can not be initialized with a Readable Node.js compat — Request body/Response can not be initialized with a Readable Oct 2, 2024
@jasnell jasnell added feature request Request for Workers team to add a feature nodejs compat labels Oct 2, 2024
@mhart
Copy link
Collaborator

mhart commented Oct 2, 2024 via email

@jasnell
Copy link
Member

jasnell commented Oct 2, 2024

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.

@mhart
Copy link
Collaborator

mhart commented Oct 2, 2024 via email

@vicb
Copy link
Contributor Author

vicb commented Oct 2, 2024

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

@jasnell
Copy link
Member

jasnell commented Oct 3, 2024

... Can't we re-use ReadableStream.from implementation 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.js stream.Readable is 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.

@IgorMinar
Copy link
Collaborator

@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: denoland/deno#24849

@jasnell
Copy link
Member

jasnell commented Oct 10, 2024

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.

@vicb
Copy link
Contributor Author

vicb commented Oct 10, 2024

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!

@jasnell
Copy link
Member

jasnell commented Oct 11, 2024

Does the runtime team plan to implement AsyncIterable, Iterable 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.

@vicb
Copy link
Contributor Author

vicb commented Oct 11, 2024

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?

@jasnell
Copy link
Member

jasnell commented Oct 11, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature nodejs compat
Projects
None yet
Development

No branches or pull requests

4 participants