Add support to specify the package.json for no-extraneous-dependencies rule #685

ramasilveyra · 2016-12-09T00:01:38Z

Add support for nested package.json, projects with a folder architecture with internals package.json files (see #458).

Discussion

Actually change the behavior of use the closest package.json to use the root package.json breaks the rule when you want to check the dependencies from an internal package.json, so I have this two paths in mind:

Per default get the dependencies from the closest package.json and if the option getPkgFromRoot is true get the dependencies from the root package.json file.
Add an option like pkgPath to the rule to be able to specify the package.json path and if is null just find the closest package.json like the last version. IMHO this looks better, it's more flexible.

What do you think about this?

12/24/2016 update

I choose the 2. path, the option is called packagePath.

01/29/2017 update

Commit name changed to Add support to specify the package.json as this solution doesnt fully solve all the cases of #458 Look: #685 (review)

coveralls · 2016-12-24T06:58:51Z

Coverage remained the same at 94.863% when pulling a945976 on ramasilveyra:fix-root-pkg into c975742 on benmosher:master.

ramasilveyra · 2016-12-24T07:04:06Z

@ljharb @benmosher I found time to finish this pr 😀, It's ready for a review.

I choose the 2. solution. The option that allows you to specify the package.json path is packagePath.

coveralls · 2016-12-24T07:09:07Z

Coverage remained the same at 94.863% when pulling e2fb13b on ramasilveyra:fix-root-pkg into c975742 on benmosher:master.

coveralls · 2016-12-24T07:12:56Z

Coverage remained the same at 94.863% when pulling e2fb13b on ramasilveyra:fix-root-pkg into c975742 on benmosher:master.

ramasilveyra · 2016-12-24T07:15:23Z

It looks like the appveyor failing is not related to this pr: #695 (comment).

coveralls · 2017-01-20T00:23:01Z

Coverage remained the same at 94.863% when pulling 2a0410d on ramasilveyra:fix-root-pkg into bc817f1 on benmosher:master.

coveralls · 2017-01-20T00:23:01Z

Coverage remained the same at 94.863% when pulling 2a0410d on ramasilveyra:fix-root-pkg into bc817f1 on benmosher:master.

coveralls · 2017-01-20T00:23:01Z

Coverage remained the same at 94.863% when pulling 2a0410d on ramasilveyra:fix-root-pkg into bc817f1 on benmosher:master.

jfmengels

Hi @ramasilveyra! Thanks a lot for the PR! (and sorry for the late reply)

This does not merge the dependencies from multiple packages. So from what I understand, if in your monorepo:

you define all the dependencies in the nested package.json, this works with the (old and) default behaviour
You define all the dependencies in the root package.json, this works by overriding the packagePath option
You define dependencies in multiple package.json files, and this will not work.

In the last case, maybe we could fetch both the package.json linked in packagePath and the closest package.json and merge them?

I'm fine with trying out this solution and trying my proposal (or something else) later if people find errors.

jfmengels · 2017-01-21T13:55:47Z

docs/rules/no-extraneous-dependencies.md

@@ -27,6 +27,12 @@ You can also use an array of globs instead of literal booleans:

 When using an array of globs, the setting will be activated if the name of the file being linted matches a single glob in the array.

+Also there is one more option called `packagePath`, this option is to specify the path of the package.json.


We should indicate that this is relative to the current working directory (which is what I'm understanding from the code).

jfmengels · 2017-01-21T14:15:14Z

src/rules/no-extraneous-dependencies.js

-    const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false})
-    if (!pkg || !pkg.pkg) {
+    const pkg = packagePath
+      ? JSON.parse(fs.readFileSync(packagePath, 'utf8'))


If the file is not found, the rule should report an error indicating that the file could not be found.
If the file could not be parsed, the rule should report an error indicating that too.
Please add tests for those cases :)

