Behavior of module.isBuiltin and module.builtinModules

[vicb]

We have discussed with @jasnell and @pi0 about what the behavior of module.isBuiltin and module.builtinModules should be.

Different runtimes have different behaviors.

People have different opinions:

• The behavior should match Node
• The behavior should avoid breaking dependencies
• Whatever the behavior, it should be documented
• ...

Probably the best way to move forward would be to rely on a standard.
As such the different runtimes could rely on that standard and the library could also rely on that standard.

Maybe WinterCG could help here?

Thoughts?

/cc @IgorMinar

Ref:

• Implement node:module createRequire API #2636
• https://nodejs.org/docs/latest/api/modules.html
• https://github.com/unjs/unenv/blob/70db6f157dde54d1b11f4fa77cee630fb255be49/src/runtime/node/module/index.ts

[jasnell]

This falls outside of the scope of wintercg as this is a node specific API. Node.js is the standard on this.

[vicb]

This falls outside of the scope of wintercg as this is a node specific API. Node.js is the standard on this.

The Node doc says "process.getBuiltinModule(id) accepts built-in module IDs that are recognized by module.isBuiltin(id)".

So my understanding is that if Node is the standard module.isBuiltin(id) should return false when process.getBuiltinModule(id) is undefined.

Catch22.

[pi0]

Thanks for opening the public discussion dear @vicb.

Currently, the list of 68 built-ins in unenv is exactly identical to the one included in #2636 so at least workerd, Node.js, and unenv are consistent with each other.

I do agree that also other runtimes (Deno/Bun) should not be used as a reference and they might be simply lagging behind from the latest Node.js behavior (or have some very specific reasons to not enlist some builtins)

(...about unenv...)

In unenv, we do provide polyfills that tend to mimic the closest-possible Node.js runtime behavior regardless of runtime that the code is bundled for either Bun, Deno, Node.js, or workerd (with node_compat flag enabled or not)

We adjust the behavior of unenv polyfills always based on real-world scenarios (known dependencies that are made for Node.js and end up in the user's bundle should work as intended).

The reason I have asked to keep the explicit list in unenv for module/$cloudflare entry is that it keeps working even when the Node.js compatibility layer of workerd is disabled as before and allows us to move toward a vendor-agnostic adoption of process.getBuiltinModule hopefully getting other runtimes on board too. If you still strongly believe in Wrangler usage we should avoid that list which is identical, we can do that too (unjs/unenv#262 is proposed to make sure Wrangler's tooling can decide fully on its own) but only it can add to the maintenance and possible ecosystem-wide inconsistencies IMO.

The Node doc says "process.getBuiltinModule(id) accepts built-in module IDs that are recognized by module.isBuiltin(id)".
So my understanding is that if Node is the standard module.isBuiltin(id) should return false when process.getBuiltinModule(id) is undefined.

This is a valid point 👍🏼 but also as you also pointed out @vicb in our private discussion, one thing can break another. Today consistent module.isBuiltin behavior (between actual Node.js and compatible runtime) is much more likely needed than of process.getBuiltinModule co-behavior.

(...one idea as middle ground...)

I believe current #2636 behavior is best for best compatibility but as another middle ground, workerd might (same as other new hybrid natives like url) partially only list the built-in modules it is implemented so runtime (process.getBuiltinModule in workerd) matches runtime (process.getBuiltinModule) and build-time (unenv) matches build-time (bundled logic for legacy deps) what do you think @jasnell @vicb?

[jasnell]

Honestly, it's not exactly clear what the "right" solution is or even if there is one. I think that maybe, however, given the questions here, #2636 should probably just drop the isBuiltin(...) and builtinModules and parts and just provide the createRequire(...). We can address the other APIs separately once we have a better idea of what we might want.

[pi0]

Makes sense. I think we can also infer isBuiltIn: false from getBuiltinModule: undefined if needed in any userland requirement.

[vicb]

Honestly, it's not exactly clear what the "right" solution is or even if there is one.

Agreed.
That's what I think for this particular API and the case will probably repeat for other APIs.
Which is why I think a standard would help.

I think that maybe, however, given the questions here, #2636 should probably just drop the isBuiltin(...) and builtinModules and parts and just provide the createRequire(...). We can address the other APIs separately once we have a better idea of what we might want.

Agreed.
+1 to drop isBuiltin(...) and builtinModules for now (= revert those changes now that the PR has landed)
I think we have a principle of not adding anything in workerd that can be done in userland and both can be done in userland.
Using the unenv implementation for those would permit to change the implementation when we have clear idea on how those should behave.

[vicb]

I think we have a principle of not adding anything in workerd that can be done in userland and both can be done in userland.
Using the unenv implementation for those would permit to change the implementation when we have clear idea on how those should behave.

To make it clear: I was the one asking @jasnell to add builtinModules to the PR. With retrospect I was wrong. This can be be easily added in userland. The discussions we had helped me realize that. Thanks.