Babel 7 Take 2 #23424

samouri · 2018-03-19T22:19:06Z

Attempt 2 at #23026

Original PR Notes:

summary

Babel 7 has undergone ~40 iterations of alpha releases and another 40 of beta releases. It is very stable and has numerous features that Calypso development can benefit from. Those features include:

Increased adherence to specification. Notably this includes banning commas after a rest operator (not spread).
javascript configuration file (.babelrc.js vs. .bablrc.json)
Supports for the new React jsx shorthand for fragments: <></>
TypeScript support

Babel docs on the upgrade: http://new.babeljs.io/docs/en/next/v7-migration.htm

changes made

Switched to the new naming scheme for packages: babel-core --> @babel/core, babel-preset-env --> @babel/env, babel-preset-react --> @babel/react, babel-types --> @babel/types,
Updated babel-core to the bridge which is necessary for anything having babel as a peer dependency -- aka jest.
Manually specified the transform-class-propertites to be "loose" which mimics babel6 behavior and is much more concise than the new spec client format based on Object.defineProperty [1]
fixed a single case of comma after rest operator
switched from babelrc.json --> babelrc.js which simplifies the webpack configs.

Question for the reviewer:

is there any reason we had been explicitly removing the babel-lodash-es plugin from env.test within webpack considering it should be ignored by babel unless NODE_ENV=test

to test

docker
desktop
icfy build size
IE11

appendix
[1]: Here is an illustration of the loose and strict methods for transforming class variables.

For the following es6 code

class Chicken { animal = true }

Loose babel transformation would get us: (default in babel 6)

var Chicken = function() {
 this.animal = true;
}

Strict transformation would return more verbose code:

var Chicken = function() {
  Object.defineProperty( this, "animal", {
    value: true,
    writable: true,
    configurable: false, // has some implications about deleting from object, modifying getters setters, etc.
    enumerable: true,
  } );
}

matticbot · 2018-03-19T22:19:07Z

Test live: https://calypso.live/?branch=revert-23389-revert/babel-7

hzoo · 2018-03-22T12:57:08Z

@samouri I'd be happy to help y'all upgrade to v7, and if you want to do a video call to talk let me know. Also I started work on https://github.com/babel/babel-upgrade which may help with upgrading as well as http://new.babeljs.io/docs/en/next/v7-migration.html. I'm @left_pad on twitter.

I happened to be looking at the repo and saw the "revert" commit on the babelrc file on the main repo so I found this PR

samouri · 2018-03-23T09:28:51Z

Hi @hzoo,

First off, thank you so much for all of your work on Babel! Not only has it been an amazing tool for Calypso, the grace and openness with which you run Babel is a source of inspiration.

I think we successfully figured out a lot of the important bits as part of our first attempt: #23026 -- all of the explanations of the code changes are there. I'd still be excited to have your expert advice on our .babelrc.js file to make sure we are using the right polyfill transforms and that we are following best practices throughout!

We reverted because we noticed that the wrapNativeSuper() helper was incompatible with ie11 -- turns out we hit an unfortunate bug affecting the beta40 release that seems to have been fixed with the beta41 release. We are in the process of adding automated testing to make sure we always test pull requests in ie11 before merging which should prevent us from making the same mistake in the future.

One question for you would be: is the Babel 7 beta ready for projects like ours to use in production?

samouri · 2018-04-03T17:40:42Z

@hzoo: We'd still love to upgrade to 7 if you have time to answer our questions!

The main questions are around why is babel7 still considered a beta and if it is suitable for production projects. Create React App seems to think so.

My impression is that bugs are equally likely to come out in a post-v7 release as it is during these beta releases (we need be better at testing). That the reason babel is still in beta is so that you can you can still release breaking changes -- which usually don't affect applications. All of this is total conjecture on my part, and we'd be very appreciative of an answer!

Thanks again,
Jake

hzoo · 2018-04-04T15:42:49Z

@samouri

Yeah I think it should be ready for apps in prod. I had upgraded us at Behance to it while I was working there the whole time we were in beta. And yes your impression is correct - it's not really about bugs per se since we didn't change a lot of core things just config changes. There is one breaking change we want to do before an RC which is babel/babel#7358. Otherwise we it's ready to be released given we removed a bunch of other stuff we wanted to do (feature creep is pretty easy for a major release + oss project with all volunteers)

hzoo · 2018-04-11T02:00:51Z

.babelrc.js

+		test: {
+			plugins: [
+				'./server/bundler/babel/babel-lodash-es',
+				[ 'transform-builtin-extend', { globals: [ 'Error' ] } ],


I think this should be covered by the new PR to the classes transform itself? https://twitter.com/left_pad/status/940723982638157829

Looks right to me, thanks for looking! I've removed the unnecessary transform

The transform-builtin-extend was added in #19621 to enable extending SchemaError from Error. If I remove the transform from the Babel 6 config in master, the tests in client/state/data-layer/wpcom/concierge will start failing.

In this Babel 7 branch, these tests succeed even after the transform is gone. Therefore the removal is good and has been successfully tested 👍

hzoo · 2018-04-11T17:42:12Z

package.json

-    "babel-preset-env": "1.6.0",
-    "babel-preset-stage-2": "6.24.1",
-    "babel-register": "6.24.1",
+    "babel-register": "6.26.0",


This should be @babel/register or removed right?

After investigation it looks like we are still using babel-register in a single-location which initially made me think that switching to @babel/register would be correct. But after closer inspection I can't figure out why its using babel-register at all. I'll see if i can remove it now here: #24085

merged #24085 and removed the babel-register!

existentialism · 2018-04-11T17:43:40Z

.babelrc.js

+			{ async: isCalypsoClient && codeSplit },
+		],
+		'@babel/plugin-proposal-export-default-from',
+		'@babel/plugin-proposal-export-namespace-from',


FWIW, this is already included in stage-2

Thanks for letting me know! I'll remove it then 👍

hzoo · 2018-04-11T17:43:44Z

.babelrc.js

+		// it very slightly diverges from spec but also is more concise.
+		// see: http://new.babeljs.io/docs/en/next/v7-migration.html#babel-plugin-proposal-class-properties
+		[ '@babel/plugin-proposal-class-properties', { loose: true } ],
+		[ '@babel/plugin-transform-classes', { loose: false } ],


This was only done for backwards compat with v6? Although I guess you need it for extends Error to work I believe. Loose would be smaller, or you could just use loose for all of the transforms if you are really trying to save bytes.

. Loose would be smaller, or you could just use loose for all of the transforms if you are really trying to save bytes.

I tried that at first, but ~20 of our unit tests started failing. While they were all easily fixable, I'm worried that there are probably subtle failures that would happen within the app that we wouldn't catch for a long time

@jsnajdr built a tool called iscalypsofastyet that measures how branches affect bundle sizes. I'll do another measurement to see the exact difference and make sure it is worth losing spec compliance

This is also something I wish we could help with like showing the different options and running tests or something like that - what changes and doing a diff of the output between different builds/spec

hzoo · 2018-04-11T17:44:55Z

Also where is @babel/polyfill loaded? If you want to save on that you could use the useBuiltIns option as well https://github.com/babel/babel/tree/master/packages/babel-preset-env#usebuiltins.

blowery · 2018-04-12T01:13:57Z

Also where is @babel/polyfill loaded?

@hzoo it's not at the moment, we're just using transform-runtime and warning folks that use some es6 prototype methods. We'd like to move to using the polyfill but have not historically for size reasons.

loganfsmyth · 2018-04-12T01:17:46Z

server/bundler/babel/babel-lodash-es/index.js

@@ -1,6 +1,6 @@
 /** @format */

-const types = require( 'babel-types' );
+const types = require( '@babel/types' );


FYI, this could be deleted and moved to the line before as

module.exports = function({ types }) {

nice! I'll update that now

sirreal · 2018-04-12T10:22:37Z

client/blocks/post-share/sharing-preview-modal.jsx

@@ -15,7 +15,7 @@ const SharingPreviewModal = ( props ) => {
 	const {
 		isVisible,
 		onClose,
-		...previewProps,
+		...previewProps


Nit: I'd expect prettier to maintain this comma. File is missing /** @format */.

It shouldn't because it's "spec" behavior that we changed in Babel 7. For context, It was in a v6 patch before and it "broke" react-native so it was moved to a v7 change. And Prettier uses the Babel parser. I think it's mentioned in the description ^

Basically a rest param is always last so it doesn't need a comma

edit: link babel/babylon#149 (comment)

TIL, thanks! It is nicely mentioned in the description. I've also verified that prettier handles this correctly.

hzoo · 2018-04-12T15:01:13Z

we're just using transform-runtime and warning folks that use some es6 prototype methods. We'd like to move to using the polyfill but have not historically for size reasons.

Ok cool, that's fine. Yeah my point above ^ is that when I mean to use the polyfill it only imports a subset of the polyfill (although it probably needs better testing) to be robust. Again if y'all want to do a call or something we can chat about it.

updates since revert update to latest versions fix strange case of props rest expansion no need for transfrom-extend-builtin

samouri · 2018-04-13T16:47:42Z

Here is a comparison of some of the minified sizes of the outputted code (not gzipped) from the loose and spec versions (only applying loose to class and template-literal transforms)

You will notice that for vendor there is not a signficant difference, which makes sense considering most libraries are already transpiled and don't utilize class .

Class definitions takes up ~120 characters in spec instead of ~40 in loose. Our own code is React based and heavily uses class.

Chunk	Loose diff	Spec diff
build	+ 39 B	+ 38.9 Kb
vendors~build	+ 6.1 Kb	+ 7.9 Kb
stats	+ 459 B	+ 22.7 Kb
reader	+ 359 B	+ 5.9 Kb
posts-pages	+ 38 B	+ 3.5 Kb

source

hzoo · 2018-04-13T16:57:08Z

You will notice that for vendor there is not a signficant difference, which makes sense considering most libraries are already transpiled and don't utilize class .

Yeah, something we are trying figure out is regarding compiling node_modules/libs and how to move the ecosystem to publishing ES6+ as well as ES5. Although it's not really a Babel specific concern

jsnajdr

🐑

blowery

LGTM! Tested out fine.

samouri force-pushed the revert-23389-revert/babel-7 branch from 4da4aac to 660d82b Compare March 20, 2018 05:25

designsimply added [Status] In Progress Build labels Mar 22, 2018

hzoo mentioned this pull request Apr 4, 2018

Tracking: Tools that use @babel/core as a dep babel/babel-upgrade#20

Closed

8 tasks

samouri self-assigned this Apr 10, 2018

samouri force-pushed the revert-23389-revert/babel-7 branch 2 times, most recently from 5bc7594 to bb3a319 Compare April 10, 2018 22:18

samouri mentioned this pull request Apr 10, 2018

Babel 7 compat (take two) Automattic/wp-desktop#437

Merged

samouri requested review from jsnajdr and a team April 10, 2018 22:57

hzoo reviewed Apr 11, 2018

View reviewed changes

existentialism reviewed Apr 11, 2018

View reviewed changes

hzoo reviewed Apr 11, 2018

View reviewed changes

loganfsmyth reviewed Apr 12, 2018

View reviewed changes

sirreal reviewed Apr 12, 2018

View reviewed changes

Revert "Revert babel-7, it was breaking IE11 login."

32fa946

updates since revert update to latest versions fix strange case of props rest expansion no need for transfrom-extend-builtin

samouri force-pushed the revert-23389-revert/babel-7 branch from 4a1d6d7 to 32fa946 Compare April 12, 2018 17:41

samouri added 3 commits April 12, 2018 13:42

remove plugin-proposal-export-namespace-from

d748376

remove @babel/types

a876e36

reshrinkwrap, but this time with heart

90d305d

jsnajdr approved these changes Apr 16, 2018

View reviewed changes

blowery approved these changes Apr 16, 2018

View reviewed changes

samouri merged commit 7e38300 into master Apr 17, 2018

samouri deleted the revert-23389-revert/babel-7 branch April 17, 2018 15:12

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 17, 2018

samouri mentioned this pull request Apr 18, 2018

test circleci transform-class #24308

Merged

gziolo mentioned this pull request Jun 6, 2018

Update packages to work with Babel 7 WordPress/packages#136

Closed

gziolo mentioned this pull request Jul 9, 2018

Framework: Update code to work with Babel 7 WordPress/gutenberg#7832

Merged

jsnajdr mentioned this pull request Jun 17, 2020

Remove isCalypsoClient variable from webpack.config.js #43378

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Babel 7 Take 2 #23424

Babel 7 Take 2 #23424

samouri commented Mar 19, 2018 •

edited

Loading

matticbot commented Mar 19, 2018

hzoo commented Mar 22, 2018 •

edited

Loading

samouri commented Mar 23, 2018

samouri commented Apr 3, 2018

hzoo commented Apr 4, 2018

hzoo Apr 11, 2018

samouri Apr 11, 2018

jsnajdr Apr 11, 2018

hzoo Apr 11, 2018

samouri Apr 11, 2018

samouri Apr 12, 2018

existentialism Apr 11, 2018

samouri Apr 11, 2018

hzoo Apr 11, 2018

samouri Apr 11, 2018

samouri Apr 11, 2018 •

edited

Loading

hzoo Apr 11, 2018

hzoo commented Apr 11, 2018

blowery commented Apr 12, 2018 •

edited

Loading

loganfsmyth Apr 12, 2018

samouri Apr 12, 2018 •

edited

Loading

sirreal Apr 12, 2018

hzoo Apr 12, 2018 •

edited

Loading

sirreal Apr 12, 2018

hzoo commented Apr 12, 2018

samouri commented Apr 13, 2018 •

edited

Loading

hzoo commented Apr 13, 2018

jsnajdr left a comment

blowery left a comment

Babel 7 Take 2 #23424

Babel 7 Take 2 #23424

Conversation

samouri commented Mar 19, 2018 • edited Loading

Original PR Notes:

matticbot commented Mar 19, 2018

hzoo commented Mar 22, 2018 • edited Loading

samouri commented Mar 23, 2018

samouri commented Apr 3, 2018

hzoo commented Apr 4, 2018

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

samouri Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Apr 11, 2018

blowery commented Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

samouri Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo Apr 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzoo commented Apr 12, 2018

samouri commented Apr 13, 2018 • edited Loading

hzoo commented Apr 13, 2018

jsnajdr left a comment

Choose a reason for hiding this comment

blowery left a comment

Choose a reason for hiding this comment

samouri commented Mar 19, 2018 •

edited

Loading

hzoo commented Mar 22, 2018 •

edited

Loading

samouri Apr 11, 2018 •

edited

Loading

blowery commented Apr 12, 2018 •

edited

Loading

samouri Apr 12, 2018 •

edited

Loading

hzoo Apr 12, 2018 •

edited

Loading

samouri commented Apr 13, 2018 •

edited

Loading