Make no-extraneous-deps lint only dependencies that can be resolved
#943
Conversation
…e resolved, as only resolved paths can be reliably tested if they live in a `node_modules` folder. Up until now, this rule assumed, that every package that lives in a `node_modules` folder or *cannot be resolved* is an external package. This happens because of [that line](https://github.com/benmosher/eslint-plugin-import/blob/master/src/core/importType.js#L32). If project uses aliasing, or transforms imports, this assumption is wrong, as they may not be resolvable by import plugin, but also, they should not be defined in package.json. Changing `isExternalPath` will make any ordering lints unstable (order may change depending if you ran `npm install` or not). Using `import/ignore` setting does not make sense too, as it's used to ignore imports unknown to the plugin, like .css files. But still, you may import a css file from a dependency, and this rule should check that.
panrafal
changed the title
no-extraneous-deps lint dependencies that can be resolvedno-extraneous-deps lint only dependencies that can be resolved
|
I like ignoring unresolved to start. If I understand correctly, that is less of a change than also considering the |
|
BTW: stellar PR! ⭐️⭐️⭐️⭐️⭐️ I really appreciate the detail! |
|
@benmosher yes, that's correct. I've came to that conclusion once I actually did the ignores - that's why I left them, but just ignoring unresolved is more sound here AND simpler, a win-win in my eyes :D |
|
If they can't be resolved, doesn't that mean this plugin can't check across imports to them? If you're going to use aliases or transforms, I want errors until I've provided a resolver to eslint-plugin-import - this seems like it erases useful errors. At the very least, I'd expect it to go under an option and not be on by default. |
|
@ljharb you actually will get errors - for unresolved dependencies - another linter. Here I'm trying to fix the case, where for unresolved dependencies you're told to add them to package.json, which is not correct. |
|
@panrafal ahhh ok so you're saying that this specific rule shouldn't be checking aliases - that makes sense. Should it be ensuring that the thing that the alias resolves to, though, is either a local file or in package.json? |
|
@ljharb if you've aliased something, I think it's fine to just decide it's out of scope for this rule. This rule is just supposed to be a gentle reminder that you may have installed a dependency locally without properly declaring it in package.json. Once a dependency is more complicated than that, I think it's fine if it goes off the radar for this rule. |
|
@ljharb @benmosher cool :) do you want me to squash it/rebase it, or you will do it through GitHub when merging? |
|
Doing it through github sadly doesn't actually update your PR branch; it's definitely ideal if you do it; but if not, one of us can do it for you on the command line prior to merging :-) |
panrafal
force-pushed
the
feature/no-extraneous-deps-ignore
branch
from
4d49f24 to
e80a989
Compare
2 similar comments
ljharb
force-pushed
the
feature/no-extraneous-deps-ignore
branch
from
e80a989 to
ae37318
Compare
no-extraneous-depsassumes, that every package that lives in anode_modulesfolder or cannot be resolved is an external package. This happens because of that line.If project uses aliasing, or transforms imports, this assumption is wrong, as they may not be resolvable by import plugin, but also, they should not be defined in package.json.
Changing
isExternalPathwill make any ordering lints unstable (order may change depending if you rannpm installor not). My proposal, is thatno-extraneous-depsignores packages that cannot be resolved, as otherwise it's impossible to test whether they live innode_modulesor somewhere else.In #903, we talked about using
import/ignoresetting, but I think it doesn't make sense, as it's mostly used to ignore imports unknown to the plugin, like .css files. But still, you may import a css file from a dependency, and this rule should check that.Both with ignore and without are parts of this PR - I'll squash them once we decide.