-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Comments
I'm finding that the only way to accomplish this is to put the rule in an |
Typically, you'd apply the rule to your codebase, and use I'm not clear on the primary issue you're having with that setup - namely, |
In my current setup, |
ahhh - you're saying that |
Exactly! My solution was to introduce an |
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 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. |
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 |
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 added the “help wanted” label, so yes, that’s the situation. |
Fix : import-js#2400 Idk how to use globs here, so it's no glob, and support `./` for rootDir
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 |
@happylolonly when the PR review comments are addressed. |
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:
Both
entry.js
andutility.spec.js
import fromutility.js
. Here, we'd want to includeentry.js
andutility.spec.js
inignoreExports
since one is an entry point and the other is a test. We're ensured if both files stop importing fromutility.js
then the eslint rule will error, which is good!The issue is when just
entry.js
stops importing fromutility.js
, which is what we want this rule to identify. Right now, the tests will keeputility.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 usingutility.js
. This will never happen; I'd assume that the change inentry.js
would identify this for me, but I'd have to manually check ifutility.spec.js
is the only one importing it, which negates the value of this rule. If I could declare in options toignoreImports
from**/*.spec.*
, thenutility.js
will be identified as unused, which would be good!.Currently, there's no way to accomplish this. Including
**/*.spec.*
inignoreExports
, settingsrc
to['**/!(*.spec.js)']
, or having an override whereexcludedFiles
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.The text was updated successfully, but these errors were encountered: