Eslint-plugin-import: include/exclude by glob in no-unused-modules

[benmonro]

I'd like to use no-unused-modules to find files that are exporting but not being imported anywhere, however, i want to exclude all files that match the glob **/*-test.js

[ljharb]

Indeed, there should be a way to specify this glob in the rule config directly.

[rfermann]

This might be related to #1326.

Although globbing is not fully working, your use case should work, @benmonro.

Adding and removing "ignoreExports": ["**/*-test.js"], to my eslint config either ignores or reports unused exports in any files matching the file name pattern.

Can you please re-test this and provide a minimal repo for reproducing this issue in case this is still not working for you? Also feel free to point me in the right direction, if i misunderstood your point.

[benmonro]

@rfermann ok i have a repo showing the issue.

https://github.com/benmonro/unused-modules-example

I've configured the rule in here:
https://github.com/benmonro/unused-modules-example/blob/master/.eslintrc.js#L37

And i added a file called Blah.js and Blah.test.js that imports it. I want the rule to report that Blah.js is not imported anywhere. I'm only interested in finding exports that are not imported in app code. if they are imported in tests it should ignore those.

[benmonro]

btw, I also tried this in the config (with no luck)

    "import/no-unused-modules": ["error", { "unusedExports": true, "ignoreExports": ["src/Blah.test.js"] }]

[benmonro]

@rfermann any update on this? we could really use this. would be good to know if exports are unused in actual app code. what do you think about a new option for ignoreImports that allows us to provide a glob /array of globs to specify paths to ignore in imports when determining if a module is used.

[benmonro]

@rfermann decided to take a stab at this myself, wanted to get your thoughts (or yours @ljharb )

I was able to get my above example to report as an error, however another use case came up that I would like to get your opinion(s) on. If a module exports something, specifically for a test, but then another export in the same file uses the first export, it will still report the error. A pretty common example of this is a redux connected component:

export const MyComponent = () => <div>hello</div>

export default connect(mapStateToProps)(MyComponent);

in this case MyCompont is _only_ imported in a test file, but because it's part of the default export, it _is_ used. So my thought here is to add a new option for tests that would be a little bit different than an ignoreImports or ignoreExports option in that it would only report if the export is imported in a test and not used in the same file. this one could get hairy so figured i'd brain dump and see what your thoughts are...

[rfermann]

Sorry for reacting that late, was packed with other stuff recently. Do you mind sharing how you solved that topic? I suppose the changes needed for that are not that simple.

I understand your example in this way: with export const MyComponent = () => <div>hello</div> you create a component and mark it as a named export, both in one step.
In export default connect(mapStateToProps)(MyComponent); that component gets consumed. But in my opinion it's the component which is consumed here, not the export of that component. Hence the export is still unused, even if the exported component is used somewhere within the same file.

[ljharb]

@benmonro things that are only used in tests should not be tested or exported at all, so that's a problem unrelated to linting :-)

[benmonro]

@ljharb lol yes, tell that to the people who don't work here anymore. believe me I want to clean all of those up but we have like 1400 instances of it. 🎉

@rfermann yeah as a test i added a check at this line: https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-unused-modules.js#L175 that looked something like this:

              if (!ignoredFiles.has(listKey)) {
                whereUsed.add(listKey)
              }

at a bare minimum it did at least show us potential dead code, but it also highlighted the redux example above that we are not contending with.

[rfermann]

Just experimented a bit with .eslintignore and your example. When I add **/*.test.js to this file, I get 2:1 error exported declaration 'Blah' not used within other modules as the only linting error.

Another option could be providing an ignoreImports option (just as you suggested), reading all files specified by that option and removing them from the list of source files before the source files are analyzed. I already prototyped that using @benmonro s example and it also only reports 2:1 error exported declaration 'Blah' not used within other modules

The second approach would have the advantage of not switching off other eslint rules for the files specified in .eslintignore

@benmonro what do you think of these 2 options?

[benmonro]

@rfermann option 1 is a hard no. We can't turn off lint on test files.

Option 2 will work but as I mentioned in the redux connect example above I do believe it needs to also check for usages in the same file that re export. While I agree with Jordan's point, I do believe that the pattern of exporting parts of files just for tests is a pretty common usage so the rule should take that into account. Otherwise using this rule would have a huge barrier to entry.

[rfermann]

I still struggle to understand why the rule should also check for usage in the same file. For that other eslint rules exist.

In your redux example, MyComponent isn't reexported as a default export. Instead redux creates a new component by just consuming MyComponent. In my understanding that are 2 different things.

In case you see that MyComponent is only imported by a test file, you could remove the export keyword. Without this keyword, no-unused-vars might report that component, if it is not used anywhere else. Wouldn't that also solve your use case?

[benmonro]

Because it's (sadly) a common pattern that people export internal functions only for a test. So the problem is that the named exports ARE being used so they wouldn't be reported. In my opinion the main goal is to identify dead code. In the redux example the code is not dead bit it's still being reported.

[ljharb]

To be clear, the following patterns should be supported by this rule:

1. it should be possible to specify one (or ideally more than one) glob of "files whose exports are checked for use"
2. it should be possible to optionally specify one (or ideally more than one) glob of "files whose use of exports is checked". The default value for this should be the previous setting.
3. it should be possible to optionally specify one (or ideally more than one) glob of "entry points", ie, files whose exports are not checked for use.

Specifically, i should be able to use this to warn on code that my production code does not use; i should be able to separately use this to warn on code that nothing in my repo uses. Both of these needs apply to both apps and libraries - for apps, any code that nothing uses, is dead, but for libraries, i have two sets of things to check: that everything is tested (that nothing is unused) and that all production code is either an entrypoint or used by production code.

[benmonro]

@ljharb I agree with all of that but to be clear, are you saying you do think the rule should not report on the use case I'm showing? i.e. Not reporting on an export that is used by a test AND another export? But do report on an export that is only used by a test, and nothing else? (at least in the case of an app, i can see some differences / options for libraries.

[ljharb]

@benmonro I think that it should be possible to configure either scenario.

[ascpeter]

I could use this feature as well. I installed eslint-plugin-import to help cleanup dead code, but most everything has a test file so it's not helping so far.