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

no-extraneous-dependencies only targets one package.json #935

Closed
Robinfr opened this issue Sep 28, 2017 · 27 comments
Closed

no-extraneous-dependencies only targets one package.json #935

Robinfr opened this issue Sep 28, 2017 · 27 comments

Comments

@Robinfr
Copy link

Robinfr commented Sep 28, 2017

In the Node.js environment, a call using require() will walk up the tree to find the dependency, this means that a dependency that is required can be in any node_modules folder up the tree. Currently it only looks at one folder.

Could this be adjusted?

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017

Also related to #458 which allows for a single package folder to be set. But in this case, I have multiple..

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

It walks up the tree until it finds package.json, then it stops.

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017

Yes, it shouldn't. It should keep walking and find all of the package.json files and merge them for this rule. This is how Node does it also.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

Can you provide a concrete example of what problem you need solved?

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017

Sure, I have a Lerna monorepo which has several packages that contain tests that make use of Chai and Sinon. Since I don't want to install those in all packages, I installed it at the root level. This works perfectly well, except that this rule is complaining.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

That's not how a lerna monorepo is supposed to be organized tho; lerna bootstrap is supposed to install all those as dev deps via npm link. Each package in that multirepo is supposed to be able to function as if it was the root of the repo.

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017 via email

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

The babel repo isn't the best example for the best way to set up a lerna monorepo :-)

Regardless, does that mean you're running the linter per-package?

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017 via email

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

Gotcha.

While I think that organizing your repo this way is a very bad practice, I agree that since node does it, this plugin should support it - at least up to the dir in which eslint is run (traversing upwards from there could hide bugs if there's /package.json or ~/package.json)

@Robinfr
Copy link
Author

Robinfr commented Sep 28, 2017 via email

@ljharb
Copy link
Member

ljharb commented Oct 3, 2017

I'd suggest making each dir inside packages have its own package.json, have root: true in its .eslintrc, and be 100% self-containing - in other words, I should be able to cd into each package dir, and everything should function identically as if i had each package dir in a separate repo.

Then, the root package.json would contain lerna commands in "scripts" to automate common tasks across packages.

@Hypnosphi
Copy link
Contributor

That's not how a lerna monorepo is supposed to be organized

Lerna docs disagree: https://github.com/lerna/lerna#common-devdependencies

@ljharb
Copy link
Member

ljharb commented Nov 16, 2017

I'm using "supposed" here based on community best practices; if Lerna documentation deviates from that, Lerna is wrong.

@Robinfr
Copy link
Author

Robinfr commented Nov 17, 2017

I guess you can make both work by having Lerna hoist certain dev dependencies. This will make each module still self-contained, but you'll also run into this problem potentially.

@ljharb
Copy link
Member

ljharb commented Nov 17, 2017

Yes, lerna should only hoist deps when the versions are the same; if it doesn’t work this way now, it’s a bug.

@Robinfr
Copy link
Author

Robinfr commented Dec 11, 2017

Any update on this? I would like to use this rule, but I can't until it works properly.

@evocateur
Copy link

lerna bootstrap --hoist makes it faster, but it doesn't strictly preclude listing the dev deps in each package. It's mostly a performance optimization that takes advantage of the node resolution algorithm. It's not meant to be an excuse for implicit dependencies (dev or otherwise).

@hulkish
Copy link
Contributor

hulkish commented Feb 7, 2018

+1

i feel like this could be solved by providing a way to configure lookup paths for a package.json in addition to the current behavior of seeking first found package.json relative to the context of the file being linted.

@penx
Copy link

penx commented Apr 9, 2018

@ljharb hopefully this is a concrete example of why you would want some dependencies to be at the root of a monorepo.

I have a monorepo, one of the packages is a dev tool to generate documentation that is a devDependency of a lot of other packages. I only expect to run this tool with the package checked out under the monorepo structure.

I can either:

  1. add the tool as a devDependency to every package that needs it
  2. add the tool as a devDependency at the root of the monorepo

Unfortunately, if I do (1), then make a code change to the tool and then run lerna publish, lerna will increment the devDepencency version of the tool in every package that uses it and then republish a new version of all these packages.

I only want to republish packages when dependencies that are relevant to users outside of the monorepo structure change, so I am considering putting devDependencies like these on the monorepo's root package.json.

Project specific issue: govuk-react/govuk-react#175 (comment)

Alternatively, it would be nice for no-extraneous-dependencies config to accept an array of whitelisted dependencies so that should not error if missing.

@Hypnosphi
Copy link
Contributor

it would be nice for no-extraneous-dependencies config to accept an array of whitelisted dependencies so that should not error if missing

https://github.com/benmosher/eslint-plugin-import#importcore-modules

@ljharb
Copy link
Member

ljharb commented Apr 9, 2018

@penx tbh that sounds like a flaw (not the only one) in lerna publish :-/

@evocateur
Copy link

@penx Just turn off brittle import/no-extraneous-dependencies for test files, and move all the devDependencies to the root. If you're using lerna, you might as well recognize that you don't have to pretend your leaf packages are self-sufficient (why bother with a multi-package repository otherwise?).

---
  # repo-root .eslintrc.yaml
  root: true
  plugins:
    # or just extend airbnb ¯\_(ツ)_/¯
    - import
  rules:
    import/no-extraneous-dependencies: error

  # ↓ this is the important part
  overrides:
    - files: "**/*{-,.}{test,stories}.js"
      rules:
        # devDependencies are all in the root
        import/no-extraneous-dependencies: off

import/no-unresolved is still able to ensure you actually have those dev dependencies installed in the root, and is more than sufficient for the task.

Each package in that [multi-package repository] is supposed to be able to function as if it was the root of the repo.

@ljharb Citation needed?

@ljharb
Copy link
Member

ljharb commented Apr 10, 2018

The reasons to "bother" with a multi-package repo aren't because you don't want self-sufficient leaf packages, they're because you can make related changes in multiple packages in the same PR, and more easily run tests against multiple packages at once.

@evocateur
Copy link

Self-sufficient leaf packages aren’t at all necessary to accomplish those two goals, they’re just a stylistic preference. Repeating the same devDependencies and scripts and configuration &c in every single leaf package is a mind-numbing maintenance burden.

@penx
Copy link

penx commented Apr 17, 2018

I think having a config option for no-extraneous-dependencies to support multiple parent directories with package.json files would be useful, though perhaps disabled by default.

I can certainly see the argument for making a package in a monorepo self sufficient so that a developer who only wants to work on one package can just cd to that package and yarn/npm install. For devDependencies that install a CLI tool it is useful to have this on the path when running scripts in a sub project (I'm not sure if .bin from a parent node_modules folder is included in the npm script resolution path, I don't think it is).

However I agree with @evocateur that this is a stylistic preference. I also find the maintenance of repeated devDependencies a pain, and can see the benefit of certain devDependencies (that are only used by files that assume they are being run as part of the monorepo) to be in the root package.json.

@hulkish
Copy link
Contributor

hulkish commented Apr 17, 2018

@ljharb I view this as a debate of correctness vs efficiency/what's practical.

I believe I have a very valid use case for needing to hoist dev dependencies to monorepo root. I don't think being "correct" is worth the trade of a poorer dev environment. Not to.mention, the difference also has performance implications - Using leaf packages for dev dependencies being far less work for installs.

I also think it's inappropriate to suggest that it's not a valid concern all because you personally find it ugly.

penx added a commit to govuk-react/govuk-react that referenced this issue Apr 17, 2018
Fixes #175 (hopefully)

Updates to a devDependency for a package causes `lerna publish` to republish that package. This has a knock on effect throughout our repo meaning a single change to one component can result in every component being republished. We don't want this to happen so are including most devDependencies at the project root (where we expect these dependencies are only used when using the package as part of a monorepo).

Exceptions are where a component has specific peerDependencies and we want to also represent that in the devDependencies.

Related: 

lerna/lerna#1269
lerna/lerna#1345
import-js/eslint-plugin-import#935
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants