-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Prefix node
built-in module imports with node:
.
#18235
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
base: main
Are you sure you want to change the base?
Conversation
This is in anticipation of emscripten-core/emscripten#18235
Responding to #17915 (comment) by @RReverser
Given the link above documenting that this is supported in I regularly work with code that is difficult or impossible to polyfill in older versions of If this is not quite appropriate to do by default at this time, would a flag be appropriate to emit such prefixed imports? |
Yeah I was basing my assumption off articles like this - https://fusebit.io/blog/node-18-prefix-only-modules/ - and my own experiencing seeing What about my other part - does it work as expected with bundlers e.g. webpack / rollup? |
As a general point, sure, I get that might be the requirement for a specific project, just saying that at the tooling level backward compat is often more important as tooling like Emscripten needs to support a lot of projects in the wild. But then again, since this feature is supported all the way back to Node 12, this point is moot here :) |
Yeah, I understand, supporting both modern and older targets can certainly be very time-consuming, especially when For some context, I've spent a lot of time recently trying to get a C++ project to build to a usable WASM output using a variety of WASM tools. My first impression of Emscripten's emitted wrapper file was that it had a lot of outdated assumptions and bulky workaround code that we would never need, and that it would be a pain to get working in This PR is my attempt to address the main remaining artifact of the current code that affects my day-to-day work, but it's definitely a much smaller detail! It would definitely be nice to ask |
Yeah, as I said, personally I think it's reasonable given that my understanding of this being a bleeding edge feature was wrong, just want to double-check
|
This is not quite true, the Details$ nvm install 12
$ nvm use 12
Now using node v12.22.12 (npm v6.14.16)
$ node -v
v12.22.12
$ node -e 'require("node:fs").writeSync(1, "Hello world!\n");'
internal/modules/cjs/loader.js:818
throw err;
^
Error: Cannot find module 'node:fs'
$ node -e 'require("fs").writeSync(1, "Hello world!\n");'
Hello world!
$ nvm install 14.17
$ nvm use 14.17
Now using node v14.17.6 (npm v6.14.15)
$ node -v
v14.17.6
$ node -e 'require("node:fs").writeSync(1, "Hello world!\n");'
internal/modules/cjs/loader.js:892
throw err;
^
Error: Cannot find module 'node:fs'
$ node -e 'require("fs").writeSync(1, "Hello world!\n");'
Hello world!
$ nvm install 14.18.0
$ nvm use 14.18.0
Now using node v14.18.0 (npm v6.14.15)
$ node -v
v14.18.0
$ node -e 'require("node:fs").writeSync(1, "Hello world!\n");'
Hello world! We should probally add a Adding such an option would also mean that we could set Lines 409 to 415 in 7307f41
(i.e., the above could be removed when doing -sMIN_NODEJS_VERSION=16.0 )
|
Hmm, I was going off the official docs, that's a little concerning. 😬
I don't use Webpack or Rollup anymore, so unfortunately I can't help answer that. 😕
I'd be very happy with that! 😃 Many projects specify an |
Adding |
As someone who works hard to write portable code, I would like to strongly argue against this. It's hard enough providing one universally compatible build with a slimmed-down set of workarounds, and I cannot afford to maintain multiple builds to keep the code size down. In addition, it can often be a bad idea to leave unused code around for a long time, as bundlers or browsers will start having issues with code that isn't ever actually going to run, which can waste a whole bunch of debugging time. 😕 |
@lgarron Sounds reasonable. Ok, if the general preference is |
poke 🤓 |
Given that emsdk uses node v14.18.2 I'd be in favor of simply declaring that we don't support node versions older than that. I'm also find with landing this change as-is without blocking on adding the new |
Do we have a sense of whether users are running the output on older versions? I suggest we at least ask on the mailing list first. If there are no concerns there then I'm also ok with landing this as-is, maybe with docs that mention the minimum node required. |
d088ead
to
a94bcac
Compare
I think this would still be great to land. I've rebased and updated the PR. (Note: There are a still bunch of calls in test code that I haven't updated, in order to keep the PR as simple as possible.) |
a94bcac
to
83e0fa4
Compare
This makes the imports diistinguishable from the corresponding `npm` packages, which can help with: - Development ergonomics: - Marking all `node:*` packages as external in a bundler. - Tracking the dependency graph - Supply chain security (avoiding risks from name squatting). The `node:` protocol prefix is supported in LTS versions all the way back to `node` 12: https://nodejs.org/api/esm.html#node-imports
83e0fa4
to
f7e35ae
Compare
This will regress code slightly for all targets, right? Given that, I wonder if it is worth it? Regarding npm, are there really packages that conflict in their naming with the set of modules that we |
The code will have exactly the same functionality. As discussed above, this is supported in
Yes. I have had some gnarly times fighting very bad assumptions in the ecosystem about the availability and functionality of the
They're not so much confused as they have to make assumptions that cannot be correct under all circumstances. There is literally no way for them to know which import is actually meant, whereas a
Yes. It can be difficult or impossible ship code that works properly in all bundlers when the
All of this adds up to a few footguns for "normal" projects. But more importantly, if you're a library author publishing code with WASM, you have to deal with any semantic issues across all runtimes and bundlers used by people using your library. If it's any indication of how much a difference this makes to me as a library author, I use scripts to always rewrite Emscripten output to add the |
I mean it will regress code size slightly. Each program we generate will get a few bytes bigger.
Yes but is that try for any of the module that we use here in emscripten?
That does sound like it can be quite bad. Can you point to bundler where this true, or an example with will fail with the current emscripten output? We have tests that run under rollup, vite and webpack: emscripten/test/test_browser.py Lines 5492 to 5522 in 8e65a04
|
Ah, I see. From my impression, Emscripten already includes a lot of code to handle various environments and edge cases, and the number of bytes added would be rather mild. (If anything, the presence of a
Certainly: https://www.npmjs.com/package/ws
I personally use // main.ts
import { fileURLToPath } from "url";
import { readFile } from "fs/promises";
const path = fileURLToPath(new URL("./package.json", import.meta.url));
console.log(JSON.parse(await readFile(path, "utf-8"))); package.json
{
"type": "module",
"dependencies": {
"@types/node": "^22.13.1",
"esbuild": "^0.25.0"
}
} Run:
This returns:
I avoid
This works quite well (assuming, of course, that all
Happy to give it a go, what would it test? That the source code includes strings like |
Just to confirm, you are trying to run the resulting bundled code on node? I assume so. Otherwise you can remove all the node imports (and code overhead) from the emscripten output using |
You are correct the code size changes should minimal, and I'm tempted to agree with you say we should accept them in this case. But we have to very vigilant in emscripten because historically our code size has suffered from the death-by-a-thousand-cuts.
Are you saying that node has a builtin
So the errors only show up for you when you are targeting node but don't want to use the |
I suppose that's fair! What's the best way to measure this for a PR?
Indeed!
Correct. I personally don't consider it contrived because its very much an everyday reality for me, since I generally publish libraries that work in
The
I personally do not use bundlers other than But hopefully I've made a clear case why bundlers cannot make a safe choice.
In particular, note that a non-trivial codebase may include code (either in-tree or in a dependencies) that:
If such code is in dependencies, it can be very impractical to edit the call sites. (Vendoring can be expensive to maintain, sometimes requires bespoke package name mapping, and can lead to security issues more easily. Patching is supported by some tools, but I think not very common in the JS ecosystem — and can also be tricky to maintain.) It's much more straightforward to aim for a world where:
I understand this to be a current best practice. In fact, I would break CI in a lot of my projects if I tried to use |
Perhaps I am misunderstand something here? I cannot seem to find any information on a |
To measure the code size impact of a PR you first need to make sure you have the latest emsdk binaries installed ( |
One other blocker we might have this change it would require bumping the minimum version of node that we support. By default we target node v16 but users can opt into support for node versions going back as far as v10.19. See Lines 1928 to 1934 in f3b1194
It likely possible that we can bump this to v12, but we should perhaps open a separate issue for that. |
This is the minimum node version that we support targeting. Bumping to v12.20 specifically allows us to start using `node:xx` for imports. See emscripten-core#18235
This is the minimum node version that we support targeting. Bumping to v12.20 specifically allows us to start using `node:xx` for imports. See emscripten-core#18235 Fixes: emscripten-core#23652
This is the minimum node version that we support targeting. Bumping to v12.20 specifically allows us to start using `node:xx` for imports. See emscripten-core#18235 Fixes: emscripten-core#23652
This is the minimum node version that we support targeting. Bumping to v12.20 specifically allows us to start using `node:xx` for imports. See emscripten-core#18235 Fixes: emscripten-core#23652
Looks like this would actually require node v16 to be sure it is supported: #23663 (comment) |
This makes the imports distinguishable from the corresponding
npm
packages, which can help with:node:*
packages as external in a bundler (or even better, enabling them to do so automatically).The
node:
protocol prefix is supported in LTS versions all the way back tonode
12: https://nodejs.org/api/esm.html#node-imports