Eslint-plugin-import: `allowed` modules in `no-nodejs-modules` should match the base name of the imported module

[joebeachjoebeach]

Background

I work in a monorepo where we don't use relative imports, but rather we have webpack configured to resolve imports to our JS source directory prior to looking in node_modules. For example, given a source directory of src/js, import "my/module" will resolve to src/js/my/module.js.

We have some directories in that JS source directory that match names of Node.js builtin modules. For example, we have a src/js/util/ directory, and in it we have things like src/js/util/debounce.js or src/js/util/escape.js. Doing import debounce from "util/debounce" currently triggers a no-nodejs-modules error, even though util/debounce is not a Node.js builtin (in fact, you can't import any Node util builtin through a subpath like that).

However, adding "util" to the array of allowed modules doesn't silence the error. The only way to silence it is to add an explicit entry for every file under the src/js/util/ directory (and its subdirectories). That's not tenable in a large codebase worked on by hundreds of engineers.

Proposal

I think the most straightforward thing would be to match the allowed behavior to the isBuiltIn behavior. Namely, the rule should expect each item in the allowed list to be a baseModule name.

I suppose the implementation would be:

• export baseModule from src/core/importType.js
• import it in src/rules/no-nodejs-modules.js
• Use allowed.indexOf(baseModule(name)) instead of allowed.indexOf(name) when checking the allowed list in that rule.

Concerns

This would be a breaking change if any users are currently using subpaths in their allowed lists to target only specific submodules. That said, I'm not familiar with any Node builtins where the convention would be to import a submodule with a subpath like util/<some-thing>. I'm not even sure if that's possible with any builtins (but I'm not a 100% authoritative source on the subject).

I'd be happy to open a PR, but I'd rather discuss it before doing that.

[ljharb]

fs/promises is one, and there's a number of others; util/some-thing is almost always going to be a bug, since util is a core module.

In other words, I think the issue here is that your aliasing approach is overlapping with node core module names - i'd strongly suggest a convention like @/util/whatever so it's very clear to both human readers, and static analysis tools, that your import path is something custom.

[joebeachjoebeach]

That's a totally valid point, and I definitely agree that on our end, we should improve our practices to improve the clarity and functionality of our codebase.

That said, your argument seems to apply to the existence of the allow list in the first place. Somebody with a util.js file who has configured import util from "util" to resolve to it can put "util" in the list of allowed modules, and their import will not trigger an error for this rule. That person, as you suggest, would do well to modify their alias convention. However, this plugin already enables them to sidestep that convention.

What I'm suggesting is that the behavior to determine whether a module is "allowed" should match the behavior that determines if a module is a built-in. These two don't quite square with each other (in my opinion):

• Q: "Is this module a built-in?" — A: "Let's only look at its base name. Its base name matches a known built-in, so yes, it is a built-in".
• Q: "Is this module allowed?" — A: "Let's look at its exact name. Its exact name does not match an allowed module, so no, it is not allowed".

I guess to put it simply:
Given that this plugin already provides for sidestepping convention, does it not make sense that import someThing from "util/some-thing" should be allowed given { "rules": { "import/no-nodejs-modules": [ "error", { allow: ["util"] } ] } }?

[ljharb]

I think that no-nodejs-modules should certainly be able to handle your use case. I worry it would break a lot to change the rule to automatically check the base name, but I'd accept a PR that allowed:

{ "rules": { "import/no-nodejs-modules": [ "error", { allow: ["fs", { pattern: "util/*" }, { path: "assert" }] } ] } }

iow, the string form becomes sugar for { path: string }, and { pattern: string } is added (that only supports globbing)?

[joebeachjoebeach]

I think that makes sense; I was worried about introducing a breaking change. I'll put together a PR.