... } catch (e) { if (packagePath && e.code === 'ENOENT') { context.report({ message: 'The package.json file could not be find.' }) } if (packagePath && e instanceof SyntaxError) { context.report({ message: 'The package.json file could not be parsed due to syntax errors.' }) } return null } ...

doesn't work because context.report( needs a loc or node, cc: http://eslint.org/docs/developer-guide/working-with-rules#contextreport

But maybe I can throw the error in those cases? Or log in console with console.error?

You could report the error at the beginning of the file, like here.

I would advise against throwing an error, as ESLint doesn't properly handle them, the process will crash and the error will be hard to understand. By reporting it this way, the user will get an error for pretty much every file, but it will be much clearer and visible than a wondering console.error.

if (packagePath && err.code === 'ENOENT') { // find --> found context.report({ message: 'The package.json file could not be found.' }) } if (packagePath && err instanceof SyntaxError) { // Change the error message to this and append the error message context.report({ message: 'The package.json file could not be parsed: ' + err.message}) }

Good hack! You are right, thanks for your comments and explanation!

Now this change request is fixed.

jfmengels · 2017-01-21T14:15:44Z

src/rules/no-extraneous-dependencies.js

  try {
-    const pkg = readPkgUp.sync({cwd: context.getFilename(), normalize: false})
-    if (!pkg || !pkg.pkg) {
+    const pkg = packagePath


Rename pkg to packageContent

coveralls · 2017-01-29T20:52:05Z

Coverage decreased (-0.003%) to 94.91% when pulling 2b31ca7 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T20:52:05Z

Coverage decreased (-0.003%) to 94.91% when pulling 2b31ca7 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T20:52:05Z

Coverage decreased (-0.003%) to 94.91% when pulling 2b31ca7 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T21:08:15Z

Coverage decreased (-0.003%) to 94.91% when pulling 708f086 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

ramasilveyra · 2017-01-29T21:32:49Z

PR title and commit title changed to Add support to specify the package.json since what @jfmengels says it's right and this doesn't fully solve #685 .

jfmengels

Almost there! Can you add a test for when the package.json is not found?

coveralls · 2017-01-29T21:34:51Z

Coverage increased (+0.07%) to 94.977% when pulling 57d80f9 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T21:34:51Z

Coverage increased (+0.07%) to 94.977% when pulling 57d80f9 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

ramasilveyra · 2017-01-29T21:37:40Z

@jfmengels test case for when the package.json is not found added

jfmengels · 2017-01-29T21:41:13Z

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

+    }),
+    test({
+      code: 'import "not-a-dependency"',
+      options: [{packagePath: path.join(__dirname, '../../../doesnt-exist.json')}],


This kind of highlights a weirdness of this rule: you can specify a file that is not named package.json, which I don't think makes sense. I'm wondering whether packagePath should not rather be packageDir and be the path to the folder containing package.json.
i.e. [{packagePath: path.join(__dirname, '../../../package.json')}] --> [{packageDir: path.join(__dirname, '../../../')}]

What do you think? 🤔

Brainstorming: The name of this property could be named also as manifestFilePath. https://en.wikipedia.org/wiki/Manifest_file

But yes, packageDir seems better! I will do that.

Yes, packageDir seems better, less confusing for npm users :)

coveralls · 2017-01-29T21:41:52Z

Coverage increased (+0.1%) to 95.034% when pulling 83489b8 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T22:40:36Z

Coverage increased (+0.1%) to 95.034% when pulling f456871 on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

coveralls · 2017-01-29T22:55:46Z

Coverage increased (+0.1%) to 95.048% when pulling d6586bd on ramasilveyra:fix-root-pkg into 790dd66 on benmosher:master.

johnsardine · 2017-02-01T15:14:44Z

Can we specify if it should look also in bower_components?

ljharb · 2017-02-01T16:34:05Z

No, bower should not be used by anyone, and I would be very against supporting it.

johnsardine · 2017-02-01T16:39:29Z

@ljharb I understand. Can you share some information about why you think that? I'm just curious and looking to learn more. Thanks

ljharb · 2017-02-01T16:57:02Z

@johnsardine to start: 1) there never was a reason for bower to exist, npm has always been also for frontend code. 2) bower isn't a package manager, it's a CLI over curl-to-github, and one should use a proper package manager (that allows for nested dependencies when it is needed). 3) anything on bower that isn't on npm isn't worth using, and there'll be a plethora of better alternatives that are on npm.

