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

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

Merged
merged 1 commit into from
May 18, 2017

Conversation

ramasilveyra
Copy link
Contributor

@ramasilveyra ramasilveyra commented Dec 9, 2016

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:

  1. 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.

  2. 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
Copy link

coveralls commented Dec 24, 2016

Coverage Status

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

@ramasilveyra
Copy link
Contributor Author

ramasilveyra commented Dec 24, 2016

@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.

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

coveralls commented Dec 24, 2016

Coverage Status

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

@coveralls
Copy link

coveralls commented Dec 24, 2016

Coverage Status

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

@ramasilveyra
Copy link
Contributor Author

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

@coveralls
Copy link

coveralls commented Jan 20, 2017

Coverage Status

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

2 similar comments
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

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.

@@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready 😄

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

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

@ramasilveyra ramasilveyra Jan 29, 2017

Choose a reason for hiding this comment

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

...
  } 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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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})
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Now this change request is fixed.

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

Choose a reason for hiding this comment

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

Rename pkg to packageContent

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready 😄

@coveralls
Copy link

Coverage Status

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

2 similar comments
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

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

@ramasilveyra 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
@ramasilveyra
Copy link
Contributor Author

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 .

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

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

@coveralls
Copy link

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@ramasilveyra
Copy link
Contributor Author

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

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

Choose a reason for hiding this comment

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

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? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready!

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Jan 29, 2017

Coverage Status

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

@johnsardine
Copy link

Can we specify if it should look also in bower_components?

@ramasilveyra 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
@ljharb
Copy link
Member

ljharb commented Feb 1, 2017

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

@johnsardine
Copy link

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

@ljharb
Copy link
Member

ljharb commented Feb 1, 2017

@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.

Copy link
Collaborator

@jfmengels jfmengels left a comment

Choose a reason for hiding this comment

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

LGTM! (sorry about the delay)

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

@ramasilveyra
Copy link
Contributor Author

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

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.

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

@coveralls
Copy link

Coverage Status

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

2 similar comments
@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

coveralls commented Feb 17, 2017

Coverage Status

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

@0xR
Copy link

0xR commented May 1, 2017

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 benmosher merged commit 0fb592e into import-js:master May 18, 2017
@benmosher
Copy link
Member

:shipit: 👍🏻 thanks for review @jfmengels

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.

LGTM

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.

7 participants