`allowed` modules in `no-nodejs-modules` should match the base name of the imported module

joebeachjoebeach commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

jason-ha commented 8 months ago

Might another solution here be to check the resolution of the import beyond just checking against a list of names? My project uses an import of "events" package to supply node like EventEmitter implementation. We very much want to prevent node uses, but the rule is currently confused about the resolution. It thinks an import of "events/" is node built-in when at runtime the "events" package is used.

Repro setup:

package.json:

    "dependencies": {
        "@types/events": "^3.0.0",
        "events": "^3.1.0"
    }

source code:

import { EventEmitter } from "events/";

ljharb commented 8 months ago

@jason-ha that's a bug; events/ is NOT the core module. Since you're using TS, and presumably the typescript eslint import resolver, can you file it on that package?

jason-ha commented 8 months ago

Thanks @ljharb, I had traced the issue source into eslint-plugin-import. The ESM code version (same as typescript) was expected to produce the same results. However, I can no longer reproduce the situation for TypeScript source or straight JavaScript. I can only guess that my system was in a funny state. When I debugged it (I think) the resolve from eslint-module-utils was not providing a full path. Now it always does. So, nothing to raise anywhere. I'll say something if I see it again.

import-js / eslint-plugin-import