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

Babel should not silently remove unknown options after commander arguments #10698

Merged
merged 3 commits into from Nov 12, 2019

Conversation

@JLHwung
Copy link
Contributor

JLHwung commented Nov 12, 2019

Q                       A
Fixed Issues? Fixes #4825
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
Any Dependency Changes? commander.js is bumped to v4.0.1
License MIT

In this PR we bump commander.js to v4.0.1, which can throw on unknown options after arguments when an action handler is provided. To do so we register an empty action handler so that unknown options error can be thrown.

Impact analysis of upgrading commander.js from 2.8 to 4.0

Breaking changes from 2.0 to 3.0

  • Change TypeScript to use overloaded function for .command.
    Not related
  • custom event listeners: --no-foo on cli now emits option:no-foo (previously option:foo)
    Not related since we are not using custom event listeners
  • default value: defining --no-foo after defining --foo leaves the default value unchanged (previously set it to false)
    Not related since we don't have any option that has both --no- and -- prefixes.

Breaking changes from 3.0 to 4.0

  • keep command object out of program.args when action handler called
    Not related since we are not using the command object in an action handler
  • Commander is only officially supported on Node 8 and above, and requires Node 6
    It should be fine since we will drop Node 6 and 8 soon, too. If CI is good we may say it should work on Node 6 from now on.
  • Testing for no arguments is changed
    Not related since I can't find any .args.length usage in our codebase.
Copy link
Member

nicolo-ribaudo left a comment

Commander is only officially supported on Node 8 and above, and requires Node 6
It should be fine since we will drop Node 6 and 8 soon, too. If CI is good we may say it should work on Node 6 from now on.

Ok since CI is passing, but we should be ready to pin an exact version in the future if it stops working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.