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.

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

Bump commander from v2 to v12 #137

Merged
merged 3 commits into from
Aug 30, 2024
Merged

Conversation

fregante
Copy link
Contributor

@fregante fregante commented Aug 4, 2024

@fregante fregante marked this pull request as draft August 4, 2024 17:27
Copy link
Contributor Author

@fregante fregante left a comment

Choose a reason for hiding this comment

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

@willdurand second PR ready


program
.command("start")
.description("Start Firefox")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

.option() must now follow .command()

.command("start")
.description("Start Firefox")
.action(function() {
console.log(Object.keys(program))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test harness relied on program being outputted to the user. Commander no longer applies the flags to program and they no longer appear as --flags anymore, so the test had to be updated.

I made two changes:

  • the user no longer sees this noise, it's hidden behind a fx-runner-specific ENV
  • instead of looking for random flags, it matches against the whole parsedOptions object. This ensures that the additional logic in this object can also be tested.

@fregante fregante marked this pull request as ready for review August 4, 2024 17:50
@Rob--W Rob--W merged commit 9c8dcd7 into mozilla:master Aug 30, 2024
3 checks passed
@fregante fregante deleted the bump-commander branch August 30, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dependencies cleanup targeting the next major version bump
2 participants