Skip to content

Conversation

@gziolo
Copy link
Member

@gziolo gziolo commented May 29, 2018

Description

This PR updates @wordpress/scripts to the next minor version which exposed lint-pkg-json command. It exposed the same checks for package.json files for all packages maintained by Lerna. This brings the setup of Gutenberg and packages closer to each other before they get merged together in the near future.

I also reordered scripts section to make everything sorted by script name. I also pulled in npm-run-all package to make it easier to run groups of scripts.

How has this been tested?

  • npm run lint-pkg-json
  • npm run lint

To make sure that this new linter works you can update any package.json file inside packages folder and change the order of properties or update author field, or remove publishConfig field.

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@gziolo gziolo added [Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality labels May 29, 2018
@gziolo gziolo added this to the 3.0 milestone May 29, 2018
@gziolo gziolo self-assigned this May 29, 2018
@gziolo gziolo requested review from aduth, ntwb, pento and youknowriad May 29, 2018 14:24
@gziolo gziolo force-pushed the add/lint-pkg-json branch from 3ea9263 to bb8a6e5 Compare May 29, 2018 14:37
@youknowriad youknowriad removed this from the 3.0 milestone May 30, 2018
@youknowriad
Copy link
Contributor

Removed the milestone as we don't really care when this lands.

(Looks like there's some failing tests)

package.json Outdated
"valid-values-author": [
"error",
[
"WordPress"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Out of curiosity, why did we decide upon "WordPress" as author when wordpress-develop/package.json specifies it as "The WordPress Contributors".

https://github.com/WordPress/wordpress-develop/blob/ecb3302/package.json#L13

(cc @ntwb)

Copy link
Member

@ntwb ntwb May 31, 2018

Choose a reason for hiding this comment

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

Not sure there was ever a discussion, let's change it to The WordPress Contributors

Also ref: Copyright 2011-2018 by the contributors via license.txt

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated 👍

package.json Outdated
"fixtures:server-registered": "docker-compose run -w /var/www/html/wp-content/plugins/gutenberg --rm wordpress ./bin/get-server-blocks.php > core-blocks/test/server-registered.json",
"fixtures:generate": "npm run fixtures:server-registered && cross-env GENERATE_MISSING_FIXTURES=y npm run test-unit",
"fixtures:regenerate": "npm run fixtures:clean && npm run fixtures:generate",
"fixtures:regenerate": "npm-run-all fixtures:clean fixtures:generate",
Copy link
Member

Choose a reason for hiding this comment

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

Is npm-run-all default to parallel or sequential execution? Sequential is pretty important for this task to ensure that clean occurs before (and does not overlap with) newly generated files.

See also run-s: https://www.npmjs.com/package/npm-run-all#-usage

"lint-php": "docker-compose run --rm composer run-script lint",
"predev": "check-node-version --package",
"check-engines": "check-node-version --package",
"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
Copy link
Member

Choose a reason for hiding this comment

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

Minor: npm-run-all could be used here now in place of npm run lint && npm run build. I think they could occur in parallel as well? Edit: Though, the fact we're using concurrently separate here makes me think we needed them to occur in sequence. Sounds familiar, hmm..

Copy link
Member Author

Choose a reason for hiding this comment

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

I spend some time trying to rewrite it with npm-run-all but failed, so I removed this dependency at all in favor of concurrently :)

@gziolo gziolo force-pushed the add/lint-pkg-json branch from bb8a6e5 to 3adfdcd Compare May 31, 2018 19:49
@gziolo gziolo merged commit 68ad658 into master Jun 5, 2018
@gziolo gziolo deleted the add/lint-pkg-json branch June 5, 2018 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Build Tooling Issues or PRs related to build tooling [Type] Code Quality Issues or PRs that relate to code quality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants