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 babel-preset-env option (fix #843) #878

Merged
merged 6 commits into from
Mar 28, 2018

Conversation

samsch
Copy link
Contributor

@samsch samsch commented Feb 21, 2018

Enables babel-preset-env useBuiltIns option for source packages (and not node_modules packages). Fix for #843

No tests were harmed in the making of this pull request. (Prettier got excited and added a newline though.)

I added babel-polyfill as an explicit devDependency for a test.

As per any test dependent on external sources (browserlist), this isn't perfectly future-proof.

@codecov-io
Copy link

Codecov Report

Merging #878 into master will decrease coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #878      +/-   ##
=========================================
- Coverage   91.98%   91.7%   -0.28%     
=========================================
  Files          68      68              
  Lines        3404    3494      +90     
=========================================
+ Hits         3131    3204      +73     
- Misses        273     290      +17
Impacted Files Coverage Δ
src/transforms/babel.js 95.52% <100%> (+0.16%) ⬆️
src/assets/CSSAsset.js 83.47% <0%> (-10.13%) ⬇️
src/utils/getTargetEngines.js 96.59% <0%> (-2.15%) ⬇️
src/visitors/fs.js 89.51% <0%> (-2.09%) ⬇️
src/assets/HTMLAsset.js 97.8% <0%> (-1.3%) ⬇️
src/visitors/dependencies.js 93.5% <0%> (+0.35%) ⬆️
src/utils/config.js 91.3% <0%> (+0.82%) ⬆️
src/utils/localRequire.js 96.66% <0%> (+0.83%) ⬆️
src/assets/JSAsset.js 94.53% <0%> (+1.93%) ⬆️
... and 3 more

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 0d9d14c...bb1c00a. Read the comment docs.

@@ -239,7 +239,7 @@ async function getEnvConfig(asset, isSourceModule) {

const envCache = new Map();

async function getEnvPlugins(targets) {
async function getEnvPlugins(targets, useBuiltIns) {
Copy link

Choose a reason for hiding this comment

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

According to the babel preset-env doc, the default value of useBuiltIns is false, so what about make the second parameter of getEnvPlugins with a default false, like

async function getEnvPlugins(targets, useBuiltIns = false) { ... }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There does seem to be a little precedent to use a default argument, and it does seem to make usage a bit clearer. This does not actually change any functionality, since the argument is used for a truthy check, and undefined is falsy.

@FDiskas
Copy link

FDiskas commented Mar 21, 2018

This will fix errors like src/index.js: Unknown version 62 of android ?

@samsch
Copy link
Contributor Author

samsch commented Mar 21, 2018

@FDiskas Probably not, no. This shouldn't fix any errors, it decreases the bundle size by replaying an import of babel-polyfill with separate imports of just the necessary polyfills.

@devongovett
Copy link
Member

Thanks for doing this. When we upgrade to Babel 7, we should also switch to the new usage option for useBuiltins which makes it so you don't even need to import babel-polyfill at all.

@devongovett devongovett merged commit e26d443 into parcel-bundler:master Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants