Skip to content
This repository has been archived by the owner on May 11, 2018. It is now read-only.

add useBuiltIns option #56

Merged
merged 21 commits into from
Dec 9, 2016
Merged

add useBuiltIns option #56

merged 21 commits into from
Dec 9, 2016

Conversation

hzoo
Copy link
Member

@hzoo hzoo commented Dec 2, 2016

Ref #20

TD;LR:

turns

import 'babel-polyfill'; // all of the polyfill

into

import "core-js/modules/es6.object.assign"; // required individual modules
import "core-js/modules/...";`
...

based on supported environment

screen shot 2016-12-01 at 8 34 51 pm

cc @zloirock, @chicoxyzzy, @Kovensky, @keyanzhang, @ljharb

compat-table data -> data/builtInFeatures.js (mapping core-js module to builtins) -> babel plugin that turns import babel-polyfill into individual imports



data/builtInFeatures.js is similar to data/pluginFeatures.js

We try to match the core-js module name with the corresponding test name in the compat-table.

"es6.typed.int8-array": "typed arrays / Int8Array",

Example:

screen shot 2016-12-02 at 5 09 10 pm

"es6.object.assign": "Object static methods / Object.assign",
"es6.object.is": "Object static methods / Object.is",

import 'babel-polyfill' -> import "core-js/modules/es6.object.assign";

@hzoo hzoo force-pushed the builtins-option branch 2 times, most recently from 80be6c7 to 2c3e12a Compare December 2, 2016 21:03
@@ -0,0 +1,716 @@
{
"es6.typed.data-view": {},
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to fix this

}

function addImport(polyfill) {
let declar = t.importDeclaration([], t.stringLiteral(`core-js/modules/${polyfill}`));
Copy link
Member Author

Choose a reason for hiding this comment

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

Should think about replacing with babel-polyfill/ if we are going to generate those files instead of core-js.

@hzoo
Copy link
Member Author

hzoo commented Dec 2, 2016

via @zloirock

symbol/to-string-tag -> es6.object.to-string
symbol/match and related -> es6.regexp.match etc
es6.array.iterator for array.prototype[values|keys|entries]

@@ -0,0 +1,120 @@
// https://github.com/zloirock/core-js

/* eslint-disable quotes */
Copy link
Member

Choose a reason for hiding this comment

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

You can just set it with avoidEscape

http://eslint.org/docs/rules/quotes#options

plugin.opera = plugin.chrome - 13;

if (plugin.chrome === 5) {
plugin.opera = 12;
Copy link
Member

@Jessidhia Jessidhia Dec 4, 2016

Choose a reason for hiding this comment

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

This check is confusing, and probably doesn't do what it intended to.

Maybe if (plugin.chrome && plugin.chrome >= 28), with the special case for chrome 5 later?

Opera 12 was on Presto, Opera 15 was the first Blink release; no idea about the in-betweens :D

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it's just b/c I found opera 12/chrome 5 hardcoded but there weren't any opera 13,14 etc

Copy link
Member

Choose a reason for hiding this comment

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

Because of how the statement in line 140 sets opera to chrome - 13, you might get opera -12, opera -11, ..., opera 14 (assuming there is data for chrome older than 28) :X

Copy link
Member Author

@hzoo hzoo Dec 5, 2016

Choose a reason for hiding this comment

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

I just meant I only saw 12 as a older browser number and nothing else so you wouldn't get a negative so it works fine. But yeah we should change it anyway since it's better logic 😄

maybe

 if (plugin.chrome) {
        if (plugin.chrome >= 28) {
          plugin.opera = plugin.chrome - 13;
        } else if (plugin.chrome === 5) {
          plugin.opera = 12;
        }
      }

// core-js/fn/map

// "es6.typed/array-buffer": "typed arrays / ",
"es6.typed.data-view": "typed arrays / DataView",
Copy link
Member Author

@hzoo hzoo Dec 5, 2016

Choose a reason for hiding this comment

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

this should cover

typed arrays / DataView (Int8)
typed arrays / DataView (Uint8)
typed arrays / DataView (Int16)
typed arrays / DataView (Uint16)
typed arrays / DataView (Int32)
typed arrays / DataView (Uint32)
typed arrays / DataView (Float32)
typed arrays / DataView (Float64)

// es2015
// core-js/fn/map

// "es6.typed/array-buffer": "typed arrays / ",
Copy link
Member Author

@hzoo hzoo Dec 5, 2016

Choose a reason for hiding this comment

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

I think these would be covered with all the int8-array ones below

typed arrays / ArrayBuffer[Symbol.species] typed arrays / constructors require new typed arrays / constructors accept generic iterables typed arrays / correct prototype chains typed arrays / %TypedArray%.from typed arrays / %TypedArray%.of typed arrays / %TypedArray%.prototype.subarray typed arrays / %TypedArray%.prototype.join typed arrays / %TypedArray%.prototype.indexOf typed arrays / %TypedArray%.prototype.lastIndexOf typed arrays / %TypedArray%.prototype.slice typed arrays / %TypedArray%.prototype.every typed arrays / %TypedArray%.prototype.filter typed arrays / %TypedArray%.prototype.forEach typed arrays / %TypedArray%.prototype.map typed arrays / %TypedArray%.prototype.reduce typed arrays / %TypedArray%.prototype.reduceRight typed arrays / %TypedArray%.prototype.reverse typed arrays / %TypedArray%.prototype.some typed arrays / %TypedArray%.prototype.sort typed arrays / %TypedArray%.prototype.copyWithin typed arrays / %TypedArray%.prototype.find typed arrays / %TypedArray%.prototype.findIndex typed arrays / %TypedArray%.prototype.fill typed arrays / %TypedArray%.prototype.keys typed arrays / %TypedArray%.prototype.values typed arrays / %TypedArray%.prototype.entries typed arrays / %TypedArray%.prototype[Symbol.iterator] typed arrays / %TypedArray%[Symbol.species]

"es6.array.find-index": "Array.prototype methods / Array.prototype.findIndex",
"es6.array.fill": "Array.prototype methods / Array.prototype.fill",
"es6.array.iterator": "Array.prototype methods / Array.prototype.keys",
// "es6.array.iterator": "Array.prototype methods / Array.prototype.values",

Choose a reason for hiding this comment

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

perhaps a note about these


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

> NOTE: This means you should only use `require("babel-polyfill");` once in your whole app. I would recommend just creating a single file that only does the require.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I generally prefer not to use "I" in readmes unless there's a reason you want it to be explicit that you, the author, are saying something. Maybe something like, "Creating a single file that only does contains the require is recommended."?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 ok or "One option is to "

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I like that!

@@ -0,0 +1,13 @@
require("core-js/modules/es6.typed.data-view");

Copy link
Member

Choose a reason for hiding this comment

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

Curious why these have newlines between them

Copy link
Member Author

@hzoo hzoo Dec 5, 2016

Choose a reason for hiding this comment

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

me too lol.. probably a generator newline issue @loganfsmyth

@@ -0,0 +1,75 @@
const polyfillSource = "babel-polyfill";
Copy link
Member Author

@hzoo hzoo Dec 5, 2016

Choose a reason for hiding this comment

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

Oh I guess it should probably work on import "core-js" too..

and right now it's weird that it would be change import "babel-polyfill" to "core-js" (since babel-polyfill doesn't have any generated modules atm anyway..)

But it should work on npm 3..

"es6.promise": "Promise",
"es6.symbol": "Symbol",

"es6.symbol": "well-known symbols / Symbol.hasInstance",
Copy link
Member Author

Choose a reason for hiding this comment

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

@@hasInstance - works with babel optional transform + core-js
@@isConcatSpreadable - can be polyfilled, but for some performance reason I didn't add it before. Maybe if I'll find some time in the future...
@@iterator - works with core-js and some babel transforms.
@@match - works with core-js
@@replace - works with core-js
@@search - works with core-js
@@species - in some places works with core-js
@@split - works with core-js
@@toPrimitive - can't be completely polyfilled / transpiled
@@toStringTag - works with core-js
@@unscopables - theoretically, can be transpiled, but it should work in sloppy mode only

from zloirock on slack

@hzoo hzoo force-pushed the builtins-option branch 2 times, most recently from 7a4c064 to c95759d Compare December 5, 2016 21:37
"safari": 10,
"ios": 10
},
"es6.reflect.apply": {},
Copy link
Member

Choose a reason for hiding this comment

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

Missing data

"safari": 10,
"ios": 10
},
"es6.reflect.own-keys": {},
Copy link
Member

Choose a reason for hiding this comment

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

Also missing data

"safari": 10,
"ios": 10
},
"es6.reflect.set-prototype-of": {
Copy link
Member

Choose a reason for hiding this comment

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

What happens in, say, IE10, where it's not polyfillable?

EDIT: ah, it'll just check if it would have been a valid prototype and then return false instead.

"safari": 10,
"ios": 10
},
"es6.symbol.iterator": {},
Copy link
Member

Choose a reason for hiding this comment

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

Missing data

"node": 6,
"ios": 10
},
"es6.symbol.species": {},
Copy link
Member

Choose a reason for hiding this comment

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

Missing data

"node": 0.12,
"ios": 9
},
"es6.object.set-prototype-of": {
Copy link
Member

Choose a reason for hiding this comment

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

Is this really polyfillable? Or is it for a possible corner case where __proto__ works but setPrototypeOf doesn't?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right, __proto__ used to be a gecko extension

@hzoo
Copy link
Member Author

hzoo commented Dec 6, 2016

@Kovensky #62 fixes missing data

@Jessidhia
Copy link
Member

Jessidhia commented Dec 7, 2016

core-js/shim also includes web.timers (only needed for IE <= 9?), web.immediate (experimental?) and web.dom.iterable (needed to be able to use DOM collections with for..of).

@hzoo
Copy link
Member Author

hzoo commented Dec 7, 2016

Should we just auto include all the experimental ones for backwards compat? Seems weird to do

@Jessidhia
Copy link
Member

Jessidhia commented Dec 7, 2016

At least web.dom.iterable is certainly not experimental, and definitely should be included for DOM/CSSOM compliance, but it also doesn't need to be included in node-only builds (unless you expect to patch jsdom?).

The problem with web.dom.iterable, though, is that I don't know what data can be used to tell which browsers support it 😕

Webpack also can provide setImmediate on its own, so most people using setImmediate with webpack + babel-polyfill are not actually using core-js's version unless they put node: { setImmediate: false } in their webpack config.

I'm also not quite sure on including experimentals. On one hand, env tries to provide latest-level syntax, but should it really be providing things that go beyond latest if they are API? If they are experimental, how do you know when/where to omit them?

On the other hand, you might break people that were using the experimental APIs without noticing. Adding an experimental option could work, but that could be interpreted to mean stage-(n < 4) features...

@hzoo
Copy link
Member Author

hzoo commented Dec 7, 2016

Yeah I would like to do add any experimentals. So you think web.dom.iterable is the only one we should also add @Kovensky?

or i guess all the web ones

require('./modules/web.timers');
require('./modules/web.immediate');
require('./modules/web.dom.iterable');

"es6.symbol": "well-known symbols / Symbol.split",
"es6.symbol": "well-known symbols / Symbol.toPrimitive",
"es6.symbol": "well-known symbols / Symbol.toStringTag",
"es6.symbol": "well-known symbols / Symbol.unscopables",
Copy link
Member

Choose a reason for hiding this comment

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

Duplicated object keys is not a problem?

Copy link
Member Author

@hzoo hzoo Dec 7, 2016

Choose a reason for hiding this comment

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

Wow good point it is..

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe just should remove that section

Copy link
Member

Choose a reason for hiding this comment

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

As I wrote, for me, the structure like "core-js.module.name": ["test 1", "test 2", ...] looks like the best option here.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe to prevent hardcoding just use it with RegExp:
For s6.symbol: /(^|\s)Symbol(\.\w+)?$/ will match Symbol and / Symbol.method.
Or /Array.prototype.(keys|entries)$/ for es6.array.iterator.
But It might cause conflicts in the future. Needs really accurate RegExps.

Copy link
Member

Choose a reason for hiding this comment

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

Many modules should have in requirements some tests from different categories, so just a list of tests should be better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright yeah I thought it was be "cool" to do it this way but having it be the same as pluginFeatures is good

"es6.array.iterator": "Array.prototype methods / Array.prototype.keys",
// can use Symbol.iterator, not implemented in many environments
// "es6.array.iterator": "Array.prototype methods / Array.prototype.values",
"es6.array.iterator": "Array.prototype methods / Array.prototype.entries",
Copy link
Member

Choose a reason for hiding this comment

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

Just a note, another case of dupe keys similar to es6.symbol

"es6.set": "Set",
"es6.weak-map": "WeakMap",
"es6.weak-set": "WeakSet",
// proxy not implemented
Copy link
Member

Choose a reason for hiding this comment

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

Rather, not implementable 😢

Copy link
Member Author

Choose a reason for hiding this comment

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

right bad wording haha

"es6.string.ends-with": "String.prototype methods / String.prototype.endsWith",
"es6.string.includes": "String.prototype methods / String.prototype.includes",

"es6.regexp.flags": "RegExp.prototype properties / RegExp.prototype.flags",
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it can return "u" for (transformed) regexps like /./u.

Probably not actually possible to attach the flag to non-ES6-native regexps :(

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -98,7 +114,7 @@ const getLowestImplementedVersion = ({ features }, env) => {
});

let envFiltered = envTests.filter((t) => t);
if (envTests.length > envFiltered.length) {
if (envTests.length > envFiltered.length || envTests.length === 0) {
Copy link
Member

@Jessidhia Jessidhia Dec 9, 2016

Choose a reason for hiding this comment

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

!envTests.every(Boolean)?

Copy link
Member Author

Choose a reason for hiding this comment

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

will just leave it for now

export default function buildPreset(context, opts = {}) {
const loose = validateLooseOption(opts.loose);
const moduleType = validateModulesOption(opts.modules);
const whitelist = validateWhitelistOption(opts.whitelist);
const targets = getTargets(opts.targets);
const debug = opts.debug;
const useBuiltIns = opts.useBuiltIns;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe something more descriptive like simplifyPolyfill? Or is simplifying the core-js / babel-polyfill import an implementation detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kaicataldo asked about this as well.

It is an implementation + it's just the name we used before that's all.

babel/babel#3327 and we went with useBuiltIns

@hzoo
Copy link
Member Author

hzoo commented Dec 9, 2016

Alright lets give it a shot and move forward - will fix if we find any issues.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants