[WIP] use cloudflare native stream take 2

[vicb]

(ref: #352, #353)

@pi0 what about something like this.

It's missing a http/$cloudflare that would pass Readable and Writable from node:stream - and a few more changes.

Would that work for you?

[pi0]

Thanks for proposing alt PR.

This is also a nice idea to make factory fn and reuse.

One different solution would be we change relative imports to unenv/runtime/node/* and then in Wrangler's ESBuild plugin you can rewrite them to node:* -- or use aliases in preset that should work too! (if it really solves anything - as in runtime functionality that today works*).

I prefer second approach because does not involves increasing internal complexity of unenv and if in future can be used for possible similar situations without making factory for every internal.

---

Also I think would be really worth to investigate Next.js issue closer, if from what I understand one server fn is called from another, it looks really close concept to nitro direct fetch and actually you might want to do something like this.

(*) And the change using previous, this or any other method that avoids unenv's stream polyfill, certainly breaks nitro (we don't want that do we?

[vicb]

(*) And the change using previous, this or any other method that avoids unenv's stream polyfill, certainly breaks nitro (we don't want that do we?

We surely do not want to break nitro (or anything, really).
But I still do not get why, on cloudflare, using the built-in stream would break nitro?

[pi0]

Using a built-in stream is fine of course but if cloudflare preset / wrangler overrides relative paths of node:http it breaks other stuff.

[pi0]

Added test via #361 to make sure we don't break it.

I think doing this change would only make sense as a last resort (accepting it breaks something else) and using unenv/runtime/node/* + alias on preset (or better on framework like OpenNext that needs it).

So let's investigate opennext issue first, it is really not a bug here.

[vicb]

So let's investigate opennext issue first, it is really not a bug here.

Sorry that I am slow to understand, but what open next issue?

using unenv/runtime/node/* + alias on preset

I tried that this morning.
I needed to import { Readable } from "unenv/runtime/node/stream/index" so that it works.

I am looking at it as we speak.
I'll update here once I have more info.

Thanks for your help and adding the test 🙏

[pi0]

(refering to opennextjs/opennextjs-cloudflare#147), if you are investigating it amazing, LMK if could help or wanted me to locally investigate that.

[vicb]

(mostly thinking out loud and still investigating the OpenNext issue)

I think doing this change would only make sense as a last resort (accepting it breaks something else) and using unenv/runtime/node/* + alias on preset (or better on framework like OpenNext that needs it).

I believe that the factory solution in this PR would be work:

• http/$cloudflare would use the builtin node:stream#Readable via node:stream - it fixes the OpenNext issue
• http/index would use the internal Readable
• runtime/fetch imports from http/index so it would work (using the internal Readable

while using the alias would not work on cf because importing unenv/node/runtime/stream would import node:stream#Readable in all cases which breaks runtime/fetch.

I'm still investigating a related issue (present wether or not this PR is applied).

[vicb]

Pooya,

I have finally fixed the root cause of the related error I was seeing in OpenNext so that I was able to finally test properly.

I can confirm that using the builtin workerd stream implementation fixes the error with Open Next.

The way I think about unenv/runtime/node/stream is that it's used for 3 use cases:

1. unenv polyfill runtime/node/http/index.ts
2. fetch runtime/fetch
3. cloudflare hybrid polyfill runtime/node/http/$cloudflare.ts

What we want is to use the unenv stream polyfill for 1 and 2 but the workerd implementation for 3.

For now I can imagine 2 ways to do this:

• using a factory function as drafted in this PR - the price to pay is an added complexity
• copy-editing runtime/node/http/internal to runtime/node/http/$cloudflare-internal and use worked stream there - the price to pay is code duplication

I believe that the alias solution mentioned before would not work as it would affect all of 1, 2, and 3 - and then break the test you added in workerd.

What do you think?

[pi0]

I think either way, if cloudflare preset is applied and changes node:http polyfills's stream dependency, to use node:stream (from workerd), makes it incompatible with fetch util, cloudflare hybrid polyfill applies to the whole build.

I have finally fixed the root cause of the related error I was seeing in OpenNext

Do you have a reference? What was the issue? (I'm need to understand first how opennext depends on node:http and what is real functionality?)

[vicb]

I think either way, if cloudflare preset is applied and changes node:http polyfills's stream dependency, to use node:stream (from workerd), makes it incompatible with fetch util, cloudflare hybrid polyfill applies to the whole build.

I think it will work because fetch uses relative imports but I need to test to make sure.

Do you have a reference? What was the issue?

For the issue I was chasing:

• fix(cloudflare): fix the node wrapper opennextjs/opennextjs-aws#661
• @conico974 actually found the root cause, basically this line should instantiate an Identity transform stream

I hope this helps

[pi0]

I think it will work because fetch uses relative imports but I need to test to make sure.

(BTW unenv/fetch is one instance of usage we added tests for usage of node:http polyfill with actual functionality, happy to write redundant tests that fail by node:http behavior change itself if that helps)

Thanks for the reference, but where is the direct import of node:http exactly in opennextjs? cloudflare-node.ts looks directly importing node:stream which is not relevant to this issue that affects node:http imports.

[vicb]

Yes it would help to know what using workerd would break.

Thanks for the reference, but where is the direct import of node:http exactly in opennextjs? cloudflare-node.ts looks directly importing node:stream which is not relevant to this issue that affects node:http imports.

The PR is about the issue that was preventing me from testing fully.
node:http is used from https://github.com/opennextjs/opennextjs-aws/tree/main/packages/open-next/src/http

[pi0]

Thanks for the ref to opennextjs/src/http that's what was looking for. 👍🏼

It looks like opennextjs reinvented what we did with unenv/nitro for Node HTTP emulation but differently directly in there (and I'm interested in exploring how to unify that).

Exposing an internal factory that can be used differently is not an ideal way for me because even if it fixes current opennextjs HTTP compatibility, it means any internal changes to the unenv IncominMessage/ServerResponse can possibly break something and put even more constraints on the future development.

My suggestion as follow-up is:

• Find a quick/temp hotfix for opennextjs issue
• Make unenv/node/http and unenv/fetch compatible with node:stream (node/workerd/polyfill*)

(*) as part, if needed we can even entirely replace current stream polyfill from nstdlib)

[vicb]

The code in Open Next comes from https://github.com/dougmoscrop/serverless-http

(*) as part, if needed we can even entirely replace current stream polyfill from nstdlib)

Do you mean that fecth/polyfill would use that while cf would use the native impl?

Find a quick/temp hotfix for opennextjs issue

Would duplicating /internal to $cloudflare-internal would be a reasonable quick fix.
I'd rather have something quick to:

1. solve the current issue with open next
2. then work on the preset migration
3. then back on a long term fix

Let me know what you think and thanks for the insights !

[pi0]

Making node:http polyfill compatible with native node:stream API shouldn't be that hard and I very much prefer it, for the sake of everyone's benefit depending on unenv 🙏🏼

I will explain the extent of my concerns (of doing wrapper/dup) in our next call and will try to find some more free time to investigate this story and possibility.

[vicb]

Ok.

What the sake of bundle size, we would prefer using the native implementation but let's discuss of the pros and cons - I'll be out this Thursday, not sure if Igor will join as he's travelling.

Do you have some idea for a quick/temp fix for now?

[pi0]

As for a quick fix, same as my initial comment #355 (comment), using unenv/runtime/node/* instead of relative imports, both cloudflare preset and opennext can opt into it then, if that works for you, PR is more than welcome 👍🏼

That change will explicitly break our new test as well as nitro as a clear indicator that cloudflare overrides changes node:http behavior.

Using the wrapper only won't break current unenv/fetch tests but it breaks imports of node:http that rely on the current polyfill functionality, not a sensible option.

If you like to help, adding support for native node stream to current codebase of HTTP polyfill is not that hard thing and while makes little behavior changes in nitro, it is for sake of everyone (reducing bundle / benefit hybrid) and will be consistent between nitro/opennext.

[vicb]

As for a quick fix, same as my initial comment #355 (comment), using unenv/runtime/node/* instead of relative imports, both cloudflare preset and opennext can opt into it then, if that works for you, PR is more than welcome 👍🏼

I tried that (see this comment above) - I had to import from unenv/runtime/node/stream/index.

Would that be ok if I submit this in a PR, considering that "That change will explicitly break our new test as well as nitro" - I guess maybe because Nitro is still on 1.x

If you like to help, adding support for native node stream to current codebase of HTTP polyfill is not that hard thing and while makes little behavior changes in nitro, it is for sake of everyone (reducing bundle / benefit hybrid) and will be consistent between nitro/opennext.

Yep, I'm willing to help. I only need a few days to move the preset and then I'll be back on this. At that point we should have a VC to discuss a plan/ideas.

[pi0]

Surely! It is acceptable that we explicitly fail it with alias and as hotfix.

As soon as external preset is ready, I plan to migrate current semver-major version of nitro 2.x to unenv v2 (non breaking migration) and we already need to have another override on top of cloudflare preset as there are other issues such as process.env support, having it with alias, makes that at least easier and cleaner.

Also there is matter of having a way to opt-out from wrangler build, without it even current node_compat_v2 usage is broken :(

[vicb]

Also there is matter of having a way to opt-out from wrangler build, without it even current node_compat_v2 usage is broken :(

I can't remember what we said, would --no-bundle work for you here or do you need anything else ?

[pi0]

Yes only we need to automate it so nitro works with unenv v2 out of the box..

[vicb]

This could be part of the deployment_config=<path/to/wrangler.toml> or whatever solution we come up with

[pi0]

yep

[vicb]

Closing in favor of #363 as discussed.