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

chore: standardize npm script names per #14827 #16315

Merged
merged 11 commits into from Dec 14, 2022
Merged

chore: standardize npm script names per #14827 #16315

Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
88f5c45
chore: standardize npm script names per #14827
pmcelhaney Sep 15, 2022
fbdf6da
put lint:fix right after lint
pmcelhaney Sep 15, 2022
552072a
chore: fix ordering of :fix
pmcelhaney Nov 11, 2022
60bf6f8
chore: change release scripts to have "release:" prefix
pmcelhaney Nov 28, 2022
bb930b4
docs: update package.json naming to include 'release'
pmcelhaney Nov 28, 2022
c7007c3
docs: add css so abnf code formats okay
pmcelhaney Nov 28, 2022
ccd3370
chore: exclude linting from npm test
pmcelhaney Dec 8, 2022
a7efbeb
Merge branch 'main' into 14827-standardize-npm-scripts
pmcelhaney Dec 8, 2022
e445fcd
Revert "docs: add css so abnf code formats okay"
pmcelhaney Dec 13, 2022
c3e9628
Merge branch 'main' into 14827-standardize-npm-scripts
pmcelhaney Dec 13, 2022
9ee75d7
docs: remove counter-reset. I guess that was a merge error
pmcelhaney Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 14 additions & 14 deletions package.json
View file
Open in desktop
Expand Up @@ -13,22 +13,22 @@
"./use-at-your-own-risk": "./lib/unsupported-api.js"
},
"scripts": {
"build:docs:update-links": "node tools/fetch-docs-links.js",
"build:release": "node Makefile.js generateRelease",
"build:release:alpha": "node Makefile.js generatePrerelease -- alpha",
"build:release:beta": "node Makefile.js generatePrerelease -- beta",
"build:release:publish": "node Makefile.js publishRelease",
"build:release:rc": "node Makefile.js generatePrerelease -- rc",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit confusing that "publish" comes before "rc". The standard (which I wrote) says alphabetical order. May want to update the standard to make room for nuances like this.

Suggested change
"build:release:publish": "node Makefile.js publishRelease",
"build:release:rc": "node Makefile.js generatePrerelease -- rc",
"build:release:rc": "node Makefile.js generatePrerelease -- rc",
"build:release:publish": "node Makefile.js publishRelease",

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I understand that there are one-off cases where the ordering can be unusual, but personally, I don't think we should have exceptions to alphabetical ordering. We should just enable prefer-alphabetical-scripts from npm-package-json-lint and call it a day so there's no need to debate the ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense.

automation > bikeshedding

Copy link
Contributor

Choose a reason for hiding this comment

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

Running sort-package-json's --check flag in CI can also help here I think

pmcelhaney marked this conversation as resolved.
Show resolved Hide resolved
"build:site": "node Makefile.js gensite",
"build:webpack": "node Makefile.js webpack",
"lint": "node Makefile.js lint",
"lint:fix": "node Makefile.js lint -- fix",
"lint:docs:js": "node Makefile.js lintDocsJS",
"lint:docs:js:fix": "node Makefile.js lintDocsJS -- fix",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not alphabetical because "lint:f" should come after "lint:d". If we write a validator at some point we'll need to fuss over these details.

pmcelhaney marked this conversation as resolved.
Show resolved Hide resolved
"test": "node Makefile.js test",
"test:cli": "mocha",
"lint": "node Makefile.js lint",
"lint:docsjs": "node Makefile.js lintDocsJS",
"fix": "node Makefile.js lint -- fix",
"fix:docsjs": "node Makefile.js lintDocsJS -- fix",
"fuzz": "node Makefile.js fuzz",
"generate-release": "node Makefile.js generateRelease",
"generate-alpharelease": "node Makefile.js generatePrerelease -- alpha",
"generate-betarelease": "node Makefile.js generatePrerelease -- beta",
"generate-rcrelease": "node Makefile.js generatePrerelease -- rc",
"publish-release": "node Makefile.js publishRelease",
Comment on lines -23 to -27
Copy link
Member

Choose a reason for hiding this comment

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

These scripts are used on Jenkins, so we need to decide if we'll make a change there or keep these names here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know how complex / risky it is to update Jenkins. If you decide to use the new names, might want to do it like this.

  1. Add the new names (without removing the old names).
  2. Update Jenkins to use the new names.
  3. A second commit that removes the old names.

Copy link
Member

Choose a reason for hiding this comment

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

Let’s just leave these as-is right now. These aren’t just used within Jenkins, they are baked into eslint-release, which is used by all of our packages, making changing these script names more complex.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, was too tired when I wrote this. The only place using these command is in Jenkins, and so long as we update Jenkins before merging this, I don’t see a problem.

"gensite": "node Makefile.js gensite",
"webpack": "node Makefile.js webpack",
"perf": "node Makefile.js perf",
"docs:update-links": "node tools/fetch-docs-links.js"
"test:fuzz": "node Makefile.js fuzz",
"test:performance": "node Makefile.js perf"
},
"gitHooks": {
"pre-commit": "lint-staged"
Expand Down