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
refactor: add @babel/helper-validator-option #12006
Conversation
ModulesOption[modulesOpt.toString()] || modulesOpt === ModulesOption.false, | ||
`Invalid Option: The 'modules' option must be one of \n` + | ||
`The 'modules' option must be one of \n` + |
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.
These errors will now be thrown with @babel/preset-env:
prefix.
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 82a0546:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/28379/ |
} | ||
} | ||
|
||
// note: we do not consider rewrite them to high order functions |
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.
Probably don't need this comment, but if we want to make it clear that this should be refactored in the future maybe we can add a TODO
?
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.
In fact it is quite opposite: It is to leave it as-is (for less call stacks when error is thrown) unless we are going to support number-only preset options in the future, which is quite unlikely.
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 looks great, thanks!
Nit, but should we call this option-validator? EDIT: I guess it's to be consistent with |
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.
<3 this PR!
Converted to draft as |
import browserModulesData from "@babel/compat-data/native-modules"; | ||
|
||
import { | ||
semverify, | ||
semverMin, | ||
isUnreleasedVersion, | ||
getLowestUnreleased, | ||
v, |
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.
Is there a reason we're naming this v
? I'm generally in favor of descriptive names, and I worry that its meaning will be obscured for future maintainers.
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.
+1 If validator
is too verbose, we can still do import { validator as v }
: at least you don't have to check into another file what v
means.
Co-authored-by: Brian Ng <bng412@gmail.com>
aa94ca4
to
82a0546
Compare
@babel/helper-validator-option
is drafted to replace bothinvariant
andlevenary
This PR is meant to resolve #10927 (comment) and tanhauhau/levenary#7 (comment).
#10927 will be rebased on this PR to apply similar option checks for all other
@babel/presets
and related@babel/plugins
.