-
-
Notifications
You must be signed in to change notification settings - Fork 16.9k
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
refactor: prefix built-in node module imports #6236
base: master
Are you sure you want to change the base?
Conversation
c3329e4
to
3394d38
Compare
Since v5 relies on node >= 18, this is now possible (since v16, v14.18.0 [^1][^2]). It's functionally irrelevant: 1. It's not required for CJS nor ESM (with a few exceptions [^3]) 2. It has no performance promises However, there are upsides to this approach: 1. It brings clear boundaries to what's a built-in and what's an external dependency 2. It reduces the risk of importing unwanted deps where a built-in is expected 3. It's slightly more interoperable with other JS runtimes that provide node compatibility[^4], albeit only during development. Once imported from npm, built-ins are assumed. [^1]:https://nodejs.org/docs/latest-v22.x/api/modules.html#built-in-modules [^2]:nodejs/node#37246 [^3]:https://nodejs.org/api/modules.html#built-in-modules-with-mandatory-node-prefix [^4]:https://docs.deno.com/runtime/fundamentals/node/#using-node's-built-in-modules
3394d38
to
21d1bb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @slagiewka! I like the idea, let's see what other @expressjs/express-tc members things about it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I appreciate the effort, this isn't necessary. It's only required for newly added built-in modules that conflict with existing ones.
Node.js already prioritizes built-in modules (like http
, path
, and fs
) over user-land modules or files with the same name.
Since v5 relies on node >= 18, this is now possible (since v16, v14.18.0 12).
It's functionally irrelevant:
However, there are upsides to this approach:
Footnotes
https://nodejs.org/docs/latest-v22.x/api/modules.html#built-in-modules ↩
https://github.com/nodejs/node/pull/37246 ↩
https://nodejs.org/api/modules.html#built-in-modules-with-mandatory-node-prefix ↩
https://docs.deno.com/runtime/fundamentals/node/#using-node's-built-in-modules ↩