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

Add useBuiltIns and useESModules options to transform-runtime #5442

Merged
merged 4 commits into from
Mar 15, 2017

Conversation

Jessidhia
Copy link
Member

@Jessidhia Jessidhia commented Mar 10, 2017

useBuiltIns uses versions of the helpers that do not import even
internal polyfills from core-js.

useESModules uses versions of the helpers that do not go through
transform-es2015-modules-commonjs. This allows for smaller builds in
module systems like webpack, as it doesn't need to preserve commonjs
semantics.

This includes changes to the babel-runtime build-dist script, which
will build the versions of the runtime helpers to be used by
combinations of useBuiltIns and useESModules.

This is untested other than by looking at the output and seeing if it makes sense >_<

{ polyfill: false, useBuiltIns: true, useESModules: true } will be perfect for webpack/rollup
users that want to deduplicate helpers without accidentally bundling an extra
core-js when babel-polyfill is already in use, or when, say, targeting electron.

useBuiltIns uses versions of the helpers that do not import even
internal polyfills from core-js.

useESModules uses versions of the helpers that do not go through
transform-es2015-modules-commonjs. This allows for smaller builds in
module systems like webpack, as it doesn't need to preserve commonjs
semantics.

This includes changes to the babel-runtime build-dist script, which
will build the versions of the runtime helpers to be used by
combinations of useBuiltIns and useESModules.
@mention-bot
Copy link

@Kovensky, thanks for your PR! By analyzing the history of the files in this pull request, we identified @gaearon, @hzoo and @zertosh to be potential reviewers.

@Jessidhia Jessidhia added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Mar 10, 2017
@babel-bot
Copy link
Collaborator

Hey @Kovensky! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

@codecov
Copy link

codecov bot commented Mar 10, 2017

Codecov Report

Merging #5442 into 7.0 will decrease coverage by 0.03%.
The diff coverage is 10%.

@@            Coverage Diff             @@
##              7.0    #5442      +/-   ##
==========================================
- Coverage   85.46%   85.42%   -0.04%     
==========================================
  Files         203      203              
  Lines        9533     9537       +4     
  Branches     2703     2706       +3     
==========================================
  Hits         8147     8147              
- Misses        899      900       +1     
- Partials      487      490       +3
Impacted Files Coverage Δ
...ckages/babel-plugin-transform-runtime/src/index.js 70.76% <10%> (-4.65%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12eb25c...8dc2176. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 10, 2017

Codecov Report

Merging #5442 into 7.0 will increase coverage by 0.37%.
The diff coverage is 70%.

@@            Coverage Diff             @@
##              7.0    #5442      +/-   ##
==========================================
+ Coverage   85.41%   85.79%   +0.37%     
==========================================
  Files         203      203              
  Lines        9525     9842     +317     
  Branches     2703     2832     +129     
==========================================
+ Hits         8136     8444     +308     
- Misses        902      909       +7     
- Partials      487      489       +2
Impacted Files Coverage Δ
...ckages/babel-plugin-transform-runtime/src/index.js 80% <70%> (+4.59%)
...kages/babel-core/src/transformation/file/logger.js 40.54% <0%> (-6.13%)
...bel-plugin-transform-es2015-classes/src/vanilla.js 90.59% <0%> (+0.42%)
packages/babel-traverse/src/path/context.js 87.06% <0%> (+0.86%)
.../transformation/file/options/build-config-chain.js 96.1% <0%> (+2.22%)
.../src/transformation/file/options/option-manager.js 86.7% <0%> (+3.22%)
packages/babel-traverse/src/path/replacement.js 77.39% <0%> (+3.94%)
...ckages/babel-core/src/transformation/file/index.js 90.37% <0%> (+5.62%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01b250a...752788f. Read the comment docs.

@hzoo
Copy link
Member

hzoo commented Mar 10, 2017

Maybe we should actually add the generated files to git so we can actually compare what's happening.. Don't know how we can write tests

@hzoo
Copy link
Member

hzoo commented Mar 10, 2017

Yeah it would be nice if we could diff previous code or diff the options somehow

@xtuc
Copy link
Member

xtuc commented Mar 10, 2017

Could we add it as a snapshot in the tests?

@Jessidhia
Copy link
Member Author

I tried committing the files to git, but it would end up creating a huge commit adding 300+ files, and at least 4 of them would need to be updated every single time a helper got changed 😢

I added fixtures for the transform-runtime output, but no idea how babel-runtime could be tested. Any exec test that would pass with transform-runtime would also pass without using transform-runtime at all, and it's not possible to exec-test the useESModules output.


function buildRuntimeRewritePlugin(relativePath, helperName) {
return {
pre: function (file) {
var original = file.get("helperGenerator");
file.set("helperGenerator", function(name) {
// make sure that helpers won't insert circular references to themselves
if (name === helperName) return;
if (name === helperName) return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why false?

? `export { default } from \"./${helperName}.js\";`
: "module.exports = require(\"./" + helperName + ".js\");";
writeFile(`${dirname}_${helperAlias}.js`, content);
if (helperAlias !== helperName) writeFile(`${dirname}${helperAlias}.js`, content);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should really kill all these aliases in 7.x. If you're up for it, maybe a PR another this one? Let's keep this separate so if we want to backport it is easier.

@loganfsmyth
Copy link
Member

This looks awesome to me. The only case that comes to mind is that it's unfortunate people using useBuiltins still have to depend on core-js even though they aren't using it.

Let's land this as-is, but it might be good for us to think about if we want to explore making a separate module for "only helpers" and one for "helpers with polyfill" or something.

In the long run I'd love to have the helpers currently living in babel-helpers instead live inside their respective plugins, which would allow us to repurpose babel-helpers. It's a clear name for something like that.

@hzoo
Copy link
Member

hzoo commented Mar 15, 2017

preset-env's useBuiltIns option outputs core-js which isn't very intuitive so it would be nice for polyfill to just have the modules. Would the transform easier too cc @zloirock

@Jessidhia
Copy link
Member Author

Jessidhia commented Mar 15, 2017

Yeah, I took the useBuiltIns name from preset-env.

@loganfsmyth the point of useBuiltIns is specifically to not depend on core-js and just use whatever is in the environment, but I wouldn't be surprised if some helpers only work with either native or core-js's implementation.

EDIT: oh, you mean literally the npm dependency, rather than the runtime dependency.

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 6, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 6, 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: New Feature 🚀 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

6 participants