fix: allow packageDir and readPkgUp logic to play nice #1104

hulkish · 2018-05-17T21:24:59Z

hulkish · 2018-05-17T21:25:56Z

@benmosher @ljharb This is a fix to what was missed in #1085

coveralls · 2018-05-17T21:36:20Z

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

coveralls · 2018-05-17T21:36:20Z

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

ljharb

Please provide some tests that would fail without this change?

benmosher

agreed with @ljharb, need tests that would otherwise fail without this so that we know it's working. ~~though it is good to know (though a bit weird?) that it doesn't impact any existing tests.~~ wasn't paying close enough attention, it did break existing tests, so the real question is whether it is the new ones for the last release

hulkish · 2018-05-18T15:56:56Z

@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 · 2018-05-18T16:07:39Z

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

ljharb · 2018-05-21T03:12:36Z

tests/src/rules/no-extraneous-dependencies.js

@@ -243,24 +248,23 @@ ruleTester.run('no-extraneous-dependencies', rule, {
      options: [{packageDir: packageDirMonoRepoWithNested}],
      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",
+        message: '\'left-pad\' should be listed in the project\'s dependencies. Run \'npm i -S left-pad\' to add it',


are all these quotes changes necessary?

@ljharb Yea, oddly enough - they were actually failing lint for me...Strings must use singlequote. (quotes). So, I just made it consistent with the rest of the file.

hm, strange. the linter should be ignoring cases where it minimizes escaping backslashes.

yea.. shouldn't it prefer backticks?

Only if there's interpolations.

This still seems like it should be reverted.

@ljharb https://github.com/hulkish/eslint-plugin-import/blob/2e41085228ddd026ac7735eff36247b7f4d53d61/tests/src/rules/no-extraneous-dependencies.js as u can see, this change is consistent with the rest of the file. It actually fails lint without this change for me with the existing (in master).

actually, i can see now this was corrected in master, so ill revert.

ljharb · 2018-05-21T03:12:54Z

tests/src/rules/no-extraneous-dependencies.js

      }],
    }),
    test({
-      code: 'import react from "react";',


if this test case is supposed to pass, let's move it to "valid"

I solved this by:

keeping this test. since it's also good to be checking

added exact original test to valid, here https://github.com/benmosher/eslint-plugin-import/pull/1104/files#diff-05140578b7a943405919d8a058934d1dR97

hulkish · 2018-05-21T03:34:23Z

src/rules/no-extraneous-dependencies.js

      context.report({
        message: 'The package.json file could not be parsed: ' + e.message,
        loc: { line: 0, column: 0 },
      })
+    } else {
+      throw e


@ljharb Thought I should make note of this.. We were swallowing "all else" errors before. I thought that was a mistake.

Throwing these errors will crash eslint - I'm not sure if that's better than silently logging them.

Well, what if its a permission error? Shouldn't that crash eslint?

it perhaps should warn on the line, but not crash eslint altogether

What way should i do that? I'm thinking:

} else { context.report({ message: e.message, loc: { line: 0, column: 0 }, }) }

Atlhough, I think this is an error related to the package.json loading/parsing in this instance, and should probably somehow be attached to that instead?

Ok, this is updated. @ljharb, @benmosher anything else?

dan-kez · 2018-06-06T17:10:35Z

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.

…ated filename instead of context.getFilename()

hulkish · 2018-06-06T18:21:36Z

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

hulkish · 2018-06-08T21:21:51Z

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

Tymek · 2018-06-13T05:55:53Z

Might fix #1109

dvsekhvalnov · 2018-06-21T12:28:24Z

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

Thank you.

dan-kez · 2018-07-10T19:47:57Z

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 · 2018-07-12T12:28:41Z

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

ljharb · 2018-07-18T21:07:04Z

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 · 2018-07-18T21:43:36Z

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

ljharb · 2018-07-18T23:38:32Z

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

dan-kez · 2018-07-19T00:02:19Z

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 · 2018-07-19T00:12:39Z

@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 · 2018-07-19T00:31:44Z

@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 · 2018-07-19T00:40:19Z

@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 · 2018-07-26T18:40:26Z

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 · 2018-07-26T18:45:06Z

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

dvsekhvalnov · 2018-07-26T19:23:27Z

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

hulkish · 2018-07-26T19:55:53Z

@dvsekhvalnov I am going to refactor this pr a bit to meet the functionality described in
#1104 (comment)

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 · 2018-09-26T19:23:35Z

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

@ljharb

dismissing my review because it seems like @ljharb and @hulkish are on the same page

benmosher · 2018-10-11T11:31:59Z

@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 · 2018-10-11T14:14:19Z

@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 · 2018-10-18T15:59:15Z

@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 · 2018-11-20T03:58:20Z

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

benmosher · 2018-12-04T19:53:39Z

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 · 2020-02-11T03:04:40Z

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

ljharb · 2020-06-05T22:48:39Z

ping @hulkish - any interest in rebasing this PR?

ljharb previously requested changes May 18, 2018

View reviewed changes

benmosher previously requested changes May 18, 2018

View reviewed changes

hulkish force-pushed the master branch from 65209e4 to bbb672a Compare May 18, 2018 16:07

hulkish force-pushed the master branch 11 times, most recently from 243487a to d633420 Compare May 19, 2018 16:09

ljharb reviewed May 21, 2018

View reviewed changes

Steven Hargrove added 2 commits May 20, 2018 23:24

fix: allow packageDir and readPkgUp logic to play nice

da06b66

moved related test which was previously invalid - is now valid

bb15b05

hulkish force-pushed the master branch from 72defe2 to bb15b05 Compare May 21, 2018 03:25

hulkish commented May 21, 2018

View reviewed changes

hulkish mentioned this pull request Jun 5, 2018

no-extraneous-dependencies: multiple packageDir(s) #1085

Merged

fix: report error instead of throw

6421a0e

hulkish force-pushed the master branch from 15812c9 to 6421a0e Compare June 6, 2018 01:12

improve error reporting for package.json searching, Use already gener…

2e41085

…ated filename instead of context.getFilename()

hulkish force-pushed the master branch from b207b7f to d09bf9a Compare June 7, 2018 00:04

benmosher self-requested a review October 11, 2018 11:29

This was referenced Feb 8, 2020

[import/no-extraneous-dependencies]: better support for monorepos #1650

Closed

test: enable import/no-extraneous-dependencies conventional-changelog/commitlint#983

Merged

ljharb marked this pull request as draft February 1, 2021 21:04

fix: allow packageDir and readPkgUp logic to play nice #1104

Are you sure you want to change the base?

fix: allow packageDir and readPkgUp logic to play nice #1104

Conversation

hulkish commented May 17, 2018 • edited Loading

hulkish commented May 17, 2018

coveralls commented May 17, 2018

coveralls commented May 17, 2018 • edited Loading

ljharb left a comment

Choose a reason for hiding this comment

benmosher left a comment • edited Loading

Choose a reason for hiding this comment

hulkish commented May 18, 2018 • edited by ljharb Loading

hulkish commented May 18, 2018

Choose a reason for hiding this comment

hulkish May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish Jun 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hulkish May 21, 2018 • edited Loading

Choose a reason for hiding this comment

hulkish Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

dan-kez commented Jun 6, 2018

hulkish commented Jun 6, 2018

hulkish commented Jun 8, 2018 • edited Loading

Tymek commented Jun 13, 2018

dvsekhvalnov commented Jun 21, 2018

dan-kez commented Jul 10, 2018

hulkish commented Jul 12, 2018 • edited Loading

ljharb commented Jul 18, 2018

hulkish commented Jul 18, 2018 • edited Loading

ljharb commented Jul 18, 2018

dan-kez commented Jul 19, 2018

ljharb commented Jul 19, 2018

dan-kez commented Jul 19, 2018

ljharb commented Jul 19, 2018

dvsekhvalnov commented Jul 26, 2018 • edited Loading

ljharb commented Jul 26, 2018

dvsekhvalnov commented Jul 26, 2018 • edited Loading

hulkish commented Jul 26, 2018

tizmagik commented Sep 26, 2018

benmosher commented Oct 11, 2018

dvsekhvalnov commented Oct 11, 2018

tizmagik commented Oct 18, 2018

tylerlee commented Nov 20, 2018

benmosher commented Dec 4, 2018

DavideDaniel commented Feb 11, 2020 • edited Loading

ljharb commented Jun 5, 2020

hulkish commented May 17, 2018 •

edited

Loading

coveralls commented May 17, 2018 •

edited

Loading

benmosher left a comment •

edited

Loading

hulkish commented May 18, 2018 •

edited by ljharb

Loading

hulkish May 21, 2018 •

edited

Loading

hulkish Jun 6, 2018 •

edited

Loading

hulkish May 21, 2018 •

edited

Loading

hulkish Jun 5, 2018 •

edited

Loading

hulkish commented Jun 8, 2018 •

edited

Loading

hulkish commented Jul 12, 2018 •

edited

Loading

hulkish commented Jul 18, 2018 •

edited

Loading

dvsekhvalnov commented Jul 26, 2018 •

edited

Loading

dvsekhvalnov commented Jul 26, 2018 •

edited

Loading

DavideDaniel commented Feb 11, 2020 •

edited

Loading