cloudflare: importing node:http wrongly import the unenv stream polyfill

[vicb]

Environment

7a9f230
Nodejs 22.11.0

Reproduction

See https://github.com/unjs/unenv/pull/352/files#diff-2f5c73584260f31cbdff9f65cf07fd8f619f8b48ddda9d87e1e99d583ac0f87c

Describe the bug

Without the PR, importing node:http also imports the unenv polyfill for stream because it is imported as a relative path.

Because the Readable polyfill does not implement the async iterator the test would throw [unenv] Readable.asyncIterator is not implemented yet!.

For cloudflare, the builtin stream implementation should be used instead.

Additional context

See the comments in the tentative PR

Logs

No response

[vicb]

I am happy to help with this but I would need more info:

This breaks the direct fetch / http call mechanism of nitro, h3, and unenv itself in the Node.js environment (see this for ref), the reason is that Node.js polyfills of node:http in unenv depend on other node: polyfills (such as node:net) which are compatible with each other changed in this PR.

I am not sure to understand the problem here.
Is there some code/tests I could run to figure out?

I think we need to find a different way to solve worked situation (simplest way I would imagine is to add any missing API to polyfills) and if you can kindly please open an issue with reproduction first I would be happy to help but so sorry this is really not a possible solution.

We don't want to use the stream polyfill as it's builtin workerd.
What we want is that the http polyfill pulls in the builtin impl.
One solution could be to have a $cloudflare-internal containing the code from the PR - internal would be left unchanged and used outside of cloudflare.

(BTW generally we migrated and should change all other imports to node:, I noticed few node:events were relative we should fix only http/net/streams are that currently are functional with each other)

I changed the relative imports to event in the tentative, are you saying I shouldn't have?
The rationale is the same: worked has a builtin implementation of events and we don't want to include the polyfill transitively.

[pi0]

I think there are multiple topics here, the fact that unenv http polyfill depends on stream polyfills is not a bug, it is by design and how it works (stream polyfill actually buffers stream).

I understand that workerd runtime also implements node:stream now but built-in unenv polyfills cannot depend on it as it breaks node:http functionality of unenv that nitro, h3 and more places depend on (not for cloudflare or node compat purpose).

Other imports such as node:events are fine and if there is any leftover place that depends on them we can refactor, PR welcome 👍🏼 (this won't break anything and unrelated)

Meanwhile cloudflare preset is being extracted it is really hard to tell but I guess in wrangler esbuild plugin, path rewrites can specifically happen also.

Because the Readable polyfill does not implement the async iterator the test would throw

Can't we implement it as a quick fix?

[pi0]

Is there some code/tests I could run to figure out?

this example is under production use for all nuxt, nitro, and h3 when users call fetch('/url') in their server handlers and emulates network request. It is an actual functionality not a stub in unenv which we cannot break (if we use node:stream, polyfill breaks, because actual node does not buffers). here, and here are also other refs you can read if you like to know what is the usecase.

[vicb]

I understand that workerd runtime also implements node:stream now but built-in unenv polyfills cannot depend on it as it breaks node:http functionality of unenv that nitro, h3 and more places depend on (not for cloudflare or node compat purpose).

So IIUC the only problem is with the import of stream in http.
Would it be possible to add a test to unenv to make sure we don't break things?

Other imports such as node:events are fine and if there is any leftover place that depends on them we can refactor, PR welcome 👍🏼 (this won't break anything and unrelated)

I'll submit a PR.
I'll also add comments on the relative stream import in http so that we don't break things (hopefully we can add a test).

[pi0]

I love to add tests (and if you like to help) as soon as we finish cloudflare preset externalization.

There are tests in h3 and nitro that immediately break and it wasn't a concern before because unenv was made for nitro as a polyfill-only lib.

[vicb]

as soon as we finish cloudflare preset externalization.

Is there a dependency here or is it only because this is the logical order of things.

I have worked on the testing story today, not there yet but hopefully early next week.

[pi0]

Having the external preset, we can also properly test the compatibility (and see what needs to go where)

Do you know how/when opennextjs/opennextjs-cloudflare#147 had been introduced? Does calling server actions from client components involve using node:http in Next.js?

[vicb]

Do you know how/when opennextjs/opennextjs-cloudflare#147 had been introduced?

No, I'll investigate next week to figure out the root cause.
I'll update the issue with my findings.