-
Notifications
You must be signed in to change notification settings - Fork 22
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
Use process.getBuiltinModule
for hybrid node modules
#294
Comments
I really like the desire not to rely on cloudflare/workerd#2097 and instead create the hybrid polyfills on the fly, but what I'm concerned about and I don't quite understand is the claim that disabling tree-shaking doesn't matter much, or at least that's how I read "Unenv fallbacks are always bundled (although can be lazy evaluated and are small)" @pi0 can you please help me understand how this would not matter? some of the unenv polyfills are not small (e.g. node streams, url, etc). So how can we ignore the significant size of these? |
You are right @IgorMinar we will bundle polyfills that's the inevitable cost if we do this for other benefits (still bundlers can tree-shake per export - not per module that was my side-point). IIRC the initiatives behind the idea were mentioned when we were talking about the possibility of shipping unenv dist via workers module registry directly instead of bundler. Out of curiosity, I did a POC to see how much is the cost of entire unenv node polyfills bundled. It is |
some in progress notes... still wip Pros:
Cons:
|
If workerd have a known issue, unenv polyfills can explicitly avoid using it (per export) and only use polyfill. (unenv has control per build/deployment) If workerd has a problematic native impl for any reason or drops a feature in favor of polyfill (like #302 i guess?) or when it enables it back, this approach actually self-adopts without need to rebuild/deploy. (workerd has control at runtime)
You are right about the cost of fallback check. But it can be as low as a one time nullish check per export or symbol|prop i guess right? ( |
@pi0 the notes above were from my brainstorming session with @petebacondarwin which we had to wrap up quickly so I just posted the WIP notes we captured before we finished. I see a lot of value in this but I'm also a bit uneasy because of the potential cost of this approach, and hard to predict future interactions between workerd and unenv, which evolve and are updated differently:
In an ideal world, the version skew issue should not be a problem, it's just very hard to predict all of the possible interactions and corner-cases given the magnitude of the surface area, and the fact that workerd, node.js, and unenv are continuously changing and evolving. So maybe it makes sense to assume the ideal world conditions and consider version skew not to be a problem, and explore if performance cost would rule out this approach even before we need to worry about version skew. And performance comes to two things:
Both of these could be explored via experimentation in real world conditions, and we could help explore these some time over the next few weeks (just not in September). Thoughts? |
Thanks for sharing your thoughts @IgorMinar. Let's stay with the current approach (unenv-bundled-by-usage) for wrangler tooling, considering the current system works for you and the announced date is close, we can keep manual sync between workerd<>unenv<>wrangler versions 👍🏼 Considering past experiences of build-time-only polyfills we had with Nitro/Nuxt , i am really positive that increased [runtime] coverage and stability of this alternative approach worth experimenting. I will put a pending label on this issue until we setup a workerd testing system in unenv repo so we can properly measure any possible overhead and test functionality in actual runtime directly. |
Currently we have
$cloudflare
specific entries that allow hybrid support for runtime compatible features usingprocess.getBuiltinModule(id)
. (example:node:buffer
)Since
process.getBuiltinModule
is a Node.js standard, we might instead just merge entries. We can still treeshake (each export prefers built-in module if available and fallback to polyfill).Cons:
Pros:
The text was updated successfully, but these errors were encountered: