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

New: no-unused-modules rule #1142

Merged
merged 20 commits into from
Apr 7, 2019
Merged

New: no-unused-modules rule #1142

merged 20 commits into from
Apr 7, 2019

Conversation

rfermann
Copy link
Contributor

@rfermann rfermann commented Jul 23, 2018

As discussed in #186, this rule adds the possibility to find modules without any exports and/or exports within a module, which aren't used in other modules

ToDos:

  • implement feature/fix bug
  • update docs
  • write tests
  • make a note in change log

This is the first rule I created. Writing tests for it may therefor take some additional time.

Signed-off-by: René Fermann <rene.fermann@gmx.de>
@coveralls
Copy link

coveralls commented Jul 23, 2018

Coverage Status

Coverage increased (+3.6%) to 97.775% when pulling d35b7ff on rfermann:master into af976b9 on benmosher:master.

docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
rfermann added 5 commits July 30, 2018 18:26
Signed-off-by: René Fermann <rene.fermann@gmx.de>
Signed-off-by: René Fermann <rene.fermann@gmx.de>
…ilename

Signed-off-by: René Fermann <rene.fermann@gmx.de>
Signed-off-by: René Fermann <rene.fermann@gmx.de>
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann

This comment has been minimized.

docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
tests/src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
@coreyfarrell
Copy link

While looking into how ignore is implemented I noticed a couple language features which are not tested.

import *

I'm unsure what should happen when import * as localVar is used, any limitations should be documented. Take the following source:

import * as localVar from './file-1';
localVar.something();

Can we detect that we're actually using the named export something? If not I think the import * syntax should cause all exports of ./file-1 to be marked as used and we should document how it works. Probably suggest also using a rule that forbids use of import *.

export from

Given file-2.js containing export {something} from './file-1', tests should verify this is recognized as:

  1. An import, so even if nothing else imports ./file-1 it should not be reported as unused.
  2. An export, if nothing imports something from file-2.js it is reported.

dynamic import

I suspect dynamic import cannot be detected, this should be mentioned in the docs.

@ljharb
Copy link
Member

ljharb commented Aug 3, 2018

Yes, an import * should mark all exports as used, i think.

… sanity checks

Signed-off-by: René Fermann <rene.fermann@gmx.de>
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann
Copy link
Contributor Author

Support for import * from 'somewhere', export * from 'somewhere' and export { stuff } from 'somewhere' has been added with the last 2 commits.
The documentation has been adjusted accordingly.

How can I analyze, why the AppVeyor build is failing all the time? It btw also fails for tests I haven't touched at all 😕

@rfermann rfermann changed the title WIP: New: no-unused-modules rule New: no-unused-modules rule Sep 1, 2018
@rfermann
Copy link
Contributor Author

rfermann commented Sep 1, 2018

@ljharb: do you have any ideas how to tackle the failing AppVeyor build?

@ljharb
Copy link
Member

ljharb commented Sep 1, 2018

@rfermann i believe it's on master, and unrelated to this PR.

@rfermann
Copy link
Contributor Author

rfermann commented Sep 2, 2018

Okay, good to know.

What else needs to be done from my site to let this PR be merged? Should I adjust the change log? Or is that one of these tasks you will perform @ljharb?

@ljharb
Copy link
Member

ljharb commented Sep 9, 2018

@rfermann you don't have to worry about the changelog; the appveyor issue should be fixed on master if you rebase.

…orts', revised docs

Signed-off-by: René Fermann <rene.fermann@gmx.de>
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann
Copy link
Contributor Author

@ljharb , @benmosher: what else am I expected to do to get this PR merged? Is it the outstanding rebase which is blocking the merge?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing a fresh review - looks good overall. Just a few comments.

docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
…riable, fixed documentation

Signed-off-by: Fermann <rene.fermann@capgemini.com>
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure all files have trailing newlines.

docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
docs/rules/no-unused-modules.md Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
@rfermann
Copy link
Contributor Author

Thanks for your latest feedback. I implemented your suggestions.

Please ensure all files have trailing newlines.

I don't get that. Did I forgot the trailing newline anywhere? I checked the files I pushed today and all already had a trailing newline 😕

docs/rules/no-unused-modules.md Show resolved Hide resolved
tests/files/no-unused-modules/file-b.js Outdated Show resolved Hide resolved
tests/src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
specifiers.some(({ type }) => type === IMPORT_DEFAULT_SPECIFIER)

module.exports = {
isNodeModule,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically for testing, just as you supposed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't need direct testing; let's please not export it.

src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
tests/src/rules/no-unused-modules.js Outdated Show resolved Hide resolved
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfermann hey! i know it's been awhile; are you still interested in addressing the outstanding comments, rebasing, and ensuring the tests pass?

specifiers.some(({ type }) => type === IMPORT_DEFAULT_SPECIFIER)

module.exports = {
isNodeModule,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't need direct testing; let's please not export it.


module.exports = {
meta: {
docs: { url: docsUrl('no-unused-modules') },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the name no-unused-modules misleading? This rule reports individual export declarations, but the name suggests it reports whole files as unused.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my understanding is that it reports on both - both unused files, and unused exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as @ljharb says, it reports on both:

  • modules without any exports are reported here
  • and modules with unused exports are reported here and here

However this rule currently does not report, if a module only consists of unused exports. Each of these exports is reported separately.
I am not sure whether it makes sense to have one single report being triggered if none of the existing exports is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does make sense to me in that case to only provide one report; just as I’d expect when a module has no exports.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, let's give this a shot :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants