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
chore: standardize npm script names per #14827 #16315
Changes from 2 commits
88f5c45
fbdf6da
552072a
60bf6f8
bb930b4
c7007c3
ccd3370
a7efbeb
e445fcd
c3e9628
9ee75d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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" | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
automation > bikeshedding
There was a problem hiding this comment.
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