Eslint-plugin-import: no-extraneous-dependencies doesn't support nested package.json

[nkt]

Example of my project structure:

├── package.json
├── src
│   ├── components
│   │   ├── Avatar
│   │   │   ├── Avatar.css
│   │   │   ├── Avatar.js
│   │   │   ├── README.md
│   │   │   └── package.json
│   │   ├── Button
│   │   │   ├── Button.css
│   │   │   ├── Button.js
│   │   │   ├── ButtonSpinner.css
│   │   │   ├── ButtonSpinner.js
│   │   │   ├── README.md
│   │   │   └── package.json

Root package.json contains all dependencies, nested package.json looks like this:

{
  "name": "Avatar",
  "main": "Avatar.js"
}

That's why I got wrong warnings:

src/components/Avatar/Avatar.js
  1:1  error  'react' should be listed in the project's dependencies. Run 'npm i -S react' to add it            import/no-extraneous-dependencies
  2:1  error  'classnames' should be listed in the project's dependencies. Run 'npm i -S classnames' to add it  import/no-extraneous-dependencies

Is this fixable?

[jfmengels]

@sindresorhus Any chance you have already written a tool to list all package.json files (like pkg-up but multiple? I might create one otherwise.

@nkt Out of curiosity, what is the use of those package.json files? Do you distribute plenty of packages like this?

[jfmengels]

@benmosher I propose to have an option (name TBD) to use all package.json files, and keep the current behavior by default. I'm not used to monorepos, but I guess that looking at all previous package.json files is not always wanted, depending on how you declare your dependencies for each sub-package.

[nkt]

@jfmengels no, I'm not. I use package.json instead of index.js.

I'm not used to monorepos

I'm too. It's not monorepo. It's react component library.

react-starter-kit uses the same approach.

[sindresorhus]

I haven't. Go ahead.

[benmosher]

@jfmengels I'm thinking maybe this just shouldn't be supported. Autofix would be impossible in this case, plus npm i src/components/Avatar would fail if it did not declare _all_ of its dependencies.

I occasionally run into problems with the resolvers because they are descendants of the folder with the root package.json for this project. CI passes, but installing from npm fails.

Up to you, though, if it's easy and sane to do it behind an option.

[jfmengels]

I'm thinking maybe this just shouldn't be supported

I'm having my doubts too, but I'm not sure what the viable option would be for people who want to have such as project setup, except turn this rule off either entirely or in that folder. Well, either that or stop using package.json instead of index.js just because of this rule.

I do think it's strange to use a package.json file for something that is not a package. But the use case might come for monorepos (though it's probably better to wait for issues from monorepo maintainers to do anything prematurely to handle those.

though, if it's easy and sane

Easy: Meh, have to create a new package and have to add a new dependency here.
Sane: No clue here :sweat_smile:

Any comments from others to help us decide? Especially about the following concern:

I occasionally run into problems with the resolvers because they are descendants of the folder with the root package.json for this project. CI passes, but installing from npm fails.

[nkt]

You should know, package.json allows define custom entry points for browser and server environments:

{
  "name": "GoogleMap",
  "main": "./ServerGoogleMap.js",
  "browser": "./GoogleMap.js"
}

What if rule will check path.join(process.cwd(), 'package.json') and if it exists, use them.
For monorepos you can add monorepo option.

[jfmengels]

You should know, package.json allows define custom entry points for browser and server environments

Didn't know that, thanks!

What if rule will check path.join(process.cwd(), 'package.json')

You would then necessarily need to use ESLint from the root of the project, which is not always the case. I'm not sure your editor does that for instance when you open your workspace folder.

[nkt]

@jfmengels maybe eslint has API which gives root config filename? I mean if config /project/.eslintrc, you can call path.dirname(rootConfigFile) and it will be cwd from previous example.

[jfmengels]

I couldn't find one, but that wouldn't work either for some projects.
1 - You don't necessarily have a .eslintrc next to your package.json

├── package.json
├── client
│   ├── .eslintrc
│   └── file1.js
└── server
    ├── .eslintrc
    └── file2.js

2 - .eslintrc files are cascading, so you can have multiple ones. (I know you said root config, just wanted to clarify for other), but this could have worked if ESLint gave the list of all files and we could have taken the "first" one.

This still wouldn't address the concern @benmosher mentionned about the sub-packages not declaring all their dependencies, or is that what you meant with the monorepo option?

[acusti]

This would be great for me. I work on a monorepo (here’s why) with a root package.json and sub-directories with their own package.json. As it stands, I just have to disable this rule, which is a bummer, because it’s so useful.

As I understand it, in order to resolve the installed dependencies accurately (in a way that matches node’s module system), the getDependencies function would basically need to recurse up all found package.json files, updating the context arg as it goes, until the app root directory, then merge the dependencies object from each package.json (something like Object.assign({}, ..._.pick(packageContents, 'dependencies')), where packageContents is an array of the contents from root to leaf).

I understand the concerns about introducing this, but I would still like to put a strong favorable vote towards adding this as an option. Monorepos are fairly common and probably worth supporting. See Lerna for evidence of this, which can also serve as a way to make a quick example repo test case.

And in terms of the more general question of sub-packages not declaring all their dependencies and therefore having to do multiple npm installs in your codebase, I think that’s just part of the tradeoff one makes. Part of the benefit of Lerna is having a CLI that makes it easy to do monorepo npm stuff.

[jfmengels]

@acusti So am I understanding it right that https://github.com/benmosher/eslint-plugin-import/issues/458#issuecomment-235261668 should resolve your problem?

I'm thinking maybe this just shouldn't be supported. Autofix would be impossible in this case, plus npm i src/components/Avatar would fail if it did not declare all of its dependencies.

@benmosher Would you be fine with adding this as an option? For the autofix, we could disable the autofix altogether if that option is enabled (imagining we get to it one day).

[acusti]

Yup, an option to check all package.json for dependencies would work for me.

[benmosher]

@jfmengels Yeah, that should be fine. I like having it behind an option. and it's fair to ignore autofix until it actually exists 😎

[migueloller]

The option to check all package.json sounds good! Could I suggest the option to specify an array of package.json paths or a blob based on the eslintrc file that is applying the rule?

[jfmengels]

@migueloller That sounds to me like a good idea if you have dependencies declared in /foo/package.json that you do not want to be accessed in /foo/bar/baz.js (meaning that you'd have to re-declare all wanted dependencies in /foo/bar/package.json).

Can someone confirm that they have a need for something like this?

[migueloller]

I guess I'll confirm my own reasons 😉 .

If I can specify that the package.json to check is the one at the root then it would ignore other nested package.json. The array would look something like ['./package.json']. This would solve the issue.

Potentially this could also include negation patterns, and other blob features (see .eslintignore).

[migueloller]

Expanding on my previous comment, here's what my ESLint settings look like:

'import/resolver': {
  node: {
    moduleDirectory: [
      'node_modules',
      './src',
    ],
  },
},

Inside src I have 3 main folders: services, components, and scenes (this is for a large React Native app, by the way). Each of those main folders contains a package.json that looks like this (each with their respective names):

{
  "name": "services"
}

Elsewhere in my project I can now import a component (or a service or a scene) by simply typing:

import Button from 'components/Button'

This is much more manageable than typing something like:

import Button from '../../../../../../../../components/Button';

Currently, I get import/no-extraneous-dependencies: 'react' should be listed in the project's dependencies. Run 'npm i -S react' to add it because the minimal package.json doesn't have dependencies and the rule doesn't care to look up higher in the directory tree.

If the rule allowed me to do the following, it would solve this issue:

'import/no-extraneous-dependencies': {
  moduleDirectories: ['./'] // or any other configuration that's most convenient for all
},

[vvo]

Jumping here, monorepo user, maybe we could:

• be able to provide a base dir (monorepo base)
• allow to provide an option to walk up to the base dir for packages.json

Or just a way to express some different packages.json to read. Dunno

[tibdex]

In some specific situations, enabling this rule to go up the hierarchy of package.json will make it less powerful by introducting some unexpected behaviour.

Let's say you have the following project structure:

├── package.json
├── file1.js
└── nested
    ├── package.json
    └── file2.js

with the following contents:

// package.json

{
  "name": "my-app",
  "dependencies": {
    "lodash": "3.10.1"
  }
}

// file1.js

const _ = require('lodash');

// Expected output: 3.10.1
console.log(_.VERSION);

// nested/package.json

{
  "name": "nested",
  "dependencies": {
    "babel-types": "6.16.0"
  }
}

// nested/file2.js

const _ = require('lodash');

// Expected output: 3.10.1
console.log(_.VERSION);

Then we have the following outputs:

node file1.js
> 3.10.1 // All good!

node nested/file2.js
> 4.16.4 // Wrong! Expected to also be 3.10.1: the version we depend on in the root package.json

Even though we declared the dependency on lodash 3.10.1 only once in the root package.json, it's not actually this version that is imported in nested/file2.js. It happens because babel-types depends on lodash ^4.2.0 and with the npm flat module structure, this new version of lodash ends up directly in nested/node_modules/lodash and "shadows" the version of lodash required by our root package.json.

However, if nested/package.json also declared an explicit dependency on lodash 3.10.1 then it will be this version that npm would write in nested/node_modules/lodash. Indeed, the lodash package required by babel-types would be under nested/node_modules/babel-types/node_modules/lodash and all would be fine.

Thus, the only sane situations in which it would be good to support nested package.json is when the deep ones doen't have any dependencies (dev included). Because then, nested/node_modules won't exist and lodash 3.10.1 will get picked up by the Node.js module resolution algorithm.

In the end, the use-cases of @nkt and @migueloller will work fine, but it's not generally safe - at least version wise - to support nested package.json.

[migueloller]

@tibdex, you make a fair point. Usually nested package.json are used in a monorepo or very large projects. In the first case, all dependencies will be declared in the nested package.json for the relevant package (meaning that for your example nested/package.json would explicitly declare the dependency of lodash as you mentioned). In the second case, there are usually no dependencies and the package.json simply serves as a way to avoid crazy relative imports (e.g., ../../../../../../../...) by taking advantage of the Node.js module resolution algorithm.

The issue you raise is a valid but I would argue that it's more of a Node.js issue that the developer should be aware of if they opt in to use nested package.json. In this case it would still make sense for ESLint to warn nested package.json by default. If the developer wants to opt in though, they shouldn't have to disable the entire rule to do this. Being able to modify it to support the nested package.json (much like Node.js module resolution algorithm does) seems OK to do. Would you agree?

[tibdex]

Agreed that it's more of a Node.js issue. And sure, being able to configure this rule to support nested package.json would be a good thing! I'm just saying that having this feature enabled by default could lead to some hard to find bugs for people that are not aware that the situation I explained above can happen.

[joefiorini]

Maybe a stupid question, but is this something someone could write a custom resolver (ie. nested-dependencies-resolver) to solve? Seems like that would solve the problem of opt-in vs. opt-out and reuses the existing infrastructure nicely. No?

[benoitgrelard]

Hi, currently in the process of converting about 20 repos to one monorepo using lerna and I just hit that limitation as well. So unfortunate that the only option for now is to disable that rule alltogether as it's really useful. Having a compatible option for nest packages would be a great addition.

[ramasilveyra]

Same problem here, with a project with a folder architecture with internals package.json files (like the example of the OP). Just thinking about how to solve this, until now without success:

1) Find the closest node_modules folder and then path.resolve(nodeModulesPath, '../package.json')? Nope, some projects doesn't has npm dependencies (and might be running eslint from global).
2) Find the closest package.json folder doesn't work with folders architecture with internal package.json (this is what is being done now).

Maybe the only solution is try if 1) exists and if not fallback to 2) ?

[ramasilveyra]

Looks like using node-app-root-path works for me, I will send a pr using this module.

[ramasilveyra]

Hello!

If you want you can try my fix with npm i -D ramasilveyra/eslint-plugin-import#fix-root-pkg-copy-dont-remove (https://github.com/ramasilveyra/eslint-plugin-import/tree/fix-root-pkg-copy-dont-remove, fork it and rebuild if you want :) ) until #685 gets a review & merge.
So far It works well on the projects where I had this problem.

Example of conf:

"import/no-extraneous-dependencies": ["error", { "packagePath": "./package.json" }]

[vvo]

very nice!

[jfmengels]

Could anybody with experience with monorepos look at https://github.com/benmosher/eslint-plugin-import/pull/685 (maybe try it out by following this) and give some feedback in that PR whether that would solve their problem? I'm not sure I see enough of the picture here.

[acusti]

@jfmengels Your comments are good, seems like you fully get it. At the same time, the extra option to specify a particular package.json path implemented by that PR will probably solve most users’ issues, so it would likely be worth starting with the simple solution before trying to build out the ideal or correct solution, which would be to check and resolve dependencies exactly as node and webpack do.

[ankurp]

When will this be merged?

[lotap]

Looks like #685 was merged in May.

In my case, I was able to solve the problem with:
"import/no-extraneous-dependencies": ["error", {"packageDir": "./"}],
(.eslintrc.js & package.json are in the working directory root)

Leaving this here because this is the page google led me to, but I would recommend checking out the docs for this rule.

[benmosher]

ah sweet, closing because #685 is indeed merged (thanks for the bump, @lotap)

[kopax]

@lotap I am using the version 2.8.0 and this didn't work for me. Why?

[kilpatty]

I am also using 2.8.0 with the same code that @lotap provided and getting the same error :/.

[lotap]

@kopax @kilpatty this is the code I currently use in my boilerplate:

'import/no-extraneous-dependencies': ['error', {'devDependencies': true, 'packageDir': path.join(__dirname, './')}],

That being said, I haven't worked on a project that uses multiple package.json files in a long time, so your mileage may vary

[tizmagik]

I just hit this today too. I think the only real way to fix is to enable an array of packageDirs to use, something like...

      'import/no-extraneous-dependencies': [
        'error',
        {
          devDependencies: true,
          packageDirs: [
            './', // this package
            '../../', // mono-repo root
          ],
        },
      ],

[rjhilgefort]

Just throwing a +1 for the solution that @tizmagik proposed. I'm hitting this problem in a monorepo architecture as well. I see that this issue is closed, should I created a new issue for this?

[malixsys]

@rjhilgefort this solved it for me:

"import/no-extraneous-dependencies": ["error", {"devDependencies": true, "packageDir": "./"}]

[medfreeman]

Here's a version that works in all contexts (lerna root + packages / vscode) with a root level .eslintrc.js config :

'import/no-extraneous-dependencies': context => [
      'error',
      {
        devDependencies: true,
        packageDir: [context.getFilename(), __dirname]
      }
    ]

The context.getFilename() gets you the nearest package.json(it's the same code that's used inside the rule source), and the __dirname gets you the root package.json

edit: doesn't cover sibling package dependencies though, since the require are declared using the fqpn @scope/package, for this to work the rule would need to accept package name exceptions

[i8ramin]

@medfreeman how do you define context in that example?

[ljharb]

You don’t; eslint provides it.

[jacobrask]

When trying to pass a function to the rule with both eslint 5.16.0 and 6.0.0-alpha I get

    Configuration for rule "import/no-extraneous-dependencies" is invalid:
    Severity should be one of the following: 0 = off, 1 = warn, 2 = error (you passed '[Function: import/no-extraneous-dependencies]').

[mudin]

Any solution for @jacobrask 's issue?

[mudin]

@medfreeman what version of eslint are you using? you didnt face any issue like @jacobrask's issue?

[medfreeman]

@mudin eslint 5.13.0
Not at all with this specific version, i'll try to test with 5.16.0 and see if it happens.

[mudin]

@medfreeman Thanks!

[tumidi]

Is there any news on @jacobrask issue?
I guess many are experiencing this.

[lx0n2acl]

Also wondering if this has a solution other than "off"

[MariuzM]

so we are on 2020 no further improvements on this apart just to switch the rule off?

[ljharb]

@MariuzM this was closed in 2017. If you would like further improvements, please file a new issue - there has been nothing actionable here for 3 years.