-
-
Notifications
You must be signed in to change notification settings - Fork 368
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: Update commander.js dependency #1255
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for submitting this chore PR! — Once you’re done I’m happy to review this properly. Just commenting pre-emptively on the npm-run-all
thing [To be frank, I didn’t realize this PR was still in draft state, haha]
package.json
Outdated
@@ -71,7 +71,8 @@ | |||
"docs:cp_defs": "mkdir docs/docs_generate; cp source/danger.d.ts docs/docs_generate; cp node_modules/@octokit/rest/index.d.ts docs/docs_generate/github.d.ts", | |||
"docs": "yarn run docs:cp_defs; yarn typedoc --ignoreCompilerErrors --mode modules --json docs/js_ref_dsl_docs.json --includeDeclarations source", | |||
"dts-lint": "yarn run declarations && yarn dtslint types", | |||
"danger:prepush": "yarn build:fast; yarn test:fixtures; node distribution/commands/danger.js local --base main --dangerfile dangerfile.lite.ts" | |||
"danger:local": "node distribution/commands/danger.js local --base main --dangerfile dangerfile.lite.ts", | |||
"danger:prepush": "npm-run-all build:fast test:fixtures danger:local" |
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.
Do we really need this npm-run-all
dependency? Is it just to have a lighter-weight syntax to run multiple commands?
yarn --silent build:fast && yarn --silent test:fixtures && yarn --silent danger:local
seems to produce the same result without adding a new dependency, unless I’m missing something?
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.
this PR is based on changes from #1254 branch
This is now blocked on the "engine" keyword, as I'm not sure to what version we can update here. see discussion: |
3fbe1cd
to
56f0100
Compare
|
Depends on:
Breaking points:
Skipped because this.opts() change seems too much right now: