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

Change how required built-ins are polyfilled #8727

Closed
blowery opened this issue Aug 8, 2018 · 7 comments
Closed

Change how required built-ins are polyfilled #8727

blowery opened this issue Aug 8, 2018 · 7 comments
Assignees
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take

Comments

@blowery
Copy link
Contributor

blowery commented Aug 8, 2018

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 the gutenberg npm packages. This is unexpected and can alter the runtime environment in surprising ways. Additonally, some client environments do not require these polyfills and would work without them. Bundling them increases the overall bundle size and slows down loading.

Instead, gutenberg's packages should take one of two approaches: use use babel's transform-runtime plugin to replace problematic built-in usage, or stop polyfilling altogether and declare that host environments must polyfill certain features for Gutenberg to work on downlevel browsers. The later is the direction being taken by react-dom and some other libraries that require full ES5 compliance.

Original discussion: #8057 (comment)

@designsimply designsimply added [Type] Task Issues or PRs that have been broken down into an individual action to take [Type] Build Tooling Issues or PRs related to build tooling labels Aug 8, 2018
@gziolo
Copy link
Member

gziolo commented Aug 9, 2018

This is the current state of art:

  1. We use @babel/plugin-transform-runtime when producing a distribution version of packages. See:
    https://github.com/WordPress/gutenberg/blob/master/packages/babel-preset-default/index.js#L23

  2. We also add @babel/runtime-corejs2 as a dependency of each package which is transpiled, e.g.:
    https://github.com/WordPress/gutenberg/blob/master/packages/blocks/package.json#L23

@blowery
Copy link
Contributor Author

blowery commented Aug 9, 2018

@gziolo The problematic line is https://github.com/WordPress/gutenberg/blob/master/packages/babel-preset-default/index.js#L11. That includes polyfills that affect the global scope when a given module uses language features that require a polyfill on older browers.

@blowery
Copy link
Contributor Author

blowery commented Aug 9, 2018

Here's an example of what Gutenberg outputs today.

source:

function findCarrot( arr ) {
  return arr.find( item => item.type === 'carrot' );
}

transpiled:

"use strict";

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

function findCarrot(arr) {
  return arr.find(function (item) {
    return item.type === 'carrot';
  });
}

My contention is that Gutenberg should not ship code that requires the core-js polyfill.

@gziolo
Copy link
Member

gziolo commented Aug 9, 2018

@blowery, we have just published new packages to npm which have @babel/runtime usage replaced with @babel/runtime-corejs2. Can you double check if that changes anything?

/cc @aduth @youknowriad @ntwb

@gziolo
Copy link
Member

gziolo commented Aug 9, 2018

Okey, this is still the case. I see:

import "core-js/modules/es6.array.find";

in https://unpkg.com/@wordpress/editor@2.0.0/build-module/components/navigable-toolbar/index.js

Code of this polyfill:
https://unpkg.com/core-js@2.5.7/modules/es6.array.find.js

@gziolo
Copy link
Member

gziolo commented Aug 20, 2018

Opened #9171 hoping that it will solve the issue. However I still see imports like:

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

@gziolo gziolo self-assigned this Aug 31, 2018
@gziolo
Copy link
Member

gziolo commented Sep 5, 2018

This should be resolved with the #9171 and is now available on npm.

Check related versions of updated packages in df6f8da.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling [Type] Task Issues or PRs that have been broken down into an individual action to take
Projects
None yet
Development

No branches or pull requests

3 participants