-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
[Babel 8] Disallow excess arguments in babel-external-helpers #17741
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
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60712 |
| // register an empty action handler so that program.js can throw on | ||
| // unknown options _after_ args | ||
| // see https://github.com/tj/program.js/issues/561#issuecomment-522209408 | ||
| program.action(() => {}); |
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.
The empty action handler is also removed as the upstream issue was fixed.
|
commit: |
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.
The CI error appears unrelated.
|
Yes, the CI error is from setup-node. It seems that the action struggles to retrieve the information of the new node.js release 25.5.0. |
|
I also noticed that downloading Node.js v0.10 in test262 timed out, which suggests a problem with the Node.js server. Although I didn't encounter any issues accessing it locally. Updated: |
| // unknown options _after_ args | ||
| // see https://github.com/tj/program.js/issues/561#issuecomment-522209408 | ||
| program.action(() => {}); | ||
| program.usage("[options] [files...]"); |
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.
The usage is updated from <files...> to [files...], because in the REPL -e mode, the files argument is not required.
To be honest I have no idea why nowdays somebody would want to use external helpers 😅 |
Exactly, if we split external-helpers-cli from babel-cli, we can gather the actual usage data from the % of downloads. If the dedicate library does not see very much usage and we receive no complaints from the removal in Babel 8, we can discontinue that library in Babel 9. |
|
Ok let's do it 👍 |
commander,globandslashto the latest major releases.In this PR we bumped three external dependencies to the latest major releases. As a result of the
commander.jsupdate, thebabel-external-helperscli now rejects excess arguments. I think this change is reasonable as that script does not expect an input argument, any passed arguments might indicate that there is something wrong, i.e. if--output-typeis written asoutput-type, this check will throw an error "unexpected arguments", currently the script will silently accept the arguments even if they are not used.Aside: I think we could also consider extract the
babel-external-helpersscript from thebabel-clilibrary, maybe into a dedicate library, since most@babel/cliusers would not need this script.