Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

no-unused-modules needs an ignoreImports option #2400

Open
saiichihashimoto opened this issue Mar 8, 2022 · 18 comments · May be fixed by #2557
Open

no-unused-modules needs an ignoreImports option #2400

saiichihashimoto opened this issue Mar 8, 2022 · 18 comments · May be fixed by #2557

Comments

@saiichihashimoto
Copy link

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.

@saiichihashimoto
Copy link
Author

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.

@ljharb
Copy link
Member

ljharb commented Mar 8, 2022

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.

@saiichihashimoto
Copy link
Author

saiichihashimoto commented Mar 8, 2022

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.

@ljharb
Copy link
Member

ljharb commented Mar 8, 2022

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.

@saiichihashimoto
Copy link
Author

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.

@DamienCassou
Copy link
Contributor

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.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2022

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?

@saiichihashimoto
Copy link
Author

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.

@ljharb
Copy link
Member

ljharb commented Mar 9, 2022

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".

@saiichihashimoto
Copy link
Author

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. 😊

@ljharb
Copy link
Member

ljharb commented Mar 9, 2022

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.

@saiichihashimoto
Copy link
Author

I meant to say: it fits the tone of the other options of the other rules in the package as well.

@saiichihashimoto
Copy link
Author

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.

@saiichihashimoto
Copy link
Author

@ljharb

@ljharb
Copy link
Member

ljharb commented May 30, 2022

I added the “help wanted” label, so yes, that’s the situation.

kopax-polyconseil added a commit to kopax-polyconseil/eslint-plugin-import that referenced this issue Sep 27, 2022
Fix : import-js#2400

Idk how to use globs here, so it's no glob, and support `./` for rootDir
@kopax-polyconseil
Copy link

kopax-polyconseil commented Sep 27, 2022

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,

@happylolonly
Copy link

@kopax-polyconseil when it will be merged? Also need it for storybook files

@ljharb
Copy link
Member

ljharb commented Jul 28, 2023

@happylolonly when the PR review comments are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants