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

Add ability to ignore certain packages from all rules #275

Closed
sindresorhus opened this issue Apr 26, 2016 · 12 comments
Closed

Add ability to ignore certain packages from all rules #275

sindresorhus opened this issue Apr 26, 2016 · 12 comments

Comments

@sindresorhus
Copy link

sindresorhus commented Apr 26, 2016

Electron exposes an const electron = require('electron'); import, but it's not listed anywhere, as it's more of an additional built-in. Same goes for atom, when creating Atom plugins.

I think we could just ignore these by default as no one would want it to be linted, but there also might be other cases of this, so would still be useful to have an ignore option.

Here's the errors I get when running this plugin on an Electron project:

  browser.js:2
  ✖  2:18  'electron' is not listed in the project's dependencies. Run 'npm i -S electron' to add it  import/no-extraneous-dependencies
  ✖  2:26  Unable to resolve path to module 'electron'.                                               import/no-unresolved

  index.js:4
  ✖  4:18  'electron' is not listed in the project's dependencies. Run 'npm i -S electron' to add it  import/no-extraneous-dependencies
  ✖  4:26  Unable to resolve path to module 'electron'.                                               import/no-unresolved

  menu.js:3
  ✖  3:18  'electron' is not listed in the project's dependencies. Run 'npm i -S electron' to add it  import/no-extraneous-dependencies
  ✖  3:26  Unable to resolve path to module 'electron'.                                               import/no-unresolved

  storage.js:4
  ✖  4:18  'electron' is not listed in the project's dependencies. Run 'npm i -S electron' to add it  import/no-extraneous-dependencies
  ✖  4:26  Unable to resolve path to module 'electron'.                                               import/no-unresolved

  tray.js:2
  ✖  2:18  'electron' is not listed in the project's dependencies. Run 'npm i -S electron' to add it  import/no-extraneous-dependencies
  ✖  2:26  Unable to resolve path to module 'electron'.                                               import/no-unresolved

Example import: https://github.com/sindresorhus/caprine/blob/2acdd358eafdc42b45b532c90e87ba5465a30de7/index.js#L4

@benmosher
Copy link
Member

What loads modules in an Electron project?

I.E. does it have its own Browserify- or Webpack-like bundler? or is it just a special Node process and imports get transpiled to CommonJS?

I can imagine solving this with either

  • an Electron import resolver: someone writes eslint-import-resolver-electron. Probably only worthwhile if there is other Electron config that impacts module path resolution.

or

  • an import/builtins String[]-valued setting that could be set in a plugin:import/electron shared config or whatever.

An import/builtins setting could alternatively be a reference to a package/module that matches the interface of your builtin-modules npm package (also a String[]?), which @jfmengels is using as the definition of the builtiin import type. no-unresolved could be enhanced to be sensitive to this as well.

@sindresorhus
Copy link
Author

sindresorhus commented Apr 27, 2016

Electron embeds Node.js and injects the electron module into it using internal API's.

an Electron import resolver: someone writes eslint-import-resolver-electron. Probably only worthwhile if there is other Electron config that impacts module path resolution.

Probably not. Electron only has one export => electron.

an import/builtins String[]-valued setting that could be set in a plugin:import/electron shared config or whatever.

Yeah, that would work, although I'd argue electron and atom should be included by default as it makes no sense to lint them this way.

@benmosher
Copy link
Member

Actually, maybe an env: electron: true would be a decent way to trigger this.

The existing builtins could be sensitive to node env as well, which might be a decent solution to rejecting node builtins. I thought someone had asked for this but I can't find the issue ATM.

@rgbkrk
Copy link

rgbkrk commented May 14, 2016

Is there a way to specify a list of built-ins for Electron's sake?

@benmosher
Copy link
Member

@rgbkrk: not yet. I was thinking to add env sensitivity and configure it that way. I think the Node resolver will need to be involved though--need to know that the module resolves but there is no path for it.

I'll take a look soon, can probably do something simple.

@benmosher benmosher added this to the next milestone May 14, 2016
@andreieftimie
Copy link

Any simple workaround for this issue?
Trying unsuccessfully to have electron as an ignored package.

@jacobwgillespie
Copy link

@andreieftimie and others looking for a temporary workaround, I solved this by creating an additional fake "node_modules" with the package I want to ignore (in this case, electron) and configuring eslint-plugin-import to use that as an additional modules directory.

Here is the relevant change: radiant-player/radiant-player-electron@9935f8f

@andreieftimie
Copy link

@jacobwgillespie that seems better than my solution, which was to completely disable the rule

    // TEMP untill can ignore `electron`
    "import/no-unresolved": 0,

@benmosher
Copy link
Member

There is always eslint-disable-line.

@benmosher
Copy link
Member

Published with v1.10.0, see import/core-modules and the shared electron config.

@benmosher benmosher modified the milestones: v1.10.0, next Jun 30, 2016
@sjungwirth
Copy link

sjungwirth commented Jul 11, 2016

It is great to have this ignore certain external module feature, but I am having a slightly different use case: I have a symlink app in my node_modules folder so that within my app code I can do import 'app/path/file' without messing with relative path imports, except now I get this lint error everywhere. I tried adding 'app' to the core-modules setting, and while it works great for some other truly core-modules that I have, app doesn't work, because my imports don't match the configured value "app/path/file" != "app" and "app" is not identified as an external module.

[edit] I think also if you don't like my use case, the same issue is present if I want to import something like external/submodule. The fix could be as easy as split('/')[0] on the module being imported before testing against the core-modules array.

@benmosher
Copy link
Member

benmosher commented Jul 11, 2016

@sjungwirth: you've got an interesting point. However, the node resolver's paths setting might be a better answer for your case: https://github.com/substack/node-resolve/blob/master/readme.markdown

The node resolver takes the same options as the node-resolve package: https://www.npmjs.com/package/eslint-import-resolver-node

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

Successfully merging a pull request may close this issue.

6 participants