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

Conversation

Projects
None yet
5 participants
@rfermann
Copy link
Contributor

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.

New: `no-unused-modules` rule
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@coveralls

This comment has been minimized.

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.

Show resolved Hide resolved docs/rules/no-unused-modules.md Outdated

rfermann added some commits Jul 30, 2018

New: `no-unused-modules` rule - minor refactoring
Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - added tests
Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - removed destructoring of context.getF…
…ilename

Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - minor refactoring
Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - added more tests
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann

This comment was marked as resolved.

Copy link
Contributor Author

rfermann commented Jul 31, 2018

One of the last travis builds timed out during npm install. Is there a way to restart this build without making a new commit?

Show resolved Hide resolved 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
@coreyfarrell

This comment has been minimized.

Copy link

coreyfarrell commented Aug 2, 2018

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

This comment has been minimized.

Copy link
Collaborator

ljharb commented Aug 3, 2018

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

rfermann added some commits Aug 5, 2018

New: `no-unused-modules` rule - added default src, more comprehensive…
… sanity checks

Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - add support for 'import *'
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann

This comment has been minimized.

Copy link
Contributor Author

rfermann commented Aug 19, 2018

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

This comment has been minimized.

Copy link
Contributor Author

rfermann commented Sep 1, 2018

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

@ljharb

This comment has been minimized.

Copy link
Collaborator

ljharb commented Sep 1, 2018

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

@rfermann

This comment has been minimized.

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

This comment has been minimized.

Copy link
Collaborator

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.

rfermann added some commits Sep 12, 2018

New: `no-unused-modules` rule - renamed 'ignore' option to 'ignoreExp…
…orts', revised docs

Signed-off-by: René Fermann <rene.fermann@gmx.de>
New: `no-unused-modules` rule - corrected typo in docs
Signed-off-by: René Fermann <rene.fermann@gmx.de>
@rfermann

This comment has been minimized.

Copy link
Contributor Author

rfermann commented Oct 14, 2018

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

@ljharb
Copy link
Collaborator

ljharb left a comment

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

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
Show resolved Hide resolved src/rules/no-unused-modules.js Outdated
Show resolved Hide resolved src/rules/no-unused-modules.js Outdated
Fermann
New: `no-unused-modules` rule - reworked schema, removed UNDEFINED va…
…riable, fixed documentation

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

ljharb left a comment

Please ensure all files have trailing newlines.

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 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
@rfermann

This comment has been minimized.

Copy link
Contributor Author

rfermann commented Dec 18, 2018

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 😕

Show resolved Hide resolved 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
specifiers.some(({ type }) => type === IMPORT_DEFAULT_SPECIFIER)

module.exports = {
isNodeModule,

This comment has been minimized.

Copy link
@ljharb

ljharb Dec 23, 2018

Collaborator

why is this exported?

This comment has been minimized.

Copy link
@rfermann

rfermann Dec 23, 2018

Author Contributor

Basically for testing, just as you supposed.

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 27, 2019

Collaborator

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

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 tests/src/rules/no-unused-modules.js Outdated
@ljharb
Copy link
Collaborator

ljharb left a comment

@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,

This comment has been minimized.

Copy link
@ljharb

ljharb Mar 27, 2019

Collaborator

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

@rfermann rfermann force-pushed the rfermann:master branch from c788fe6 to 0dd398c Mar 31, 2019


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

This comment has been minimized.

Copy link
@futpib

futpib Apr 5, 2019

Contributor

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

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 6, 2019

Collaborator

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

This comment has been minimized.

Copy link
@rfermann

rfermann Apr 7, 2019

Author Contributor

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.

This comment has been minimized.

Copy link
@ljharb

ljharb Apr 7, 2019

Collaborator

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.

@ljharb

ljharb approved these changes Apr 7, 2019

Copy link
Collaborator

ljharb left a comment

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

@ljharb ljharb merged commit 3aefa79 into benmosher:master Apr 7, 2019

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+3.6%) to 97.775%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.