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

Packages: Change how required built-ins are polyfilled with Babel 7 #9171

Merged
merged 10 commits into from
Sep 5, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Aug 20, 2018

Description

Fixes #8727.
Closes #9478.

Discussed during Core JS meeting: https://make.wordpress.org/core/2018/08/14/javascript-chat-summary-august-14/.

Problem: The current npm packages that being built for Gutenberg polyfill usage of some browser features that are not available on older browsers. This polyfill is global and affects any client who uses WordPress npm packages. This is unexpected and can alter the runtime environment in surprising ways.

Action items:

  • corejs flag should be set to false when generating distribution version for packages and set to 2 when bundling with Webpack.
  • we should inform developers consuming WordPress packages to install proper polyfills on their own.

This PR also updates Babel to the latest RC version since we need to update all occurrences of @babel/runtime-corejs2 with @babel/runtime anyways.

I also changed the configuration for the build-module distribution output. I took advantage of the new setting for @babel/preset-env: "targets": { "esmodules": true }

We use Babel twice to produce output for packages and then use Webpack to produce build files to consume inside WordPress, so we can take advantage of it and offer build version of package which works with all browsers and build-module version which works with all browsers that support <script type="module"></script>.

How has this been tested?

npm run dev
npm run build
npm test

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

/cc @blowery @jsnajdr

@gziolo gziolo added [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling npm Packages Related to npm packages labels Aug 20, 2018
@gziolo gziolo self-assigned this Aug 20, 2018
@gziolo gziolo requested review from youknowriad, gwwar and aduth August 20, 2018 16:28
@gziolo
Copy link
Member Author

gziolo commented Aug 20, 2018

I still see imports like:

require("core-js/modules/es6.array.find");

My bet is that babel-preset-env still need some tweaks.

@ntwb
Copy link
Member

ntwb commented Aug 21, 2018

The current npm packages that being built for Gutenberg polyfill usage of some browser features that are not available on older browsers.

Would this be due to the supported @wordpress/browserslist-config browsers matrix, which is included in babel-preset-default?

browsers: [ 'extends @wordpress/browserslist-config' ],

@gziolo
Copy link
Member Author

gziolo commented Aug 21, 2018

Would this be due to the supported @wordpress/browserslist-config browsers matrix, which is included in babel-preset-default?

I don't think it's an issue, it's rather useBuiltIns: 'usage',.

@gziolo gziolo force-pushed the update/babel-corejs2 branch from 55f0e02 to 932df9a Compare August 21, 2018 07:02
@gziolo
Copy link
Member Author

gziolo commented Aug 21, 2018

I was able to tweak Babel to ensure that we won't publish any references to core-js or @babel/runtime-corejs2 polyfills to npm. I hope this aligns with what we discussed at the Core JS meeting last week.

@gziolo
Copy link
Member Author

gziolo commented Aug 21, 2018

Do you happen to know any good example of the message that could be included to inform people using our packages to install required polyfills themselves?

It also raises the questions like:

  • whether we should include polyfills for ES5 build? I assumed: no.
  • which browsers should we target for module version? I assumed: only those which support <script type="module"></script>.

@aduth
Copy link
Member

aduth commented Aug 21, 2018

the message that could be included to inform people using our packages to install required polyfills themselves?

Part of this will require identifying what features are actually in use which could require need for a polyfill.

which browsers should we target for module version?

It's tricky, since it's not only expected to be consumed by those browsers which support the script type, but also many transpilers like Webpack can use it for bundling from a dependency; in which case your package may include modern features inadvertently (because typically Babel won't be applied over an external dependency).

@gziolo
Copy link
Member Author

gziolo commented Aug 22, 2018

After reading the docs for pkg.module from Rollup and checking a few popular libraries:

I came to the conclusion that the following statement is valid:

pkg.module will point to a module that has ES2015 module syntax but otherwise only syntax features that the target environments support.

In other words, we should leave the behavior as is and transpile all features but import & export and target the same browsers for both main and module. I will update PR accordingly.

@youknowriad
Copy link
Contributor

In other words, we should leave the behavior as is and transpile all features but import & export and target the same browsers for both main and module. I will update PR accordingly.

FYI this is already what we do in master.

@gziolo gziolo force-pushed the update/babel-corejs2 branch from 932df9a to 82f0fdf Compare August 22, 2018 09:41
@gziolo
Copy link
Member Author

gziolo commented Aug 22, 2018

the message that could be included to inform people using our packages to install required polyfills themselves?

Part of this will require identifying what features are actually in use which could require need for a polyfill.

I don't think this is an easy task to accomplish although it seems like a very expected feature to be included in @babel/preset-env. See related issue in Babel repository: babel/babel#6625. I'm not quite sure what is the best way to move forward here. I don't feel like it is an easy task to manually maintain which polyfills should be included for individual packages.

In other words, we should leave the behavior as is and transpile all features but import & export and target the same browsers for both main and module. I will update PR accordingly.

FYI this is already what we do in master.

Yes, I just thought we can make it more up to date but it looks it was premature :(

@gziolo gziolo force-pushed the update/babel-corejs2 branch from 82f0fdf to 70d46ad Compare August 24, 2018 07:23
@gziolo
Copy link
Member Author

gziolo commented Aug 24, 2018

Added 70d46ad which adds the following note to all README files of packages that need them:

This package assumes that your code will run in an ES5 environment. If you're using an environment that has limited or no support for ES5 such as lower versions of IE then using @babel/polyfill will add support for these methods. Learn more about it in Babel docs.

I also upgraded Babel to 7.0.0.-rc.2.

@gziolo gziolo removed the [Status] In Progress Tracking issues with work in progress label Aug 24, 2018
@gziolo gziolo added this to the 3.7 milestone Aug 24, 2018
@gziolo gziolo requested a review from a team August 24, 2018 07:27
@gziolo gziolo force-pushed the update/babel-corejs2 branch from d70af05 to 2f25a0a Compare August 24, 2018 13:50
@aduth
Copy link
Member

aduth commented Aug 24, 2018

When we say "assumes that your code will run in an ES5 environment", are we including things like Promise... or Array#includes? These are things I expect from @babel/polyfill, but they're also not ES5: They're ES2015+.

@gziolo
Copy link
Member Author

gziolo commented Aug 27, 2018

When we say "assumes that your code will run in an ES5 environment", are we including things like Promise... or Array#includes? These are things I expect from @babel/polyfill, but they're also not ES5: They're ES2015+.

I think I mixed up a few things :) I think this message should be closer to:

This package assumes that your code will run in an ES2015+ environment. If you're using an environment that has limited or no support for ES2015+ such as lower versions of IE then using core-js or @babel/polyfill will add support for these methods. Learn more about it in Babel docs.

@jsnajdr
Copy link
Member

jsnajdr commented Aug 27, 2018

What this PR does is that it changes the transpiled output from:

import _Set from "@babel/runtime-corejs2/core-js/set";
var ATTRIBUTES_TYPES = new _Set(['string', 'boolean', 'number']);

to just:

var ATTRIBUTES_TYPES = new Set(['string', 'boolean', 'number']);

The original version makes sure that _Set exists and has all the latest methods and behaviors. That comes at a cost of having a runtime dependency on @babel/runtime-corejs2 (and transitively on core-js itself).

Another consequence that is not always desired is that the import _Set from '...' statement has side-effects: it not only returns the polyfilled _Set, but also pollutes the global namespaces with a patched Set object. One would have to use the core-js-pure version of core-js to avoid that.

The new version just uses the "naked" Set global and shifts the responsibility to the host enviroment. The host is free to satisfy the requirement however they wish. They can use core-js:

import 'core-js/library/fn/set'; // polyfills window.Set

they can use @babel/polyfill (which is just a thin wrapper around core-js anyway), they can another polyfill package or not polyfill at all: maybe the app is guaranteed to always run on latest Node or Electron that doesn't need polyfilling?

This is an approach that React takes: they use naked new Map() or requestAnimationFrame and document the environment requirements here: https://reactjs.org/docs/javascript-environment-requirements.html

How can the required enviroment be described? I'm afraid there is no simple label like "ES2015+". The sources published as part of the module are still guaranteed to have the old ES5 syntax, but the expected buit-in classes and methods can be the latest ES2019+: Promise.finally, Symbol.asyncIterator, String.padStart, ...

The precise description of that enviroment is something like: only ES5 syntax is required, but the following ES2015+ builtins are expected to be present: [enumerated list of them].

How to construct that enumerated list? Not easy. useBuiltins: 'usage' option of the env preset can help, but it's not reliable: until recently it didn't detect Promise.finally and there are many reported gaps like this one.

By the way, useBuiltins: 'usage' is a noop after this PR: the env preset is configured not to include core-js polyfills at all, so usages detected by this feature go into /dev/null.

@gziolo
Copy link
Member Author

gziolo commented Aug 28, 2018

The precise description of that enviroment is something like: only ES5 syntax is required, but the following ES2015+ builtins are expected to be present: [enumerated list of them].

How to construct that enumerated list? Not easy. useBuiltins: 'usage' option of the env preset can help, but it's not reliable: until recently it didn't detect Promise.finally and there are many reported gaps like this one.

Unfortunately, it is almost impossible to create and what's even worse to maintain the list of polyfills for 30+ packages...

By the way, useBuiltins: 'usage' is a noop after this PR: the env preset is configured not to include core-js polyfills at all, so usages detected by this feature go into /dev/null.

We transpile code twice, so useBuiltins: 'usage' is used with Webpack. You can double check in the top level build folder.

@youknowriad youknowriad added this to the 3.8 milestone Aug 30, 2018
@gziolo gziolo force-pushed the update/babel-corejs2 branch 2 times, most recently from ece002a to 39f6b7d Compare August 30, 2018 11:42
} ),
plugins: map(
plugins,
( plugin ) => overrideOptions( plugin, '@babel/plugin-transform-runtime', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain we need this plugin anymore because we're using useBuiltIns: true. Any idea @aduth In my understanding the runtime is a subset of the polyfills (added by preset-env).

Copy link
Contributor

Choose a reason for hiding this comment

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

Just read the docs https://babeljs.io/docs/en/next/babel-polyfill and https://babeljs.io/docs/en/next/babel-plugin-transform-runtime and I think it confirms my thoughts that we should just get rid of the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be using useBuiltIns: false, not true and using the transform runtime. We should not be shipping polyfills that modify global prototypes.

Copy link
Contributor

Choose a reason for hiding this comment

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

@blowery We are using useBuiltIns: false in the packages (npm) but I was referring to useBuiltIns: true used for Gutenberg.

Copy link
Contributor

Choose a reason for hiding this comment

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

This raises the question of whether we should keep the runtime transform for the packages. I personally don't see any difference between those polyfills and the remaining ones not included in the runtime (aside from the way they are included), which means I think we should remove all the polyfills and not only the ones added by babel-preset-env

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'm not quite sure. I followed the configuration from Calypso which was reviewed by @hzoo 😄

It has both @babel/preset-env with useBuiltIns: "entry" (https://github.com/Automattic/wp-calypso/blob/master/babel.config.js#L22) and @babel/plugin-transform-runtime with corejs: false (https://github.com/Automattic/wp-calypso/blob/master/babel.config.js#L51).

Copy link
Contributor

Choose a reason for hiding this comment

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

Still not sure what's the point of @babel/plugin-transform-runtime if you do corejs: false. I guess there are some helpers but aren't these also polyfills?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, there are Babel helpers that are not polyfills. When transpiling classes, for example, Babel uses helper functions with names like _classCallCheck, _possibleConstructorReturn or _inherits. The runtime transform plugin prevents them from being duplicated all the time and imports them from @babel/runtime/helpers instead.

Another thing that is not a CoreJS polyfill is the regenerator runtime used when transpiling generators and async/await.

The CoreJS and regenerator are "polyfills" in a sense that they might be provided by the platform already. Babel helpers never are though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation @jsnajdr it makes sense 👍

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gziolo gziolo force-pushed the update/babel-corejs2 branch from 39f6b7d to 6c2d121 Compare September 5, 2018 08:32
@gziolo gziolo merged commit 4abf4a7 into master Sep 5, 2018
@gziolo gziolo deleted the update/babel-corejs2 branch September 5, 2018 08:43
@aduth
Copy link
Member

aduth commented Sep 5, 2018

This package assumes that your code will run in an ES2015+ environment. If you're using an environment that has limited or no support for ES2015+ such as lower versions of IE then using core-js or @babel/polyfill will add support for these methods. Learn more about it in Babel docs.

Did we verify this assumption for Gutenberg itself? 😄 There appear to be some issues loading the editor in current master in IE11 which on preliminary testing seem to be attributable to changes here.

@aduth
Copy link
Member

aduth commented Sep 5, 2018

I can confirm this appears to have broken the editor in IE11 due to the lack of a provided polyfill, verified via the following (very-naive-definitely-not-final) fix:

diff --git a/lib/client-assets.php b/lib/client-assets.php
index 7ad3546c5..d2bcabd21 100644
--- a/lib/client-assets.php
+++ b/lib/client-assets.php
@@ -66,6 +66,11 @@ function gutenberg_get_script_polyfill( $tests ) {
 	return $polyfill;
 }
 
+function gutenberg_ie11_ಥ_ಥ() {
+	?><script src="https://cdnjs.cloudflare.com/ajax/libs/babel-polyfill/7.0.0/polyfill.min.js"></script><?php
+}
+add_action( 'admin_enqueue_scripts', 'gutenberg_ie11_ಥ_ಥ', 0 );
+
 /**
  * Registers the main TinyMCE scripts.
  */

In trying to come to a more solid solution, I'm left wondering what precisely we're expecting people to polyfill. Is it ES2015? How about things like Array#includes (ES2016) or Object.values (ES2017)?

@aduth
Copy link
Member

aduth commented Sep 5, 2018

Tracking IE11 issue for release candidate: #9640

@gziolo
Copy link
Member Author

gziolo commented Sep 6, 2018

Did we verify this assumption for Gutenberg itself? 😄 There appear to be some issues loading the editor in current master in IE11 which on preliminary testing seem to be attributable to changes here.

We should 🤦‍♂️ I assumed this is going to be transparent for Gutenberg itself...

@aduth
Copy link
Member

aduth commented Sep 6, 2018

In trying to come to a more solid solution, I'm left wondering what precisely we're expecting people to polyfill. Is it ES2015? How about things like Array#includes (ES2016) or Object.values (ES2017)?

To expand on this with an answer to my own question:

Babel's polyfill intends to provide anything which is part of a published ECMAScript specification v2015 or newer.

This will emulate a full ES2015+ environment (no < Stage 4 proposals)

https://babeljs.io/docs/en/babel-polyfill.html

This is the reason why, in #9643, I chose the name wp-polyfill-ecmascript as the script handle, as it is a forever-moving target intended to follow the latest ECMAScript specification.

@gziolo
Copy link
Member Author

gziolo commented Sep 7, 2018

This is the reason why, in #9643, I chose the name wp-polyfill-ecmascript as the script handle, as it is a forever-moving target intended to follow the latest ECMAScript specification.

As I learned yesterday based on the link shared by @youknowriad - it is meant to work that way but as always there are some exceptions. They are listed on the bottom of the page: https://github.com/zloirock/core-js#missing-polyfills

In particular:

window.fetch is not a cross-platform feature, in some environments it makes no sense. For this reason, I don't think it should be in core-js. Looking at a large number of requests it may be added in the future. Now you can use, for example, this polyfill.

When doing review, I did quick search in the source file for fetch keyword and I was surprised it isn't there 🤷‍♂️

@aduth
Copy link
Member

aduth commented Sep 7, 2018

Well, to be fair, if one assumes that babel-polyfill is a polyfill for ECMAScript, which is how I've taken it to mean, then it's entirely expected that it should not include fetch, as it's not part of any ECMAScript specification.

https://www.ecma-international.org/publications/files/ECMA-ST/Ecma-262.pdf

Rather, it's a WHATWG (and I assume W3) specification:

https://fetch.spec.whatwg.org/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
npm Packages Related to npm packages [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants