Breaking: make current "useBuiltIns" auto import only used + necessary polyfills per file #241

hzoo · 2017-03-31T19:52:28Z

https://twitter.com/left_pad/status/847911365645938690

TL;DR: automatically add polyfills based on actual usage in the file (check the presence of Promise), and remove any polyfills supported by the target environment (Chrome 55 already supports Promise). Check the tests in this pr for more examples!

Size savings: if you don't use any polyfills in your code: then none will be added. If you do use polyfills, only the ones not supported in your target environment will be added.

Examples

Older browser (like IE): imports are added

Modern browser (Chrome 58): no imports added

astexplorer live example (only the adding part): http://astexplorer.net/#/gist/dc031f702921a727ec5f82c2bc3341b0/6b87729fb2ed77e8512c0b137e8a3c19dbcdc63c

The only issue not covered in #84 (comment) is regarding the rest of node_modules (3rd party).

Basically the issue is that if you don't use Promise in any of the code that you run Babel on, this option doesn't know to include the Promise polyfill if a library you use needs it.

A workaround is a disclaimer of the option and to use includes instead. We can even warn if you include but it's natively supported.

Not sure about the option name, or if we should making a breaking change for it. It does seem like this should be opt-in since it doesn't know about the use of polyfills in 3rd party modules unless you run babel over those too

more tests (suggest some more?)
change the console.warn on instance methods to be part of the debug flag?
change option to be useBuiltIns, and move current option to be useBuiltIns: "entry"
fix import not being transform (due to use of Program.exit, should change to adding the imports in the visitors themselves)
Can we track VariableDeclaration with StringLiteral matched to built-in definition and in case of a[trackedVariable] include built-in according to StringLiteral value?

codecov-io · 2017-03-31T19:52:59Z

Codecov Report

❗ No coverage uploaded for pull request base (2.0@c50d330). Click here to learn what that means.
The diff coverage is 77.14%.

@@          Coverage Diff           @@
##             2.0     #241   +/-   ##
======================================
  Coverage       ?   87.67%           
======================================
  Files          ?        6           
  Lines          ?      284           
  Branches       ?       99           
======================================
  Hits           ?      249           
  Misses         ?       17           
  Partials       ?       18

Impacted Files	Coverage Δ
src/transform-polyfill-require-plugin.js	`94.87% <ø> (ø)`
src/normalize-options.js	`87.09% <ø> (ø)`
src/index.js	`95.61% <100%> (ø)`
src/built-in-definitions.js	`100% <100%> (ø)`
src/add-used-built-ins-plugin.js	`75.51% <75.51%> (ø)`

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 c50d330...e68a7aa. Read the comment docs.

hzoo · 2017-03-31T19:53:23Z

test/fixtures/preset-options-add-used-built-ins/test-modules-tranform/expected.js

@@ -0,0 +1,4 @@
+"use strict";
+
+import "core-js/modules/es6.promise";


bug: not transformed due to program.exit

hzoo · 2017-03-31T19:55:06Z

src/built-in-definitions.js

@@ -0,0 +1,108 @@
+export const definitions = {


This is basically the opposite of built-in-features.js so maybe can just reverse it somehow

hzoo · 2017-03-31T19:56:09Z

src/index.js

    plugins.push([transformPolyfillRequirePlugin, { polyfills, regenerator }]);
+  } else if (addUsedBuiltIns) {
+    plugins.push([addUsedBuiltInsPlugin, { polyfills, regenerator }]);


right now it's a separate option/plugin/file to make it easier to look at

yavorsky · 2017-04-01T12:19:06Z

Can we track VariableDeclaration with StringLiteral matched to built-in definition and in case of a[trackedVariable] include built-in according to StringLiteral value?

hzoo · 2017-04-03T21:22:46Z

Yeah we can just dono if it's worth doing it

hzoo · 2017-04-04T18:01:21Z

new idea:

Make this the default behavior in 2.0 (useBuiltIns: true) which is "aggressive" in removing polyfills that aren't used in the files. Another option would be the current useBuiltIns which only removes polyfills already available in the environment. And lastly useBuiltIns: false which doesn't do anything.

We need to make sure that babel-polyfill/core-js is included as a dependency though which I'm not sure how to do other than making a runtime error if it's not defined.

…t core-js return babel-polyfill require instead of core-js

hzoo · 2017-04-04T21:47:27Z

9f754cf just appends babel-polyfill/ so it requires the same module (otherwise it requires you to have core-js in your dependencies when likely you have babel-polyfill or the wrong version of core-js)

existentialism · 2017-04-04T21:55:16Z

README.md

+
+> NOTE: Only use `require("babel-polyfill");` once in your whole app. One option is to create a single entry file that only contains the require statement.
+
+This option enables a new plugin that replaces the statement `import "babel-polyfill"` or `require("babel-polyfill")` with individual requires for `babel-polyfill` based on environment.


Worth adding a note this was the behavior in 1.x?

Eh, maybe not and just put it in upgrade guide.

existentialism · 2017-04-04T21:56:58Z

src/add-used-built-ins-plugin.js

+        state.opts.addUsedBuiltIns &&
+          console.warn(
+            `
+Adding "import 'babel-polyfill'" isn't necessary with the addUsedBuiltIns option anymore.


addUsedBuiltIns -> useBuiltIns: true (same on L84)

… true on default

hzoo · 2017-04-05T22:06:47Z

src/normalize-options.js

@@ -76,6 +76,6 @@ export default function normalizeOptions(opts) {
    loose: validateLooseOption(opts.loose),
    moduleType: validateModulesOption(opts.modules),
    targets: opts.targets,
-    useBuiltIns: opts.useBuiltIns,
+    useBuiltIns: opts.useBuiltIns === undefined ? true : opts.useBuiltIns,


breaking: default to true!

Maybe convert this to a destructure with a default?

Along with opts.target and opts.debug

We may also want to validate valid useBuiltIn values (false|true|'entry')

hzoo · 2017-04-05T22:07:16Z

src/use-built-ins-entry-plugin.js

@@ -1,5 +1,5 @@
 function isPolyfillSource(value) {
-  return value === "babel-polyfill" || value === "core-js";


since the 7.0 babel-polyfill will have core-js modules in it we can just target that

mikesherov

Cool!

mikesherov · 2017-04-06T13:26:17Z

README.md

 npm install babel-polyfill --save
 ```

-This option enables a new plugin that replaces the statement `import "babel-polyfill"` or `require("babel-polyfill")` with individual requires for `babel-polyfill` based on environment.
+#### `useBuiltIns: true`


Are you missing docs for useBuiltIns: false now? May seem obvious, but now that it's not the default you should at least describe the option.

mikesherov · 2017-04-06T13:27:17Z

src/index.js

-  useBuiltIns &&
-    plugins.push([transformPolyfillRequirePlugin, { polyfills, regenerator }]);
+  if (useBuiltIns === true) {
+    plugins.push([addUsedBuiltInsPlugin, { polyfills, regenerator, debug }]);


stray debug?

The debug flag is used in the plugin =>

babel-preset-env/src/use-built-ins-plugin.js

Line 142 in ebf77d8

state.opts.debug && warnOnInstanceMethod(getObjectString(node));

Maybe we don't want to have those warnings, not sure if it's useful or not but we can iterate

mikesherov · 2017-04-06T13:28:22Z

src/normalize-options.js

@@ -76,6 +76,6 @@ export default function normalizeOptions(opts) {
    loose: validateLooseOption(opts.loose),
    moduleType: validateModulesOption(opts.modules),
    targets: opts.targets,
-    useBuiltIns: opts.useBuiltIns,
+    useBuiltIns: opts.useBuiltIns === undefined ? true : opts.useBuiltIns,


Maybe convert this to a destructure with a default?

mikesherov · 2017-04-06T13:28:43Z

src/normalize-options.js

@@ -76,6 +76,6 @@ export default function normalizeOptions(opts) {
    loose: validateLooseOption(opts.loose),
    moduleType: validateModulesOption(opts.modules),
    targets: opts.targets,
-    useBuiltIns: opts.useBuiltIns,
+    useBuiltIns: opts.useBuiltIns === undefined ? true : opts.useBuiltIns,


Along with opts.target and opts.debug

mikesherov · 2017-04-06T13:39:00Z

src/use-built-ins-plugin.js

+          t.isIdentifier(prop) &&
+          has(definitions.instanceMethods, prop.name)
+        ) {
+          state.opts.debug && warnOnInstanceMethod(getObjectString(node));


Maybe move debug check into warnOnInstanceMethod?

mikesherov · 2017-04-06T13:40:39Z

src/use-built-ins-plugin.js

+  function addUnsupported(path, polyfills, builtIn, builtIns) {
+    if (Array.isArray(builtIn)) {
+      for (const i of builtIn) {
+        if (polyfills.indexOf(i) !== -1) {


Maybe polyfills could be a Set so you get O(1) lookups instead of O(n)? Might be premature optimization.

It's passed in as an array, so can turn it into a set before passing it down

Might be better to review using a Set throughout the plugin in 2.x?

mikesherov · 2017-04-06T13:41:57Z

src/use-built-ins-plugin.js

+          if (isRequire(bodyPath)) {
+            console.warn(
+              `
+Adding "require('babel-polyfill')" isn't necessary with the useBuiltIns option anymore.


Avoid "anymore". This message is valid for new and old users. To users it never was necessary

mikesherov · 2017-04-06T13:52:37Z

src/use-built-ins-plugin.js

+          const builtIn = definitions.instanceMethods[prop.name];
+          addUnsupported(path, state.opts.polyfills, builtIn, this.builtIns);
+        } else if (
+          node.computed &&


This is very cool that it catches: b['find'] but it won't catch var c = 'find'; b[c];.

And I know it's literally impossible to statically guarantee that you'll find all cases because of stuff like var c = 'fi' + 'nd'; b[c];

However, maybe it makes sense to iterate over all string literals and if any match an instance method name, include the polyfill?

I only say that because it's far more likely to see: b[someCondition ? 'find' : 'findIndex'] and you never see the literal string form because you can use non computed always in that case.

Ok #241 (comment). Yeah we could check strings.

I'l use path.evaluate

mikesherov · 2017-04-06T18:30:54Z

src/use-built-ins-plugin.js

+            const res = path.get("property").evaluate();
+            if (res.confident) {
+              const builtIn = definitions.instanceMethods[res.value];
+              warnOnInstanceMethod(state, `${obj.name}['${prop.value}']`);


should this be res.value?

yeah forgot to change 👍

existentialism · 2017-04-06T21:35:57Z

src/use-built-ins-plugin.js

-When setting 'useBuiltIns: true', polyfills are automatically imported when needed.
-Please remove the call or use 'useBuiltIns: "entry"' instead.
+When setting "useBuiltIns: true", polyfills are automatically imported when needed.
+Please remove the "import 'babel-polyfill'" call or use "useBuiltIns: 'entry'" instead.


Should be "require('babel-polyfill')"

lol almost forgot why it was 2 messages in the first place then

yavorsky · 2017-04-07T00:08:59Z

README.md

@@ -176,21 +174,60 @@ This option is useful for "blacklisting" a transform like `transform-regenerator

 ### `useBuiltIns`

-`boolean`, defaults to `false`.
+`boolean`, defaults to `true`.


maybe consider with entry option?

what do you mean? I have #### useBuiltIns: 'entry' below

entry is a string, maybe boolean | "entry"?

existentialism · 2017-04-07T15:27:59Z

👏

hzoo added the Tag: New Feature label Mar 31, 2017

hzoo commented Mar 31, 2017

View reviewed changes

hzoo mentioned this pull request Mar 31, 2017

useBuiltIns: figure out how to remove polyfills that aren't actually used in the codebase #84

Closed

hzoo force-pushed the add-used-built-ins branch 2 times, most recently from b96525c to 761a146 Compare March 31, 2017 20:35

hzoo changed the base branch from master to 2.0 April 4, 2017 18:12

hzoo added 3 commits April 4, 2017 14:16

create add-used-built-ins option

7741183

add instance method tests

ae10e58

remove unncessary check

ca4ae1f

hzoo mentioned this pull request Apr 4, 2017

Move transform-runtime functionality into this preset #246

Closed

hzoo force-pushed the add-used-built-ins branch from 761a146 to ca4ae1f Compare April 4, 2017 18:34

hzoo mentioned this pull request Apr 4, 2017

Meta: Polyfills + Babel helpers #248

Closed

7 tasks

Breaking: account for babel/babel#5584, only run on babel-polyfill no…

9f754cf

…t core-js return babel-polyfill require instead of core-js

existentialism reviewed Apr 4, 2017

View reviewed changes

useBuiltIns: true is now useBuiltIns: entry, and turn on useBuiltIns:…

bf6383c

… true on default

hzoo force-pushed the add-used-built-ins branch from 7f70591 to bf6383c Compare April 5, 2017 02:37

use error messages, only log instance methods in debug

f23908f

hzoo changed the title ~~[WIP] create add-used-built-ins option~~ Breaking: make current "useBuiltIns" auto import only used + necessary polyfills per file Apr 5, 2017

hzoo commented Apr 5, 2017

View reviewed changes

hzoo added this to the 2.0 milestone Apr 5, 2017

add imports as built-ins are found instead of at program exit

ebf77d8

mikesherov reviewed Apr 6, 2017

View reviewed changes

hzoo added 4 commits April 6, 2017 11:21

validate useBuiltIns against true,false,entry and test

1280f2c

fix warning + readme [skip ci]

0086568

use a Set

1db2446

move debug check

dacb669

hzoo force-pushed the add-used-built-ins branch from 2ffd850 to fc13a54 Compare April 6, 2017 18:28

mikesherov reviewed Apr 6, 2017

View reviewed changes

account for computed variables in properties

1072ca9

hzoo force-pushed the add-used-built-ins branch from fc13a54 to 1072ca9 Compare April 6, 2017 18:32

update plugins to latest alpha

c7abb31

existentialism reviewed Apr 6, 2017

View reviewed changes

hzoo force-pushed the add-used-built-ins branch from 027c4cd to 17db266 Compare April 6, 2017 21:38

yavorsky reviewed Apr 7, 2017

View reviewed changes

existentialism mentioned this pull request Apr 7, 2017

Add debug option if env preset selected. babel/website#1225

Merged

fix warning, readme [skip ci]

e68a7aa

hzoo force-pushed the add-used-built-ins branch from 17db266 to e68a7aa Compare April 7, 2017 14:25

hzoo mentioned this pull request Apr 7, 2017

Babel polyfills Shopify/javascript#231

Open

hzoo merged commit 2fc5811 into 2.0 Apr 7, 2017

existentialism mentioned this pull request Apr 11, 2017

Use Sets for polyfills and transformations #274

Merged

hzoo mentioned this pull request Apr 13, 2017

Discussion: useBuiltIns (usage only, not "entry" point method) #284

Closed

aduth mentioned this pull request May 10, 2017

Use a Babel polyfill script WordPress/gutenberg#746

Closed

yavorsky mentioned this pull request Jul 5, 2017

Debug option reports unincluded stage-x polyfills #369

Closed

hzoo deleted the add-used-built-ins branch July 12, 2017 03:01

cliveb mentioned this pull request Oct 21, 2017

v2 doesn't work on Safari gdg-x/hoverboard#317

Closed

idiotWu mentioned this pull request Oct 25, 2017

Import specific polyfills Grsmto/simplebar#120

Closed

This was referenced Oct 30, 2017

Discussion: useBuiltIns (usage only, not "entry" point method) babel/babel#6626

Closed

Meta: Polyfills + Babel helpers babel/babel#6628

Closed

Move transform-runtime functionality into this preset babel/babel#6629

Closed

creeperyang mentioned this pull request Dec 19, 2017

了解 Babel 6 & 7 生态 creeperyang/blog#25

Open

		@@ -0,0 +1,4 @@
		"use strict";

		import "core-js/modules/es6.promise";


		> NOTE: Only use `require("babel-polyfill");` once in your whole app. One option is to create a single entry file that only contains the require statement.

		This option enables a new plugin that replaces the statement `import "babel-polyfill"` or `require("babel-polyfill")` with individual requires for `babel-polyfill` based on environment.

		@@ -1,5 +1,5 @@
		function isPolyfillSource(value) {
		return value === "babel-polyfill" \|\| value === "core-js";

Breaking: make current "useBuiltIns" auto import only used + necessary polyfills per file #241

Breaking: make current "useBuiltIns" auto import only used + necessary polyfills per file #241

Conversation

hzoo commented Mar 31, 2017 • edited Loading

Examples

Older browser (like IE): imports are added

Modern browser (Chrome 58): no imports added

codecov-io commented Mar 31, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yavorsky commented Apr 1, 2017

hzoo commented Apr 3, 2017

hzoo commented Apr 4, 2017 • edited Loading

hzoo commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

existentialism Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

mikesherov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yavorsky Apr 7, 2017 • edited Loading

Choose a reason for hiding this comment

existentialism commented Apr 7, 2017

hzoo commented Mar 31, 2017 •

edited

Loading

codecov-io commented Mar 31, 2017 •

edited

Loading

hzoo commented Apr 4, 2017 •

edited

Loading

existentialism Apr 4, 2017 •

edited

Loading

hzoo Apr 5, 2017 •

edited

Loading

yavorsky Apr 7, 2017 •

edited

Loading