fix: allow packageDir and readPkgUp logic to play nice

[hulkish]

Fixes #1103, #1109

[hulkish]

@benmosher @ljharb This is a fix to what was missed in https://github.com/benmosher/eslint-plugin-import/pull/1085

[coveralls]

Coverage decreased (-1.09%) to 93.37% when pulling 65209e43e5ad651dd40031f00290c0b594dfef4c on hulkish:master into ebafcbf59ec9f653b2ac2a0156ca3bcba0a7cf57 on benmosher:master.

[coveralls]

Coverage increased (+2.8%) to 97.273% when pulling d09bf9ab0cb535ae0ace25a9266af78d3fb30058 on hulkish:master into ebafcbf59ec9f653b2ac2a0156ca3bcba0a7cf57 on benmosher:master.

[hulkish]

@benmosher @ljharb So, I think this test is actually incorrect.

With my fix, this test fails (succeeds where expected to fail). I think that particular test should instead be:

    test({
      code: 'import leftPad from "left-pad";',
      filename: path.join(packageDirMonoRepoWithNested, 'foo.js'),
      errors: [{
        ruleId: 'no-extraneous-dependencies',
        message: '\'left-pad\' should be listed in the project\'s dependencies. Run \'npm i -S left-pad\' to add it',
      }],
    }),

Because, react is available via readPkgUp per this test case. And, left-pad is not - unless you define packageDir: <path to monorepo root>.

That covers the "invalid" use case. To cover the valid one, I am thinking to add this as one of the valid tests:

    test({
      code: 'import leftPad from "left-pad";',
      filename: path.join(packageDirMonoRepoWithNested, 'foo.js'),
      options: [{packageDir: packageDirMonoRepoRoot}],
    }),

Make sense?

[hulkish]

I've updated this pr to reflect my last comment.

[dan-kez]

Just wanted to check in on this PR and see if there's any additional changes that need to be made for it to be merged.

[hulkish]

@dmk255 I think this is all - unless there's more req changes.

[hulkish]

@benmosher ok i think all concerns mentioned above are resolved, now. No more changes planned from me, unless requested.

[Tymek]

Might fix https://github.com/benmosher/eslint-plugin-import/issues/1109

[dvsekhvalnov]

Hey guys, any update on this one? Will it be merged/released soonish? :)

Thank you.

[dan-kez]

Just chiming in given that there's been a bit of time since the last comment - any chance this may be merged soonish?

Thank you!

[hulkish]

@benmosher any update on this? I don't think it's understood that the packageDir option is.currently broken without this.

[ljharb]

After thinking about this and discussing offline with @hulkish, it kind of seems to me like the current packageDir option is actually not useful.

Perhaps instead of "fixing" it with this new merging behavior, we should add a per-category packageDir option that has the new behavior? Something like this:

'import/no-extraneous-dependencies': [2, {
  dependencies: false, // or true
  optionalDependencies: ['pattern/*/1', 'pattern/**/2'],
  peerDependencies: {
    patterns: ['pattern/*/1', 'pattern/**/2'],
    additionalPackageDirs: ['../', 'somewhere else'],
  },
}]

Thoughts?

[hulkish]

@ljharb In your example snippet, I'm assuming patterns represents a glob to match against package names?

[ljharb]

@hulkish it would be the same value that's currently already supported in the schema i indicated under "optionalDependencies".

[dan-kez]

I'm a little confused as to the use of optionalDependencies and peerDependencies.patterns.

Also, as a user of a monorepo, my root package.json doesn't contain "peerDependencies" in the traditional sense but rather devDependencies that are shared across my packages.

I would find having the following valuable:

• Each leaf package has the ability to reference the root package.json to get access to the devDependencies. It would be ideal if this could be done without needing to make a custom .eslintrc.js in each leaf package but that's not a big deal.
• Each leaf package can resolve my symlinked packages that are included in my root node_modules. Right now I use https://github.com/Dreamscapes/eslint-import-resolver-lerna to help me do this. Otherwise eslint-plugin-import can't resolve my sibling packages and says they are not found.

Presently this PR fixes the first point. I've been able to use it by forking @hulkish's repo and importing that in my package.json. I'm entirely not sure what benefits @ljharb's proposal will grant though I definitely could be missing something.

[ljharb]

@dmk255 with my suggestion, you'd configure eslint to allow devDeps to be declared in the root, but regular deps to only be declared in the subproject's package.json.

[dan-kez]

@ljharb That sounds great - would you be able to give an example of the syntax you're considering to be able to do that? I don't immediately see how it fits into the example above.

[ljharb]

@dmk255

'import/no-extraneous-dependencies': [2, {
  dependencies: true,
  devDependencies: {
    additionalPackageDirs: ['.'],
  },
}]

This would require that deps be in the closest package.json, but devDeps could be there OR in the current dir's package.json.

[dvsekhvalnov]

Hey, let me ask as well. Here is our use case:

• we have monorepo, where root package.json defines 'baseline' dependencies (not devDeps) for all subprojects (let's say React15)

• each subproject can override baseline as it moving forward, (e.g. upgrade to React16).

• normally 90% of subprojects are fine with baseline and simply do not define anything new in dependencies

Will proposed solution be able to handle that case? (e.g. eslint to lookup dependencies in root package.json when linting subproject with empty deps).

P.S> we usually run eslint from root, like: eslint --ext .js,.jsx . to lint everything in single pass. And prefer to have single (common) .eslintrc on root level as well.

Thank you.

[ljharb]

@dvsekhvalnov yes, since it's "additional" - it merges - both this PR and my suggestion would address your use case.

[dvsekhvalnov]

Ok, sounds good. Any update on when any of proposed fixes can be released? :)

[hulkish]

@dvsekhvalnov I am going to refactor this pr a bit to meet the functionality described in https://github.com/benmosher/eslint-plugin-import/pull/1104#issuecomment-406074658

From there, I think the plan is to merge that functionality (assuming all is well). Since, currently the packageDir option doesn't quite fit the need.

[tizmagik]

What is remaining on this PR? Anything I can do to help get this merged?

[benmosher]

@ljharb @hulkish I am not a user of this rule. it's not clear to me based on discussion if this PR is in a useful releasable state. if it is useful in its current state and solves your problems, I'm fine to merge it.

[dvsekhvalnov]

@benmosher and all, i have use case for this change, can try to test if you can explain how i can get workable copy of non-released version.

[tizmagik]

@dvsekhvalnov I think you should be able to check out @hulkish branch and then link it in your project. Something like:

git clone https://github.com/hulkish/eslint-plugin-import
cd eslint-plugin-import
npm install
npm run prepublish
npm link
cd ~/my-project
npm link eslint-plugin-import

Then try running the linter. That should hopefully have it so that your project's eslint-plugin-import is actually linked to @hulkish 's version

[tylerlee]

I cloned @hulkish 's fork and this seems to fix my monorepo woes.

[benmosher]

I would merge but it's not clear to me why there are conflicts nor how to resolve them for the issues you are all facing. @hulkish are you able to resolve?

[DavideDaniel]

is the PR not in a testable state? I cloned @hulkish's fork and attempted seeing if this would resolve our monorepo lint issues particular to eslint-plugin-import and it did not... as I was about to post I realized that the pr comments are from 2018 but there are referenced from 2 days ago so the conflicts are probably real