module: ensure 'node:'-only modules can access node_modules

[cjihrig]

This commit allows require() and import to search the node_modules directories when importing a core module that must have the node: scheme. This prevents these core modules from shadowing userland modules with the same name but no scheme.

[nodejs-github-bot]

Review requested:

• @nodejs/modules

[ljharb]

I'm really not clear on what this does. It's actually incredibly critical that, for example, requiring fs/ loads node_modules/fs, for bundler polyfills, for example.

Every core module can be accessed both with or without node:, correct?

[giltayar]

Isn't the whole point of the node: scheme to ensure that it imports/requires only the core module, and that it does not access node_modules to find a userland module of the same name?

(I may be misunderstanding the node: schema, but that's what it intuitively feels like it should do)

[ljharb]

@giltayar anything that is a core module - fs, say - won't ever search node_modules if you require or import either fs or node:fs. It's fs/ - with the trailing slash - that searches node_modules (only with require), and that capability is critical to preserve.

I'm not aware of any core modules that only can be accessed with the node: prefix.

[Trott]

I'm not aware of any core modules that only can be accessed with the node: prefix.

The test module is the only one. It was just introduced on master (so should be in a nightly build soon) by @cjihrig. 3c4ee52

[ljharb]

Oof, why is it only with the prefix?

[giltayar]

Thanks @ljharb!

So (just a thought) shouldn't node:fs/ be disallowed instead of allowing it to look in node_modules? Or has that train already left the station? 😀

[ljharb]

That already doesn’t work, afaik.

[cjihrig]

For some background, #42325 added a new core module named test. I originally wanted to name it test_runner, as that did not conflict with anything on npm. A number of folks requested that the module only be exposed as node:test.

We currently treat new top level modules as semver major changes because they can conflict with userland modules. There has been talk for a while now of exposing new core modules only under the node: scheme to avoid conflicts.

I didn't really want to be the first one to introduce a node:-only module, but here we are.

Isn't the whole point of the node: scheme to ensure that it imports/requires only the core module, and that it does not access node_modules to find a userland module of the same name?

That's correct. test will load userland modules, but not the core module. node:test will load the core module, but not a userland module.

Every core module can be accessed both with or without node:, correct?

That was previously the case. The idea is to introduce new core modules only under the node: scheme.

Sorry for any confusion this caused.

[targos]

I'm not convinced it's a good idea to have require('node:test') that returns the builtin module and require('test') that returns the one from node_modules. It will certainly cause confusion amongst users, especially because test exists on npm.

[cjihrig]

I certainly wouldn't be opposed to going with my original plan and publishing the module as test_runner. Since #42325 is marked as semver major, we have some time to figure it out.

cc: @bmeck @mcollina @jasnell @martinheidegger from the discussion in #42325. Also @nodejs/tsc for visibility.

[cjihrig]

@targos - if we decide to go back to the test_runner name, I have a branch ready to go here.

[martinheidegger]

Since I was the original person who brought this up...

From the node perspective, the first package only under node: must certainly looks odd. I saw it more under the light of: how can the test library get the a good name? require('node:test') just looks much better to me. It is testing in golang, unittest in python and test in rust (though it uses annotations instead of a package namespace) and https://deno.land/[email protected]/testing in deno, in comparison I found test_running with an _ clucky.

Personally, I have no problem with require('test') resolving a different package. On the contrary: it kinda feels more democratic: Some other person got that namespace, no-problem: Node.js doesn't need to hog that.

A neat side-effect I also found is that it advertises the node: namespace. Right now, if you use node:fs anywhere in the code, other devs in my team will look odd at me. If there is a library that requires node: it would maybe make it a bit easier to me to argue the node: namespace.

At the end of the day, my opinion on all of this is not strong, my goal was just to make the testing framework be the best it could be.

[cjihrig]

test_runner is more clunky than node:test, no doubt. I don't think it's really any worse than child_process,diagnostics_channel, perf_hooks, string_decoder, worker_threads, etc.

I like the idea of using the node: scheme for the reasons outlined in #42430 (comment). I do have two concerns though:

• Additional complexity in the module systems. This impacts maintenance and performance.
• Security. This goes along with the confusion point, mentioned in module: ensure 'node:'-only modules can access node_modules #42430 (comment). I could easily picture Node core creating a module node:foo in the future that only exists under node:, and a malicious package being published to npm with the same name but no node: scheme. In my opinion, this should trump anything related to the niceness of the module name.

[ljharb]

also, fwiw, @Gozala may be willing to donate the test package given that it hasn't been touched in 9 years. That seems like a better approach than creating a discontinuity in the landscape of core modules.

[cjihrig]

Another very minor point - using node:test or test makes writing commit messages a bit awkward since we use test for changes to the core test suite. I'm sure we could work around that, and if we get the chance to use the bare test name, we should use it, but it is another thing to consider.

[targos]

In my opinion, this should trump anything related to the niceness of the module name.

Agreed. I have seen many times people who npm install some_core_module by mistake.

[bmeck]

Agreed. I have seen many times people who npm install some_core_module by mistake.

I don't see how naming with and without the prefix is actually going to help in the vast majority of real world situations with this point.

1. they can run npm install test_runner still
   i. this can run install scripts etc.
2. the API for these modules matching is HIGHLY unlikely
3. tooling that does analysis like code completion / types / etc. will complain
4. it could be linted. probably should be linted

I would understand the point if the goal was to avoid npm install entirely but I don't think naming choices here will prevent people from doing that workflow regardless.

---

I think it would be nice but not direly important to require https://www.npmjs.com/package/fs style reservation when possible. If truly wanting npm to be scoped out you could also add yet another mitigation to make sure it doesn't exist on the registry by using a scope like node:@node/test which is getting rather verbose again just to avoid naming issues.

I am -1 on the process we use to land new modules being blocked by this node: scheme given past arguments we have had around it repeatedly. I think any block needs to start coming with a forwards path that won't block every time this subject comes up. We spent a rather lengthy bit of discussion just to agree on node: as a way forward to avoid these issues and if that is not satisfactory to people, we need some kind of direction that would be satisfactory in the future.

[bnb]

FWIW I also particularly don't like the test_runner namespace. IMO underscores should be avoided if there's any way to do so, but I explicitly didn't comment on the original thread because I didn't feel like there was a good alternative available.

Personally, creating new and unique edge cases that people will have to learn is significantly worse than having an underscore in a name (and I really hate underscores.)

Thinking about this from the perspective of someone who's having to switch to Node.js occasionally for their job from their preferred language/platform or thinking about it from the perspective of someone who's just learning to code, this is a confusing departure for reasons that are beyond anything that will impact them or that they'll care about in the near term. IMO we should want to set them up for success with testing, and introducing an oddity like this explicitly increases the barrier to that.

I am -1 on having this be an edge case and would strongly prefer that we use another module name (if we really have to, test_runner, but it might be worth doing outreach to the owner of test who doesn't seem to actively maintain it).

[bmeck]

@bnb if we use something like @node/test that supports both with and without the node: prefix does that seem fine? It avoids the issue in the future so we don't get increasingly ugly/esoteric names for builtins.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43485/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43488/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43489/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43490/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43493/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/43494/

[nodejs-github-bot]

Landed in 93c4dc5