jfmengels

LGTM! (sorry about the delay)

@ljharb Since you've already reviewed this, do you mind reviewing and merging/approving?

ramasilveyra · 2017-02-16T22:52:19Z

FYI @ljharb @jfmengels I just resolved the conflict on the CHANGELOG.md (rebase).

ljharb

I don't see any major problems, but I'd prefer to defer to @benmosher here.

coveralls · 2017-02-17T00:48:50Z

Coverage increased (+0.1%) to 95.051% when pulling 3cb927d on ramasilveyra:fix-root-pkg into b5a962f on benmosher:master.

coveralls · 2017-02-17T00:48:50Z

Coverage increased (+0.1%) to 95.051% when pulling 3cb927d on ramasilveyra:fix-root-pkg into b5a962f on benmosher:master.

coveralls · 2017-02-17T00:48:50Z

Coverage increased (+0.1%) to 95.051% when pulling 3cb927d on ramasilveyra:fix-root-pkg into b5a962f on benmosher:master.

0xR · 2017-05-01T12:10:03Z

It seems this PR is waiting on additional review which has not been performed for several months. Can we just merge this? If any major issues show up it will reported here and we can fix them or revert.

benmosher · 2017-05-18T12:40:42Z

👍🏻 thanks for review @jfmengels

benmosher

LGTM

ramasilveyra force-pushed the fix-root-pkg branch from a32bde0 to a945976 Compare December 24, 2016 06:54

ramasilveyra force-pushed the fix-root-pkg branch 2 times, most recently from efc2efa to e2fb13b Compare December 24, 2016 07:05

ramasilveyra mentioned this pull request Jan 6, 2017

no-extraneous-dependencies doesn't support nested package.json #458

Closed

ramasilveyra force-pushed the fix-root-pkg branch from e2fb13b to 2a0410d Compare January 19, 2017 22:22

jfmengels requested changes Jan 21, 2017

View reviewed changes

ramasilveyra force-pushed the fix-root-pkg branch from 2a0410d to 2b31ca7 Compare January 29, 2017 20:46

ramasilveyra force-pushed the fix-root-pkg branch from 2b31ca7 to 708f086 Compare January 29, 2017 21:02

ramasilveyra force-pushed the fix-root-pkg branch from 708f086 to 57d80f9 Compare January 29, 2017 21:28

ramasilveyra changed the title ~~Add support for nested package.json to no-extraneous-dependencies~~ Add support to specify the package.json to no-extraneous-dependencies Jan 29, 2017

jfmengels requested changes Jan 29, 2017

View reviewed changes

ramasilveyra force-pushed the fix-root-pkg branch from 57d80f9 to 83489b8 Compare January 29, 2017 21:35

jfmengels reviewed Jan 29, 2017

View reviewed changes

ramasilveyra force-pushed the fix-root-pkg branch from f456871 to d6586bd Compare January 29, 2017 22:50

ramasilveyra changed the title ~~Add support to specify the package.json to no-extraneous-dependencies~~ Add support to specify the package.json for no-extraneous-dependencies rule Feb 1, 2017

frenzzy mentioned this pull request Feb 2, 2017

Fix ESLint warnings kriasoft/react-firebase-starter#192

Open

jfmengels approved these changes Feb 16, 2017

View reviewed changes

Add support to specify the package.json

3cb927d

ramasilveyra force-pushed the fix-root-pkg branch from d6586bd to 3cb927d Compare February 16, 2017 22:50

ljharb reviewed Feb 16, 2017

