eslint-plugin-import
eslint-plugin-import copied to clipboard
import-js
no-unused-modules needs an ignoreImports option
While ignoreExports allows us to ignore unused exports for specific files, we need to be able to ignore used imports.
Why?
Let's assume there's three files:
- entry.js
- utility.js
- utility.spec.js
Both entry.js and utility.spec.js import from utility.js. Here, we'd want to include entry.js and utility.spec.js in ignoreExports since one is an entry point and the other is a test. We're ensured if both files stop importing from utility.js then the eslint rule will error, which is good!
The issue is when just entry.js stops importing from utility.js, which is what we want this rule to identify. Right now, the tests will keep utility.js from ever being identified as unused, even though practically it's unused by the application. I'd have to manually discover that the application isn't using utility.js. This will never happen; I'd assume that the change in entry.js would identify this for me, but I'd have to manually check if utility.spec.js is the only one importing it, which negates the value of this rule. If I could declare in options to ignoreImports from **/*.spec.*, then utility.js will be identified as unused, which would be good!.
Currently, there's no way to accomplish this. Including **/*.spec.* in ignoreExports, setting src to ['**/!(*.spec.js)'], or having an override where excludedFiles includes **/*.spec.* all do the same thing, which isn't what I want. We specifically need to identify files where we want to ignore their imports. The moment we write a unit test or a storybook story, we're essentially disabling the useful of this rule.
I'm finding that the only way to accomplish this is to put the rule in an overrides, include the pattern in excludedFiles, and exclude the pattern in src. You can't follow the same glob rules in excludedFilesandsrc`, so it's unmaintainable to accomplish this.
Typically, you'd apply the rule to your codebase, and use src to limit to just non-test files, and use ignoreExports to indicate entry points.
I'm not clear on the primary issue you're having with that setup - namely, ignoreExports for entry.js, and src for utility.js.
In my current setup, ignoreExports has entry.js, and src is limited to utility.js, which is what @ljharb is suggesting. If I edit entry.js to stop importing utility.js, eslint does not raise an error. Only if I also put the entire rule in an override and include utility.spec.js in excludedFiles will this actually raise an error.
ahhh - you're saying that utility.js, being NOT an entry point, and being only imported from non-src files, should be reported as unused? I do agree with that expectation.
Exactly! My solution was to introduce an ignoreImports option to identify them (since using negates in src is very awkward) but whatever solves the problem.
I have exactly the same problem: If a file is only imported by tests, I consider it unused and would like eslint to tell me I can safely delete it.
I wonder if it would be clearer if this rule took an entryPoints glob array, and either a productionFiles glob array OR a nonProductionFiles glob array (the naming here sucks, but we can bikeshed that later).
These options could be mutually exclusive with the current ones, but that way we'd more clearly match the intended use case: entry points do not have their imports checked; non-production files do not have their exports checked, but their imports DO count towards "used"; non-entrypoint production files have all their exports considered unused unless another file in the project imports them.
Thoughts?
This is more about preference, but I find that these options are easier to digest if they explain what they do rather than what they're for. entryPoints, productionFiles, and nonProductionFiles all explain what they're for, but I will forget what they do and will have to constantly check documentation to make sure I'm using them right. ignoreImports and ignoreExports (or something like them) are very clear about what they do and I barely need documentation to understand what's going on.
Hmm, I have the opposite intuition. When I see my proposed config names, paired with file globs, i'll immediately understand what they're labeling, and the implications are safely encapsulated into the rule - whereas with the current config names, I have to actually understand the implementation details of the rule to be able to understand what files I should target there.
In other words, I think the minimum knowledge should be "this rule tells me when things are unused", not "ignoring X and Y helps me properly determine when things are unused".
To each their own! I do feel like my suggestion fits the tone of the other options in the package as well. Not a big deal to me; as long as I can accomplish what I'm asking for, I'm sure I'll set this setting once and stop thinking about it. 😊
Your suggestion absolutely does fit the existing ignoreExports option! My suggestion is to effectively deprecate this option, in favor of a new triplet of options that match the semantics.
I meant to say: it fits the tone of the other options of the other rules in the package as well.
What's the next steps here? Would love to have this! Not sure if this is a "PRs are welcome" situation; I don't have the bandwidth to purse this.
I just openede a pull request, should be improved with global, help is welcomed.
diff --git a/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js b/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js
index 6a14031..b35e6e5 100644
--- a/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js
+++ b/node_modules/eslint-plugin-import/lib/rules/no-extraneous-dependencies.js
@@ -179,6 +179,7 @@ function reportIfMissing(context, deps, depsOptions, node, name) {
if (
declarationStatus.isInDeps ||
+ depsOptions.allowIgnoreImports ||
depsOptions.allowDevDeps && declarationStatus.isInDevDeps ||
depsOptions.allowPeerDeps && declarationStatus.isInPeerDeps ||
depsOptions.allowOptDeps && declarationStatus.isInOptDeps ||
@@ -195,6 +196,7 @@ function reportIfMissing(context, deps, depsOptions, node, name) {
if (
declarationStatus.isInDeps ||
+ depsOptions.allowIgnoreImports ||
depsOptions.allowDevDeps && declarationStatus.isInDevDeps ||
depsOptions.allowPeerDeps && declarationStatus.isInPeerDeps ||
depsOptions.allowOptDeps && declarationStatus.isInOptDeps ||
@@ -229,6 +231,13 @@ function testConfig(config, filename) {
}
+function testIgnoreImports(config, filename) {
+ if (!config) {
+ return false
+ }
+ return config.some((value) => filename.includes(value.substr(0, 2) === './' ? value.replace('.', process.cwd()) : value))
+}
+
module.exports = {
meta: {
type: 'problem',
@@ -244,6 +253,7 @@ module.exports = {
'optionalDependencies': { 'type': ['boolean', 'array'] },
'peerDependencies': { 'type': ['boolean', 'array'] },
'bundledDependencies': { 'type': ['boolean', 'array'] },
+ 'ignoreImports': { 'type': ['array'] },
'packageDir': { 'type': ['string', 'array'] } },
'additionalProperties': false }] },
@@ -257,6 +267,7 @@ module.exports = {
var deps = getDependencies(context, options.packageDir) || extractDepFields({});
var depsOptions = {
+ allowIgnoreImports: testIgnoreImports(options.ignoreImports, filename) !== false,
allowDevDeps: testConfig(options.devDependencies, filename) !== false,
allowOptDeps: testConfig(options.optionalDependencies, filename) !== false,
allowPeerDeps: testConfig(options.peerDependencies, filename) !== false,
@kopax-polyconseil when it will be merged? Also need it for storybook files
