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.

Sign up for GitHub

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

Jump to bottom

Monorepo: add package.json linter #36534

Merged
merged 12 commits into from Nov 6, 2019
Merged

Monorepo: add package.json linter #36534

merged 12 commits into from Nov 6, 2019

Conversation

simison
Copy link
Member

@simison simison commented Oct 4, 2019 •
edited

Lint monorepo package.json files.

Uses neat npm-package-json-lint for the job.

Linter config based on @wordpress/npm-package-json-lint-config and customized a bit to our needs, with a few overrides.

Changes proposed in this Pull Request

  • Adds package.json linter, not only to keep monorepo packages consistent but especially to keep devDependencies out from packages/**/package.json since they can cause actual issues.

Testing instructions

  • Run npm run lint:package-json

Linter runs as part of npm run lint as well.

@matticbot
Copy link
Contributor

Test live: https://calypso.live/?branch=add/package-json-linter

@matticbot
Copy link
Contributor

matticbot commented Oct 14, 2019 •
edited

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@simison simison mentioned this pull request Oct 14, 2019
Merged
1 task
simison
simison commented Oct 14, 2019
View reviewed changes
docs/monorepo.md
@@ -51,7 +51,7 @@ The only exception are `devDependencies` which _must be declared in the wp-calyp
"module": "dist/esm/index.js",
"sideEffects": false,
"keywords": [ "wordpress" ],
"author": "Your Name <you@example.com> (https://yoursite.wordpress.com/)",
"author": "Automattic Inc.",
Copy link
Member Author

@simison simison Oct 14, 2019 •
edited

Choose a reason for hiding this comment

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

I think this is better so as not to promote too tight "code ownership". Thoughts?

I can ping individuals who's names I've switched to the company name in this PR once it's in a mergeable state.

There is separate "contributors" array if folks really would like to have their names here. https://docs.npmjs.com/files/package.json#people-fields-author-contributors

I also tried to see if using org handle @automattic as an author gives any extra features in npm CLI or http://npmjs.org/ but it doesn't seem to.

@simison simison added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 19, 2019
@simison simison marked this pull request as ready for review October 19, 2019 19:49
@simison simison requested review from a team as code owners October 19, 2019 19:49
@simison simison removed request for a team October 21, 2019 12:00
simison
simison commented Oct 21, 2019
View reviewed changes
npmpackagejsonlint.config.js
@@ -0,0 +1,49 @@
module.exports = {
// See https://www.npmjs.com/package/@wordpress/npm-package-json-lint-config
extends: '@wordpress/npm-package-json-lint-config',
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, just list all those rules here and have one package less to Renovate/update. ;-)

Copy link
Member

Choose a reason for hiding this comment

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

🤷‍♂ How much value do we get value from extending? I'm inclined to just list our own rules here, it's pretty trivial to pull in or remove a rule when it makes sense, while keeping the package up to date adds overhead and it can be surprising when rules shift around.

Copy link
Member Author

@simison simison Oct 29, 2019 •
edited

Choose a reason for hiding this comment

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

How much value do we get value from extending?

Whenever anything changes/gets deprecated in the upstream package, we'd inherit those changes. We have that problem with our Stylelint config - a bunch of it was outdated last time I checked.

@simison
Copy link
Member Author

simison commented Oct 21, 2019

@gziolo does it make sense to use @wordpress/npm-package-json-lint-config like this?

PS. might be new prefer-no-devdependencies rule in npm-package-json-lint v4 would be useful to Gutenberg repo as well?

@gziolo
Copy link
Member

gziolo commented Oct 22, 2019 •
edited by sirreal

@gziolo does it make sense to use @wordpress/npm-package-json-lint-config like this?

Sure, this is how we use it internally in Gutenberg as well:
https://github.com/WordPress/gutenberg/blob/4df25af5946c518d46ebe14a24aea1d44cd63593/package.json#L158

PS. might be new prefer-no-devdependencies rule in npm-package-json-lint v4 would be useful to Gutenberg repo as well?

Cool, see WordPress/gutenberg#18054 :)

gziolo
gziolo reviewed Oct 22, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
@sirreal sirreal force-pushed the add/package-json-linter branch 2 times, most recently from 65f1a15 to bd091fa Compare October 22, 2019 09:33
@sirreal
Copy link
Member

sirreal commented Oct 22, 2019

Rebased

sirreal
sirreal reviewed Oct 23, 2019
View reviewed changes
package.json Outdated Show resolved Hide resolved
npmpackagejsonlint.config.js
@@ -0,0 +1,49 @@
module.exports = {
// See https://www.npmjs.com/package/@wordpress/npm-package-json-lint-config
extends: '@wordpress/npm-package-json-lint-config',
Copy link
Member

Choose a reason for hiding this comment

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

🤷‍♂ How much value do we get value from extending? I'm inclined to just list our own rules here, it's pretty trivial to pull in or remove a rule when it makes sense, while keeping the package up to date adds overhead and it can be surprising when rules shift around.

blowery
blowery reviewed Oct 29, 2019
View reviewed changes
npmpackagejsonlint.config.js
rules: {
'prefer-no-devDependencies': 'error',
'prefer-property-order': 'off',
'require-bugs': 'off',
Copy link
Contributor

Choose a reason for hiding this comment

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

still my favorite rule

blowery
blowery approved these changes Nov 6, 2019
View reviewed changes
Copy link
Contributor

@blowery blowery left a comment

Choose a reason for hiding this comment

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

I think this is good to land. Anything else you want to add?

sirreal
sirreal approved these changes Nov 6, 2019
View reviewed changes
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

:shipit:

@blowery blowery merged commit fa3d9e9 into master Nov 6, 2019
@blowery blowery deleted the add/package-json-linter branch November 6, 2019 16:40
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 6, 2019
@ockham ockham mentioned this pull request Nov 13, 2019
Merged
@jsnajdr jsnajdr mentioned this pull request Jan 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

@blowery blowery blowery approved these changes

@sirreal sirreal sirreal approved these changes

Assignees
No one assigned
Labels
Build [Type] Enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@simison @matticbot @gziolo @sirreal @blowery