Add regexp support to include/exclude by aminmarashi · Pull Request #7242 · babel/babel

aminmarashi · 2018-01-20T04:03:30Z

Q	A
Fixed Issues?	Fixes #6636
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	👍
Tests Added + Pass?	👍
Documentation PR
Any Dependency Changes?

This PR adds support for regular expressions to includes/excludes list of plugins/builtins. Regular expressions can be passed as normal RegExp objects, or in string 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:

Full name: "es6.math.sign"
Partial name: "es6.math.*"
RegExp in string: "es6.math.sin.?"
RegExp Object: /^transform-.*$/ or new 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 match es6-math-sign.

babel-bot · 2018-01-20T04:12:16Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7301/

yavorsky · 2018-01-20T12:50:34Z

packages/babel-preset-env/src/normalize-options.js

+const pluginToRegExp = (plugin: any): RegExp => {
+  return plugin instanceof RegExp
+    ? plugin
+    : new RegExp(normalizePluginName(plugin));


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.

True. I'll look into optimizing the RegExp creation.

yavorsky · 2018-01-20T12:53:17Z

packages/babel-preset-env/src/normalize-options.js

+): Array<string> => pluginList.concat(selectPlugins(regexp));
+
+const isValidPlugin = (plugin: any): boolean =>
+  validRegExp(plugin) && selectPlugins(pluginToRegExp(plugin)).length > 0;


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.

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.

yavorsky · 2018-01-20T13:10:16Z

packages/babel-preset-env/src/normalize-options.js

+    )}' passed to the '${type}' option are not valid.`,
+  );
+
+  return plugins.map(pluginToRegExp).reduce(populatePlugins, []);


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.

yavorsky · 2018-01-20T13:16:08Z

packages/babel-preset-env/src/normalize-options.js

+};
+
+const selectPlugins = (regexp: RegExp): Array<string> =>
+  Array.from(validIncludesAndExcludes).filter(item => item.match(regexp));


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.

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.

aminmarashi · 2018-01-20T16:12:27Z

@yavorsky Thank you. I think I've addressed all of the issues.

Qantas94Heavy

LGTM with one very unimportant style nitpick.

Qantas94Heavy · 2018-02-17T10:41:20Z

packages/babel-preset-env/src/normalize-options.js

-  opts: Array<string> = [],
+const pluginToRegExp = (plugin: any): RegExp => {
+  try {
+    return plugin instanceof RegExp


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.

This try/catch is actually for new RegExp.

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; }

That's actually a good idea. Will change this and the doc and ask for a review again. Thank you. 👍

Qantas94Heavy

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

aminmarashi · 2018-03-17T13:34:21Z

@Qantas94Heavy Applied your comments 👍

Qantas94Heavy

LGTM bar a couple of typos which should be easily fixed.

Qantas94Heavy · 2018-03-17T13:42:40Z

packages/babel-preset-env/src/normalize-options.js

+const pluginToRegExp = (plugin: any): RegExp => {
+  if (plugin instanceof RegExp) return plugin;
+  try {
+    return  new RegExp(`^${normalizePluginName(plugin)}$`);


Typo, remove double space.

Andarist · 2018-03-18T11:00:31Z

packages/babel-preset-env/src/normalize-options.js

-export const normalizePluginNames = (plugins: Array<string>): Array<string> =>
-  plugins.map(normalizePluginName);
+const normalizePluginName = (plugin: string): string =>
+  plugin.replace("babel-plugin-", "");


why the change? it can replace too much now

We already wrap it in ^$ at line 21

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?

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 👍

Andarist · 2018-03-18T11:04:06Z

packages/babel-preset-env/src/normalize-options.js

+
+const selectPlugins = (regexp: RegExp): Array<string> =>
+  Array.from(validIncludesAndExcludes).filter(
+    item => regexp instanceof RegExp && item.match(regexp),


regexp.test(item) would be IMHO more appropriate as it returns boolean instead of maybe array

yes, sounds good.

Andarist · 2018-03-18T11:06:42Z

packages/babel-preset-env/src/normalize-options.js

  );

-  return opts;
+  return [].concat(...selectedPlugins);


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)

Yeah, good idea 👍

Do we have any special place for keeping these kinds of utils?

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

Andarist · 2018-03-18T14:19:27Z

packages/babel-preset-env/test/normalize-options.spec.js

    });

+    it("should not normalize babel-plugin with prefix", () => {
+      const normalized = normalizePluginName("prefix-babel-plugin-postfix");


Imho this is kinda testing an implementation detail, could you rewrite it to test an actual effect rather than a single normalization step?

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.

makes sense, u'd have to mock something or create fake node_modules in test dir, not worth it then at this point

hzoo · 2018-03-19T14:23:24Z

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) ...

aminmarashi added 5 commits January 19, 2018 21:20

Add support for RegExp includes/excludes

83d4e68

Keep the plugin order

909cdd1

Detect invalid modules in regexp

7647538

Add more tests for regexp

9fa7e1e

Cover builtins, and unnormalized in the RegExp tests

00bccd2

aminmarashi added 3 commits January 20, 2018 12:27

Remove babel-plugin- in all positions

bea87a8

Change babel-plugin- prefix to string

7fe2a54

Add a test for the same module in include/exclude

044a53d

yavorsky changed the title ~~Add regexp support to include/excludes~~ Add regexp support to include/exclude Jan 20, 2018

yavorsky added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: preset-env labels Jan 20, 2018

yavorsky reviewed Jan 20, 2018

View reviewed changes

aminmarashi added 5 commits January 20, 2018 22:41

Handle partial matches explicitly

150678b

Remove extra valid regexp check

29a4527

Optimise validation of plugins

10eb39c

Optimise selecting the plugins

df00532

Fix undefined include/exclude option

707986c

Qantas94Heavy approved these changes Feb 17, 2018

View reviewed changes

Qantas94Heavy suggested changes Feb 17, 2018

View reviewed changes

aminmarashi force-pushed the add-regex-for-includes-excludes branch from 1db5c98 to 707986c Compare March 17, 2018 12:47

Update documentation to reflect the new include matching

92e57a1

Qantas94Heavy approved these changes Mar 17, 2018

View reviewed changes

Fix typo

531ad27

Qantas94Heavy approved these changes Mar 18, 2018

View reviewed changes

Andarist reviewed Mar 18, 2018

View reviewed changes

Apply reviews

a38bd3e

Use regexp.test instead of string.match (slower) Define flatten helper Do not normalize babel-plugin anywhere in the string

Andarist reviewed Mar 18, 2018

View reviewed changes

Andarist approved these changes Mar 18, 2018

View reviewed changes

Andarist merged commit 8eee435 into babel:master Mar 18, 2018

aminmarashi deleted the add-regex-for-includes-excludes branch March 18, 2018 23:28

existentialism mentioned this pull request Mar 20, 2018

Disable flow on transformClass, fix preset-env errors #7605

Merged

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Uh oh!

Conversation

aminmarashi commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babel-bot commented Jan 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aminmarashi commented Jan 20, 2018

Uh oh!

Qantas94Heavy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Qantas94Heavy left a comment

Choose a reason for hiding this comment

Uh oh!

aminmarashi commented Mar 17, 2018

Uh oh!

Qantas94Heavy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andarist Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hzoo commented Mar 19, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

aminmarashi commented Jan 20, 2018 •

edited

Loading

babel-bot commented Jan 20, 2018 •

edited

Loading

Qantas94Heavy left a comment •

edited

Loading

Andarist Mar 18, 2018 •

edited

Loading