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

feat: detect extraneous/missing imports/requires #101

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

julien-f
Copy link

  • extraneous: modules that are absent from package.json
  • missing: modules that do not exist (necessary for local modules)

I have started testing this config in my projects, it seems to work fine, I did not see any major perf impact but we should pay attention to it.

- extraneous: modules that are absent from `package.json`
- missing: modules that do not exist (necessary for local modules)
@julien-f
Copy link
Author

julien-f commented Dec 5, 2017

Any idea why the tests are failing?

@LinusU
Copy link
Member

LinusU commented Dec 5, 2017

It seems like the following code is now producing two errors 🤔

var foo = 1
var bar = function () {}
bar(foo)

var code = 'var foo = 1\nvar bar = function () {}\nbar(foo)\n'
t.equal(cli.executeOnText(code).errorCount, 0)

@julien-f
Copy link
Author

julien-f commented Dec 5, 2017

@mysticatea any idea?

@mysticatea
Copy link

mysticatea commented Dec 6, 2017

eslint-plugin-node is an old version which does not include those rules.

{ filePath: '<text>',
  messages:
   [ { ruleId: 'node/no-extraneous-import',
       severity: 2,
       message: 'Definition for rule \'node/no-extraneous-import\' was not found',
       line: 1,
       column: 1,
       nodeType: 'Program',
       source: 'var foo = 1' },
     { ruleId: 'node/no-extraneous-require',
       severity: 2,
       message: 'Definition for rule \'node/no-extraneous-require\' was not found',
       line: 1,
       column: 1,
       nodeType: 'Program',
       source: 'var foo = 1' } ],
  errorCount: 2,
  warningCount: 0,
  source: 'var foo = 1\nvar bar = function () {}\nbar(foo)\n' }

@julien-f
Copy link
Author

julien-f commented Dec 6, 2017

Thank you!
I have upgraded eslint-plugin-node to 5.2.1 and the tests are passing.

@mysticatea what's your opinion of enabling these rules in standard and do you have any suggested config?

@mysticatea
Copy link

@julien-f It's a good idea. But if users use AMD's require or customized lookup algorithm, it might make false positive.

@voxpelli
Copy link
Member

I'm not sure this belongs in a javascript linter but leaning towards that it might rather belong in something that focused on linting ones dependencies.

Apart from the linting this PR mentions such linting can eg:

  • Check security issues (something npm now does)
  • Check that lock-file and package.json is in sync
  • Check that the currently installed modules are the ones they are supposed to be
  • Check that all included modules comply with the projects specified version range.

(Disclaimer: I co-maintain one such linter, https://github.com/maxogden/dependency-check, and have created a complementary such linter myself as well, https://github.com/voxpelli/node-installed-check/)

As eslint-plugin-node already supports this, I guess it doesn't hurt, but it may make people feel safe about their dependency linting rather than looking into a more complete solution for dependency linting

Copy link
Member

@theoludwig theoludwig left a comment

Choose a reason for hiding this comment

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

LGTM!
It is great new rules for standard to ensures we don't have missing dependencies or modules. 👍

Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

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

I think this is out of scope of what standard should be caring about.

And also: If we are to include this we anyways need to run it against current users of standard first to see how much of a regression it would be for standard's users and then consider whether its something that's feasible to push or whether it will cause havoc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

6 participants