Skip to content

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

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
JLHwung:fix-4825
Nov 12, 2019
Merged

Babel should not silently remove unknown options after commander arguments#10698
nicolo-ribaudo merged 3 commits intobabel:masterfrom
JLHwung:fix-4825

Conversation

@JLHwung
Copy link
Copy Markdown
Contributor

@JLHwung 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.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories pkg: cli labels Nov 12, 2019
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

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.

@nicolo-ribaudo nicolo-ribaudo merged commit 7633f09 into babel:master Nov 12, 2019
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 12, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Feb 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: cli PR: Bug Fix 🐛 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate babel-cli program options

3 participants