Skip to content
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: replace regexp-tree by regexpu #10430

Merged
merged 4 commits into from Sep 17, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 11, 2019

Q                       A
Fixed Issues? Standardize on regexpu
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

This is our first step to standardize regexp transforms on regexpu. It helps reduce the package size and pave the way for an integral approach to treat unicode regex.

Currently the test failure is due to an upstream issue.


Edit by @nicolo-ribaudo

Depends on:

@JLHwung JLHwung added the PR: Internal 🏠 A type of pull request used for our changelog categories label Sep 11, 2019
@JLHwung JLHwung force-pushed the replace-regexp-tree-by-regexpu branch from a17778b to 7e43907 Compare September 13, 2019 13:53
@JLHwung JLHwung marked this pull request as ready for review September 13, 2019 14:06
@JLHwung JLHwung force-pushed the replace-regexp-tree-by-regexpu branch from 5cc8392 to 64df546 Compare September 13, 2019 15:58
@babel-bot
Copy link
Collaborator

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

@buildsize
Copy link

buildsize bot commented Sep 13, 2019

File name Previous Size New Size Change
babel-preset-env.js 2.43 MB 2.73 MB 313.86 KB (13%)
babel-preset-env.min.js 1.4 MB 1.65 MB 256.47 KB (18%)
babel.js 2.95 MB 2.95 MB 9 bytes (0%)
babel.min.js 1.63 MB 1.63 MB 9 bytes (0%)

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 13, 2019

The size change of @babel/preset-env-standalone comes from the fact that @babel/plugin-transform-named-capturing-groups is not included in https://github.com/babel/babel/blob/master/packages/babel-standalone/scripts/pluginConfig.json, therefore, it will instead be bundled into @babel/preset-env-standalone instead of @babel/standalone.

Looking into the pluginConfig.json, it seems that even while Stage 0 proposal (function-bind) is included in pluginConfig, a Stage 4 proposal (named-capturing-groups) is not included. I believe it is a bug and would propose to move all notIncludedPlugins

const notIncludedPlugins = {
"transform-named-capturing-groups-regex": require("@babel/plugin-transform-named-capturing-groups-regex"),
"transform-new-target": require("@babel/plugin-transform-new-target"),
"proposal-json-strings": require("@babel/plugin-proposal-json-strings"),
"proposal-dynamic-import": require("@babel/plugin-proposal-dynamic-import"),
};

into pluginsConfig.json. We expect a slight size increase of babel.js and decrease of babel-preset-env.js. In all the total size will be reduced since we are not including regexp-tree.

Side note: we may need specify the peerDependencies of @babel/preset-env-standalone to be @babel/standalone.

@nicolo-ribaudo
Copy link
Member

IIRC, notIncludedPlugins includes all the plugin from preset-env which are not part of the es* or stage-* presets.

Ideally, @babel/standalone should be as small as possible and all the plugins should only be in @babel/preset-env-standalone, but for legacy reasons we can't just move them.
By doing so, users can update preset-env-standalone without having to udpate @babel/standalone when a new plugin is added.
Unless there is a webpack hack to load regexpu-core from the @babel/standalone bundle in @babel/preset-env-standalone, I don't think that there is anything that we can do to remove that duplicated dependency.

@JLHwung
Copy link
Contributor Author

JLHwung commented Sep 14, 2019

Thank you for the explainers!

Unless there is a webpack hack to load regexpu-core from the @babel/standalone bundle in @babel/preset-env-standalone, I don't think that there is anything that we can do to remove that duplicated dependency.

The webpack dll is supposed to offer a runtime require-able libraries for webpack application. But from my previous experience I would rather stay away from it especially when we required cross-version compatibility between babel/standalone and preset-env-standalone.

I guess we have to accept the size change. But maybe we can deprecate plugin usage in babel-standalone? So we can move plugins to preset-env on v8 (not the JS engine).

@existentialism existentialism merged commit 87dc201 into babel:master Sep 17, 2019
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 PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants