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

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

Closed
ntwb opened this issue Jul 29, 2016 · 15 comments
Closed

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

ntwb opened this issue Jul 29, 2016 · 15 comments
Labels

Comments

@ntwb
Copy link

ntwb commented Jul 29, 2016

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
Copy link
Owner

mysticatea commented Jul 29, 2016

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
Copy link
Owner

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
Copy link

Twipped commented Oct 11, 2016

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
Copy link

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

@jokeyrhyme
Copy link
Contributor

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
Copy link
Owner

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

@Ginden
Copy link

Ginden commented Sep 6, 2017

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
Copy link

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
Copy link

mjlescano commented Oct 10, 2017

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
Copy link

kuzyn commented Jan 8, 2018

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
Copy link
Author

ntwb commented Jan 8, 2018

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
Copy link

@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
Copy link

l1bbcsg commented Sep 10, 2019

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
Copy link

jtouzy commented May 17, 2020

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
Copy link

KaiSchwarz-cnic commented May 13, 2022

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 🦸

MrChocolatine added a commit to DazzlingFugu/ember-reading-time that referenced this issue Feb 12, 2023
What was done and why:

- After the `stream` module, the modules `util` and `process` had to be
  added to the dependencies as well in order to satisfy Webpack v5 which
  does not include polyfills anymore, otherwise tests were failing.
  - https://webpack.js.org/migrate/5/
  - https://webpack.js.org/configuration/resolve/#resolvefallback
  - https://github.com/ef4/ember-auto-import/blob/v2.6.0/docs/upgrade-guide-2.0.md
  - https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

  For the `process` module, adding it to the `fallback` key was not
  working, it had to be included in the `plugins` key:
  - https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined/65018686#65018686
  - https://discord.com/channels/480462759797063690/898671957007011841/898682272847384596
    Overall, these messages on Discord give the entire final solution in
    this commit.

- `wepack` moved to the dependencies because otherwise `eslint-plugin-n`
  was returning the error:

  ```
  "webpack" is not published.eslintn/no-unpublished-require
  ```

  See:
  - https://github.com/eslint-community/eslint-plugin-n/blob/15.6.1/docs/rules/no-unpublished-require.md
  - mysticatea/eslint-plugin-node#47
MrChocolatine added a commit to DazzlingFugu/ember-reading-time that referenced this issue Feb 12, 2023
What was done and why:

- After the `stream` module, the modules `util` and `process` had to be
  added to the dependencies as well in order to satisfy Webpack v5 which
  does not include polyfills anymore, otherwise tests were failing.
  - https://webpack.js.org/migrate/5/
  - https://webpack.js.org/configuration/resolve/#resolvefallback
  - https://github.com/ef4/ember-auto-import/blob/v2.6.0/docs/upgrade-guide-2.0.md
  - https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

  For the `process` module, adding it to the `fallback` key was not
  working, it had to be included in the `plugins` key:
  - https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined/65018686#65018686
  - https://discord.com/channels/480462759797063690/898671957007011841/898682272847384596
    Overall, these messages on Discord give the entire final solution in
    this commit.

- `wepack` moved to the dependencies because otherwise `eslint-plugin-n`
  was returning the error:

  ```
  "webpack" is not published.eslintn/no-unpublished-require
  ```

  See:
  - https://github.com/eslint-community/eslint-plugin-n/blob/15.6.1/docs/rules/no-unpublished-require.md
  - mysticatea/eslint-plugin-node#47
BlueCutOfficial pushed a commit to DazzlingFugu/ember-reading-time that referenced this issue Mar 15, 2023
What was done and why:

- After the `stream` module, the modules `util` and `process` had to be
  added to the dependencies as well in order to satisfy Webpack v5 which
  does not include polyfills anymore, otherwise tests were failing.
  - https://webpack.js.org/migrate/5/
  - https://webpack.js.org/configuration/resolve/#resolvefallback
  - https://github.com/ef4/ember-auto-import/blob/v2.6.0/docs/upgrade-guide-2.0.md
  - https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

  For the `process` module, adding it to the `fallback` key was not
  working, it had to be included in the `plugins` key:
  - https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined/65018686#65018686
  - https://discord.com/channels/480462759797063690/898671957007011841/898682272847384596
    Overall, these messages on Discord give the entire final solution in
    this commit.

- `wepack` moved to the dependencies because otherwise `eslint-plugin-n`
  was returning the error:

  ```
  "webpack" is not published.eslintn/no-unpublished-require
  ```

  See:
  - https://github.com/eslint-community/eslint-plugin-n/blob/15.6.1/docs/rules/no-unpublished-require.md
  - mysticatea/eslint-plugin-node#47
BlueCutOfficial pushed a commit to DazzlingFugu/ember-reading-time that referenced this issue Apr 28, 2023
* build: upgrade Ember.js from v3.28.3 to v4.10.0

* refactor: use buil-in tests setup wrappers

* refactor: standardise helper & fix types checks

- Standardise helper's function signature and body
  To match what is generated by default when creating a new helper.

- Fix types errors provided by implicit TypeScript checks
  This will help whenever the project is migrated to TS

* test: fix test, add required Webpack polyfills

What was done and why:

- After the `stream` module, the modules `util` and `process` had to be
  added to the dependencies as well in order to satisfy Webpack v5 which
  does not include polyfills anymore, otherwise tests were failing.
  - https://webpack.js.org/migrate/5/
  - https://webpack.js.org/configuration/resolve/#resolvefallback
  - https://github.com/ef4/ember-auto-import/blob/v2.6.0/docs/upgrade-guide-2.0.md
  - https://gist.github.com/ef4/d2cf5672a93cf241fd47c020b9b3066a

  For the `process` module, adding it to the `fallback` key was not
  working, it had to be included in the `plugins` key:
  - https://stackoverflow.com/questions/65018431/webpack-5-uncaught-referenceerror-process-is-not-defined/65018686#65018686
  - https://discord.com/channels/480462759797063690/898671957007011841/898682272847384596
    Overall, these messages on Discord give the entire final solution in
    this commit.

- `wepack` moved to the dependencies because otherwise `eslint-plugin-n`
  was returning the error:

  ```
  "webpack" is not published.eslintn/no-unpublished-require
  ```

  See:
  - https://github.com/eslint-community/eslint-plugin-n/blob/15.6.1/docs/rules/no-unpublished-require.md
  - mysticatea/eslint-plugin-node#47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests