mysticatea / eslint-plugin-node

Additional ESLint's rules for Node.js
MIT License
962 stars 171 forks source link

Issue with `no-unpublished-require` and `devDependencies` modules #47

Closed ntwb closed 7 years ago

ntwb commented 8 years ago

If I use this code:

const test = require("ava")

With the rule:

'node/no-unpublished-require': 'error',

And AVA is in my package.json:

  "devDependencies": {
    "ava": "^0.15.1",
  },

I get the warning:

1:22  error  "ava" is not published  node/no-unpublished-require

The rule readme says:

"...If the file require is written is not published then it's also OK that "devDependencies" field of package.json includes the module."

Am I correct in reading that this should not error if the module is in devDependencies?

mysticatea commented 8 years ago

Thank you for this issue.

"published" is that satisfying the following conditions:

  • If it's a file:
    • "files" field of package.json includes the file, or the field is nothing.
    • .npmignore does not include the file.
  • If it's a module:
    • "dependencies" or "peerDependencies" field of package.json includes the module. If the file require is written is not published then it's also OK that "devDependencies" field of package.json includes the module.

That rule disallows imports of unpublished files/modules from published files. (Because such imports may cause severe problems after npm publish) If the test file is published, the rule disallows imports of devDependencies since the rule handles devDependencies as unpublished.

If you don't have both "files" field of package.json and .npmignore, your all files would be published.

mysticatea commented 8 years ago

https://github.com/mysticatea/eslint-plugin-node/blob/master/package.json#L5

Like this, I'd like to recommend to use "files" field of package.json.

Twipped commented 8 years ago

This tripped me up as well. After reading this issue I understand why, but I feel like the docs for this rule could be more clear in stating that the rule will fail if you try to load devDependencies.

I surely can't be the only person using this rule in an application (as opposed to a package) that will never be published to npm.

pjanuario commented 8 years ago

+1 here, supertest and knex-cleaner package are causing that problem in my new API too. :/

jokeyrhyme commented 8 years ago

I would definitely not want to publish code that has require('lodash') if "lodash" is only a devDependency. That would be broken. So I totally appreciate the way this rule currently operates.

That said, I do think we need a way to change the way this rule works for development files (e.g. tests). It's absolutely okay for a test to require('ava') as long as "ava" is a devDependency. But it would be a bad thing if "ava" was not mentioned at all in package.json.

For now, I'm just going to disable this rule for my test directories. But it would be nice to have a way to protect them from mistakes, too.

mysticatea commented 8 years ago

A few day ago, I updated the document about this.

Ginden commented 7 years ago

This issue happens to me very often, can we get other error message if dependency is present in devDependencies? Like dev dependency used in published code or something like that.

jaydenseric commented 7 years ago

This issue comes up when you have development scripts.

For example, I have a script that requires graphql-config to update the local schema file from a configured remote GraphQL API. This update-schema script will only be run in development, thus, all dependencies of the script should be dev dependencies.

It's a very confusingly named lint error, because graphql-config totally is published.

mjlescano commented 7 years ago

If it helps someone, just fixed this adding another .eslintrc inside my test folder (which is where my devDependencies are used) with this:

test/.eslintrc:

{
  "rules": {
    "node/no-unpublished-require": 0,
    "node/no-missing-require": 0
  }
}
kuzyn commented 6 years ago

And for other like me who keep their tests next to their components rather than in one top level folder, you can add an override to your top .eslintrcfile:

{
  "overrides": [{
    "files": "**/*.test.js",
    "rules": {
        "node/no-unpublished-require": 0,
        "node/no-missing-require": 0
    }
  }]
} 
ntwb commented 6 years ago

Another optional workaround is to include the devependancy in peerDepenancies, this does depend on what the dep is and how it's used I guess.

StarpTech commented 5 years ago

@ntwb Just to fullfill eslint-node-plugin needs I would never move my project dependencies to peer that can produce other effects. After reading the issue the solution should be clear: Either you

  1. define what's really published via files property
  2. Overwrite eslint config for those files

@mysticatea What's left here is the situation that this rule shouldn't be applied on private packages because they can't be published. In my case I get errors for private packages.

l1bbcsg commented 5 years ago

I also encountered this problem with a private unpublished project.

The way to fix this was to add files field to package.json and keep it empty (empty array). This way all of project files became unpublished and therefore stopped triggering the rule.

Depends on how you look at it this might be either a proper solution or an ugly workaround, I'm not sure myself.

jtouzy commented 4 years ago

Old issue, but devDependencies can be easily allowed in some files with something like this:

const packageJson = require('./package.json')
const devDependencies = Object.keys(packageJson.devDependencies || {})

module.exports = {
  // your eslint config...
  rules: {
    'node/no-unpublished-require': ['error', {
      'allowModules': devDependencies
    }]
  }
}
KaiSchwarz-cnic commented 2 years ago

Just to have it mentioned for others as I had a bit of trouble understanding the issue related to devDependencies ad-hoc, but figured it out. Great to see this checked btw. The check is stating out that files are getting published to npmjs.com loading devDependencies via require. So, add all files / folders of your dev scripts or automated tests to .npmignore and you're fine. It shouldn't be of interest to publish dev scripts and automated tests together with your "production" package.

Thanks again for getting this checked - leading to very clean published packages! GJ :superhero: