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

Improve monorepo support for no-extraneous-dependencies rule #1016

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Feb 7, 2018

Solves #935

@hulkish hulkish changed the title Improve monorepo support no extraneous dependencies rule Improve monorepo support no-extraneous-dependencies rule Feb 7, 2018
@hulkish hulkish changed the title Improve monorepo support no-extraneous-dependencies rule Improve monorepo support for no-extraneous-dependencies rule Feb 7, 2018
@hulkish hulkish force-pushed the improve-monorepo-support-no-extraneous-dependencies-rule branch from 7e6c720 to 815abf0 Compare February 7, 2018 04:23
@coveralls
Copy link

coveralls commented Feb 7, 2018

Coverage Status

Coverage increased (+0.01%) to 96.504% when pulling eac714f on hulkish:improve-monorepo-support-no-extraneous-dependencies-rule into 4e37dbf on benmosher:master.

@hulkish hulkish force-pushed the improve-monorepo-support-no-extraneous-dependencies-rule branch 2 times, most recently from 7c150b2 to 980b087 Compare February 7, 2018 07:27
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, I'm still not sure this is a useful feature. Deps shouldn't be resolved from outside of a subpackage in a lerna-style monorepo.


if (!packageContent) {
return null
if (Object.prototype.hasOwnProperty.call(context.settings, 'import/paths')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use the has module for this.

@hulkish
Copy link
Contributor Author

hulkish commented Feb 7, 2018

@ljharb It's a documented approach in the readme for lerna: https://github.com/lerna/lerna/blob/master/README.md#common-devdependencies

Furthermore, node itself inherently resolves dependencies from parent node_modules dirs.

When you have... let's say 10-30 packages you're managing in a monorepo - then sure, not a big deal to manage dev deps on a per package basis.

However, at my job for example we manage a monorepo with well over 100 packages.

In this scenario; the task of managing dev deps for this volume of packages quickly outgrows what's practical.

While I agree that this is up for debate on a best practices basis - I'm not sure how valueable it is to have this flexibility prevented at the level of an eslint plugin.

Personally, I've found that hoisting all of these dev deps to the root of monorepo brings a significant benefit and gained sanity.

@tizmagik
Copy link
Contributor

Thank you for this PR @hulkish -- this has been a pain point for us so far. Hopefully this gets merged and released 🙏

@hulkish
Copy link
Contributor Author

hulkish commented Mar 4, 2018

@ljharb hoping this can land.. is there anything I can improve to allow it?

@gotbahn
Copy link

gotbahn commented Mar 18, 2018

Thanks for PR. We also reach this problem in monorepo. There even extra plugin in the wild https://github.com/Dreamscapes/eslint-import-resolver-lerna which partially solves it.
But have a solution in eslint-plugin-import is definitely better.

@dan-kez
Copy link

dan-kez commented Mar 18, 2018

I've also run into this issue. I would love to see this merged.

Copy link
Member

@benmosher benmosher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why add a new top-level setting (import/paths) instead of expanding the existing packageDir rule option to be either a string or a list of strings?

@hulkish
Copy link
Contributor Author

hulkish commented Mar 24, 2018

@benmosher hmm, sure - but, how would you configure packageDir in your .eslintrc?

@dan-kez
Copy link

dan-kez commented Mar 27, 2018

@hulkish I believe the relevant PR is #685

@benmosher
Copy link
Member

@evocateur
Copy link

I just turn off import/no-extraneous-dependencies for any test files in the monorepo with an overrides key. import/no-unresolved already handles "is it installed somewhere", so extraneous-ness in test files seems beside the point. ${ROOT}/.eslintrc.yaml looks like this:

---
  extends: airbnb
  root: true
  overrides:
    - files: "**/*{-,.}{test,stories}.js"
      rules:
        # devDependencies are all in the root
        import/no-extraneous-dependencies: off

@hulkish
Copy link
Contributor Author

hulkish commented Apr 17, 2018

Seems like this is just going to be an endless debate of correctness vs practicality.

I'll just fork and publish this as a different package.

@hulkish hulkish closed this Apr 17, 2018
@ljharb
Copy link
Member

ljharb commented Apr 17, 2018

You’re welcome to do that, but this should stay open.

@hulkish hulkish force-pushed the improve-monorepo-support-no-extraneous-dependencies-rule branch from 77d4423 to 2780d7c Compare April 19, 2018 00:21
@hulkish
Copy link
Contributor Author

hulkish commented Apr 19, 2018

@benmosher I actually had this work changed to use packageDir already. Just updated this pr with that code.

I don't care if we use my pr or yours, tbh. @ljharb Whichever you guys think is best.

@hulkish
Copy link
Contributor Author

hulkish commented Apr 19, 2018

Just wondering... what's holding this up? What needs to be done to get this merged?

@benmosher
Copy link
Member

busy week at work, but it's top of my queue for this project when I get time

@hulkish
Copy link
Contributor Author

hulkish commented Apr 20, 2018

@benmosher thanks!

@benmosher
Copy link
Member

@hulkish fyi, merged mine because I had fixed up and added a few more tests, LMK if I missed anything though

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

Successfully merging this pull request may close these issues.

None yet

8 participants