-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Add regexp support to include/exclude #7242
Add regexp support to include/exclude #7242
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7301/ |
const pluginToRegExp = (plugin: any): RegExp => { | ||
return plugin instanceof RegExp | ||
? plugin | ||
: new RegExp(normalizePluginName(plugin)); |
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.
We are creating RegExp 2 times. There and inside validRegExp
method. Maybe optimize it so we will call new RegExp
one time per plugin?
Also, we could check for plugin instanceof RegExp
on the validRegExp
stage and don't call new RegExp(regexp);
if it's already a RegExp.
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.
True. I'll look into optimizing the RegExp
creation.
): Array<string> => pluginList.concat(selectPlugins(regexp)); | ||
|
||
const isValidPlugin = (plugin: any): boolean => | ||
validRegExp(plugin) && selectPlugins(pluginToRegExp(plugin)).length > 0; |
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.
For example, we can get rid of validRegExp
and call pluginToRegExp
which could return null
if the value can't be converted into RegExp and RegExp instance if it can. Then just run selectPlugins(regExpValue)
if regExpInstance isn't null
.
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 problem with this approach is that we can only report the invalid plugins one by one. However, I think it's better if expandIncludesAndExcludes
reports all the invalid plugins and throw the list of invalid ones if there's any.
)}' passed to the '${type}' option are not valid.`, | ||
); | ||
|
||
return plugins.map(pluginToRegExp).reduce(populatePlugins, []); |
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.
And if the value is a string
and it's a valid regExp and could be converted with pluginToRegExp
it's still a string
, so there will be the 3-rd new RegExp(plugin)
call.
}; | ||
|
||
const selectPlugins = (regexp: RegExp): Array<string> => | ||
Array.from(validIncludesAndExcludes).filter(item => item.match(regexp)); |
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.
Notice that with new RegExp(value)
, for case like es6.reflect.set
, it will also match the es6.reflect.set-prototype-of
, which is a different built-in.
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.
With this approach, there's no easy way of distinguishing a normal string with a partial match. We can prevent the that from happening by wrapping the strings with ^...$
before converting them to RegExp
, then the partial matches can be written explicitly like: "es6.reflect.set.*"
.
Made the changes and a test case to fix that.
@yavorsky Thank you. I think I've addressed all of the issues. |
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.
LGTM with one very unimportant style nitpick.
opts: Array<string> = [], | ||
const pluginToRegExp = (plugin: any): RegExp => { | ||
try { | ||
return plugin instanceof RegExp |
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.
Nitpick: Is there a chance of plugin instanceof RegExp
throwing? If not, moving this out of the try-catch block may be a bit more readable. Not so important though.
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 try/catch
is actually for new RegExp
.
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.
I was thinking of something like this, but it doesn't really matter anyway. Don't worry about it.
if (plugin instanceof RegExp) {
return plugin;
}
try {
return new RegExp(`^${normalizePluginName(plugin)}$`);
} catch (e) {
return null;
}
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.
That's actually a good idea. Will change this and the doc and ask for a review again. Thank you. 👍
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.
On second thought, I believe the docs need to be updated for this new feature: https://github.com/babel/babel/blob/master/packages/babel-preset-env/README.md#include
1db5c98
to
707986c
Compare
@Qantas94Heavy Applied your comments 👍 |
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.
LGTM bar a couple of typos which should be easily fixed.
const pluginToRegExp = (plugin: any): RegExp => { | ||
if (plugin instanceof RegExp) return plugin; | ||
try { | ||
return new RegExp(`^${normalizePluginName(plugin)}$`); |
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.
Typo, remove double space.
export const normalizePluginNames = (plugins: Array<string>): Array<string> => | ||
plugins.map(normalizePluginName); | ||
const normalizePluginName = (plugin: string): string => | ||
plugin.replace("babel-plugin-", ""); |
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.
why the change? it can replace too much now
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.
We already wrap it in ^$
at line 21
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.
isnt it wrapped AFTER normalization? with ur approach we can get a final regexp of /^smth-foo$/
for smth-babel-plugin-foo
input, which was not the case before the PR, while we are at it maybe you could also add a test case for this?
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.
Yes, I made this change to still normalize cases like "^babel-plugin-foo"
, but I don't think that's a good idea either. I'll add the test and fix this, nice catch 👍
|
||
const selectPlugins = (regexp: RegExp): Array<string> => | ||
Array.from(validIncludesAndExcludes).filter( | ||
item => regexp instanceof RegExp && item.match(regexp), |
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.
regexp.test(item)
would be IMHO more appropriate as it returns boolean instead of maybe array
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.
yes, sounds good.
", ", | ||
)}' passed to the '${type}' option are not | ||
valid. Please check data/[plugin-features|built-in-features].js in babel-preset-env`, | ||
); | ||
|
||
return opts; | ||
return [].concat(...selectedPlugins); |
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.
I guess this is suppose to make a flatten
operation, it would be good to introduce this utility and use it here
const flatten = arr => [].concat(...arr)
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.
Yeah, good idea 👍
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.
Do we have any special place for keeping these kinds of utils?
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.
not rly (at least i dont think so), I would just make it a local function in this file for now
Use regexp.test instead of string.match (slower) Define flatten helper Do not normalize babel-plugin anywhere in the string
@@ -23,6 +21,11 @@ describe("normalize-options", () => { | |||
]); | |||
}); | |||
|
|||
it("should not normalize babel-plugin with prefix", () => { | |||
const normalized = normalizePluginName("prefix-babel-plugin-postfix"); |
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.
Imho this is kinda testing an implementation detail, could you rewrite it to test an actual effect rather than a single normalization step?
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.
It's a little bit tricky to write a test for the include
/exclude
field, because as far as I know there is no prefix-babel-plugin-*
, therefore any test will end up throwing an error (invalid plugin) regardless of this behavior.
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.
makes sense, u'd have to mock something or create fake node_modules
in test dir, not worth it then at this point
Thanks for the PR! After looking at this again (I suggested this change originally), it feels like I did this as more of a workaround. Ideally you wouldn't have to do anything like this because Babel would handle it for you, but before that this should work for people that need it. |
* master: (140 commits) Update to beta.42 (babel#7609) Disable flow on transformClass, fix preset-env errors (babel#7605) Logical Assignment: ensure computed key isn't recomputed (babel#7604) Remove obsolete max-len eslint rule and reformat some stuff to fit (babel#7602) Centralize Babel's own compilation config to make it easier to follow. (babel#7599) Run prettier to format all JSON. Tweak es2015-related plugin order in preset-env (babel#7586) Refactored quirky inheritance in babel-plugin-transform-classes (babel#7444) Add RegExp support to include/exclude preset-env options (babel#7242) v7.0.0-beta.42 Use strict namespace behavior for mjs files. (babel#7545) Remove outdated spec deviation note [skip ci] (babel#7571) Ensure that the backward-compat logic for plugin-utils copies over the version API properly. (babel#7580) Rename actual/expected test files to input/output (babel#7578) Use helper-module-import inside entry plugin too Use helper-module-imports instead of custom import (babel#7457) Fix "Module build failed: Error: Cannot find module '@babel/types'" (babel#7575) Wrap wrapNativeSuper helpers in redefining functions for better tree-shakeability (babel#7188) Favour extends helper over objectWithoutProperties when whole object gets copied anyway (babel#7390) Fix incorrect value of _cache in _wrapNativeSuper (babel#7570) ...
This PR adds support for regular expressions to includes/excludes list of plugins/builtins. Regular expressions can be passed as normal
RegExp
objects, or instring
format.We expand the regular expressions to get the list of plugins and then pass them around, therefore virtually none of the previous functions are affected.
Plugins can still be passed with full name or a partial name.
Examples:
"es6.math.sign"
"es6.math.*"
"es6.math.sin.?"
RegExp
Object:/^transform-.*$/
ornew RegExp("^transform-modules-.*")
We still normalize the plugin names before turning them into
RegExp
, therefore patterns like"^babel-plugin-transform-.*"
still work (, but/^babel-plugin-transform-.*/
doesn't).Note: In all of the above examples
.
represents a match for any character in a regular expression, and not the dot character so for example"es6.math.sign"
would matches6-math-sign
.