View reviewed changes

ljharb requested a review from benmosher February 16, 2017 23:52

timc3 mentioned this pull request May 16, 2017

package.json not being found vim-syntastic/syntastic#2016

Closed

benmosher merged commit 0fb592e into import-js:master May 18, 2017

benmosher reviewed May 18, 2017

View reviewed changes

ramasilveyra deleted the fix-root-pkg branch May 18, 2017 14:44

renovate-bot mentioned this pull request May 24, 2017

Update dependency eslint-plugin-import to version 2.3.0 renovatebot/renovate#217

Merged

gantoine mentioned this pull request Jul 2, 2017

Flow-bin checking fails to search parent package.json flow-typed/flow-typed#999

Closed

efueger mentioned this pull request Aug 11, 2017

Upgrade eslint-plugin-import to 2.7.x codeclimate/codeclimate-eslint#318

Closed

dan-kez mentioned this pull request Mar 27, 2018

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

Closed

This was referenced Apr 15, 2018

Update eslint-plugin-import to 2.12.0 sbardian/DebtTrackerAPI#25

Merged

Update dependency eslint-plugin-import to v2.11.0 Urigo/tortilla#68

Merged

		@@ -27,6 +27,12 @@ You can also use an array of globs instead of literal booleans:

		When using an array of globs, the setting will be activated if the name of the file being linted matches a single glob in the array.

		Also there is one more option called `packagePath`, this option is to specify the path of the package.json.

Add support to specify the package.json for no-extraneous-dependencies rule #685

Add support to specify the package.json for no-extraneous-dependencies rule #685

Conversation

ramasilveyra commented Dec 9, 2016 • edited

Discussion

12/24/2016 update

01/29/2017 update

coveralls commented Dec 24, 2016 • edited

ramasilveyra commented Dec 24, 2016 • edited

coveralls commented Dec 24, 2016 • edited

coveralls commented Dec 24, 2016 • edited

ramasilveyra commented Dec 24, 2016

coveralls commented Jan 20, 2017 • edited

coveralls commented Jan 20, 2017

coveralls commented Jan 20, 2017

jfmengels left a 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

ramasilveyra Jan 29, 2017 • edited

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

coveralls commented Jan 29, 2017

coveralls commented Jan 29, 2017

coveralls commented Jan 29, 2017 • edited

coveralls commented Jan 29, 2017 • edited

ramasilveyra commented Jan 29, 2017

jfmengels left a comment

Choose a reason for hiding this comment

coveralls commented Jan 29, 2017

coveralls commented Jan 29, 2017

ramasilveyra commented Jan 29, 2017

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

coveralls commented Jan 29, 2017

coveralls commented Jan 29, 2017 • edited

coveralls commented Jan 29, 2017 • edited

johnsardine commented Feb 1, 2017

ljharb commented Feb 1, 2017

johnsardine commented Feb 1, 2017

ljharb commented Feb 1, 2017

jfmengels left a comment

Choose a reason for hiding this comment

ramasilveyra commented Feb 16, 2017

ljharb left a comment

Choose a reason for hiding this comment

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017

coveralls commented Feb 17, 2017 • edited

0xR commented May 1, 2017

benmosher commented May 18, 2017

benmosher left a comment

Choose a reason for hiding this comment

ramasilveyra commented Dec 9, 2016 •

edited

coveralls commented Dec 24, 2016 •

edited

ramasilveyra commented Dec 24, 2016 •

edited

coveralls commented Dec 24, 2016 •

edited

coveralls commented Dec 24, 2016 •

edited

coveralls commented Jan 20, 2017 •

edited

ramasilveyra Jan 29, 2017 •

edited

coveralls commented Jan 29, 2017 •

edited

coveralls commented Jan 29, 2017 •

edited

coveralls commented Jan 29, 2017 •

edited

coveralls commented Jan 29, 2017 •

edited

coveralls commented Feb 17, 2017 •

edited