Use babel-polyfill and babel-preset-env #3790

voxsim · 2017-07-01T17:09:53Z

Summary
In this pull request:

I reviewed all the presets, plugin, etc.. related to babel
Upgraded babel-jest
Use babel-polyfill to fix Fix .includes issue once and for all #3693 once for all (I hope 😱 )
Use babel-preset-env instead of babel-preset-node5 and babel-preset-es2015-node4 to be futureproof
Add some plugins that babel-preset-env doesn't have by default
Rename babel env 'pre-node5' in 'node4'

Test plan
Tests in place

I am a babel newbie, if anyone found something erronous please let me know :D it took me many days to understand each part and how it works ;)

voxsim · 2017-07-02T09:39:12Z

For some reasons, in node4 with babel-polyfill the errors behave differently and put some additional slashes:

    Expected string:
      "Command failed: node \"/home/travis/build/yarnpkg/yarn/bin/yarn.js\" unknown 
    error An unexpected error occurred: \"Command \\\"unknown\\\" not found.\".
    "
    To contain value:
      "Command \"unknown\" not found"

Volune · 2017-07-02T11:06:50Z

That looks great. 👍

A few questions:

Can we move import 'babel-polyfill'; into a separate file, to ensure it's imported only once (as recommended in the doc)?
Can we put the node6 babel configuration as default configuration?
Can you also revert Remove .includes for node 4 compatibility #3592 and Fix: Fix Node 4 by not using .includes() #3654, to ensure .includes will work as expected?
I like to see you replacing stage-0 preset with specific plugins, but what was the politic about stage-0 from Yarn developers?

kaylie-alexa

Awesome, thanks for working on this! As we discussed, the one downside is that packages that depend on yarn may now end up including babel-polyfill twice. But yarn mainly being a cli tool, getting dev convenience of using the latest features, and mitigating all of it with useBuiltins, I think the change is worth it! 😄

kaylie-alexa · 2017-07-02T16:26:47Z

package.json

@@ -6,6 +6,7 @@
  "preferGlobal": true,
  "description": "📦🐈 Fast, reliable, and secure dependency management.",
  "dependencies": {
+    "babel-polyfill": "^6.23.0",
    "babel-runtime": "^6.0.0",


babel-runtime can probably be removed now.

kaylie-alexa · 2017-07-02T16:39:18Z

.babelrc

+        ["transform-object-rest-spread"],
+        ["transform-class-properties"],
+        ["transform-inline-imports-commonjs"]
+      ],


nit, they don't have to be nested arrays.

kaylie-alexa · 2017-07-02T17:36:10Z

scripts/build-webpack.js

@@ -58,7 +59,7 @@ const compilerLegacy = webpack({
        test: /\.js$/,
        exclude: /node_modules/,
        loader: 'babel-loader',
-        query: babelRc.env['pre-node5'],
+        query: babelRc.env.node4,


This can also be done by defining process.env.BABEL_ENV in https://webpack.js.org/plugins/environment-plugin/ so we don't have to parse the babelrc manually in the file, esp. since query field has been replaced with options w/ webpack2. Gulp probably has something similar as well.

Is it ok if we open another issue/pull request on that? This pr is becoming too big imho.

yup, thanks!

@kaylieEB can you open the issue? Because I don't fully understand your comment, I am afraid to write something wrong

np :) #3809

kaylie-alexa · 2017-07-02T19:39:04Z

.babelrc

+        "useBuiltIns": true
+      }]]
+    },
+    "node6": {


to what @Volune said, maybe this can be "development", which babel uses as default.

This reverts commit 645d0d8.

This reverts commit 2cb5e6c.

voxsim · 2017-07-03T11:34:58Z

@Volune some answers:

Can we move import 'babel-polyfill'; into a separate file, to ensure it's imported only once (as recommended in the doc)?

The file src/cli/index.js is always required one time, I included in that file because otherwise babel-preset-env does not do its magic (I mean replacing the require('babel-polyfill') with all require that really needs in base on the platform).

Can we put the node6 babel configuration as default configuration?

Yes, we can, I will do asap

Can you also revert #3592 and #3654, to ensure .includes will work as expected?

I did it

I like to see you replacing stage-0 preset with specific plugins, but what was the politic about stage-0 from Yarn developers?

I think we should ask this question to @bestander or maybe @kittens, I can always revert and use the stage-0 preset, It's not a problem at all :D

bestander · 2017-07-03T16:25:49Z

Looping in @BYK.

bestander · 2017-07-03T17:04:00Z

One thing to test - what is the impact on the bundle size?

voxsim · 2017-07-03T17:30:38Z

That's a good answer, I will check it O.o @bestander do you think we can integrate https://github.com/siddharthkp/bundlesize in yarn?

bestander · 2017-07-03T17:46:37Z

do you think we can integrate https://github.com/siddharthkp/bundlesize in yarn?

That looks cool and useful.
My first impression - "YES!!!" but then I start thinking about the support burden and this thread comes to mind #3696.
We can add so many things to yarn CLI and make it solve a wide range of issues but the wider it gets the harder it will be for people to discover the commands that solve really complex problems.

I would separate Yarn as a project into two areas

a package manager CLI and everything about it - keeping yarn.lock consistent, dealing with version conflicts and node_modules size and support for different runtime environments
a community project that makes package management easier - yarnpkg.com website, its blog, yarn.fyi website (yarn.fyi/react), yarn create command, github issues and RFCs

The latter is non homogenous and split across domains and tools.
Maybe we need to have a buddy tool that comes with Yarn and addresses the problem in a consistent way?

Daniel15 · 2017-07-03T18:38:16Z

@voxsim - The effect of this pull request on the bundle size looks way too big, unfortunately. Comparing the yarn-legacy JS file, it's 4.23 MB for this build (https://4316-49970642-gh.circle-artifacts.com/0/home/ubuntu/yarn/artifacts/yarn-legacy-0.27.0.js) vs 3.58 MB for the latest build of master (https://nightly.yarnpkg.com/yarn-legacy-0.27.0-20170703.1704.js).

Daniel15 · 2017-07-03T18:40:25Z

src/util/execute-lifecycle-script.js

@@ -145,7 +145,7 @@ export async function executeLifecycleScript(
  // Add global bin folder if it is not present already, as some packages depend
  // on a globally-installed version of node-gyp.
  const globalBin = getGlobalBinFolder(config, {});
-  if (pathParts.indexOf(globalBin) === -1) {
+  if (!pathParts.includes(globalBin)) {


What's the advantage of this change?

Daniel15 · 2017-07-03T18:43:05Z

I've tried using babel-preset-env in the past, and was too scared to commit it since the generated JavaScript differed. For example, it started converting arrow functions to regular functions, due to the fact that Node.js 4.x has some very minor bugs around precedence (http://node.green/#ES2015-functions-arrow-functions-correct-precedence) that don't really affect us.

Please perform a build with the old Babel config, and then another build with the new Babel config, and diff the files to see what the differences are. It's likely to be fine, but it's worth thoroughly inspecting the output.

BYK

Hey there, thanks for the PR and all the effort you've put into it. That said it looks like you've put a bit too much effort and the PR does many things at once.

I'd love to get more atomic changes so how about we split it into multiple pieces:

Upgrade babel-jest
babel-preset related changes
Rename babel env 'pre-node5' in 'node4'

And finally, not add babel-polyfill into the mix since it adds quite a bit of cruft for very small gains. We've now fixed our CI and upgraded Jest to not include polyfills in tests by default so we catch .includes() related failures on Node 4 builds.

Daniel15 · 2017-07-03T21:15:29Z

We can probably just polyfill includes with something like:

if (!String.prototype.includes) {
  String.prototype.includes = function(str, start) {
    return this.indexOf(str, start) > -1;
  }
}

voxsim · 2017-07-03T21:37:33Z

@bestander I totally agree with you, btw I was talking on integrate bundle size in our travis/circleci build to check on every pr the bundlesize of yarn node6/node4 build (i don't like call it normal or legacy because with node8 it does not make sense). I can open a pr but I don't have the permissions to do anything on Travis or CircleCI.

@Daniel15 thanks for your feedbacks and I agree with you about the polyfill, I think you should comment on #3693 and decide with us which are our next steps :)

@BYK thanks for your review, I agree with you that this PR is too big. Do you prefer one pr for each commit or can i open a pull request with:

remove babylon
-upgrade babel-core
remove babel-plugin-symbol
update babel-jest
After this new pr we will discuss our next steps ;)

bestander · 2017-07-03T21:53:47Z

I totally agree with you, btw I was talking on integrate bundle size in our travis/circleci build to check on every pr the bundlesize of yarn node6/node4 build (i don't like call it normal or legacy because with node8 it does not make sense). I can open a pr but I don't have the permissions to do anything on Travis or CircleCI.

We can sort out the permission issue.
Thanks for looking into this.

Daniel15 · 2017-07-03T22:20:36Z

Do you prefer one pr for each commit or can i open a pull request with:

One PR per commit please 😃

voxsim added 12 commits July 1, 2017 19:49

remove babylon

7aafa5c

update babel-core

b52cad0

remove babel-plugin-transform-es2015-typeof-symbol

e7026e5

use babel-polyfill instead of transform-runtime

59ebb3f

remove babel-plugin-transform-runtime

1636d98

rename babel env pre-node5 in node4

5bc0ca1

remove babel-plugin-array-includes

9ee6aca

use babel-preset-env instead of ES2015-node4

a37be39

remove package.json yarn.lock

b34531c

use babel-preset-env instead of babel-preset-node5

f176e2b

remove babel-preset-node5 and babel-preset-stage-0

abe749d

upgrade babel-jest

28ce70d

voxsim force-pushed the babel-tower branch from f773b11 to 28ce70d Compare July 1, 2017 17:59

kaylie-alexa reviewed Jul 2, 2017

View reviewed changes

voxsim added 3 commits July 3, 2017 13:22

Revert "Remove .includes for node 4 compatibility (yarnpkg#3592)"

d461882

This reverts commit 645d0d8.

Revert "Fix: Fix Node 4 by not using .includes() (yarnpkg#3654)"

9555e02

This reverts commit 2cb5e6c.

delete nested arrays in .babelrc

3ce946e

bestander requested a review from BYK July 3, 2017 16:25

Daniel15 reviewed Jul 3, 2017

View reviewed changes

BYK requested changes Jul 3, 2017

View reviewed changes

BYK self-assigned this Jul 5, 2017

voxsim mentioned this pull request Jul 10, 2017

Use bundlesize to check the size of the legacy artifact before merging each pr #3891

Open

voxsim closed this Jul 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use babel-polyfill and babel-preset-env #3790

Use babel-polyfill and babel-preset-env #3790

voxsim commented Jul 1, 2017 •

edited

Loading

voxsim commented Jul 2, 2017

Volune commented Jul 2, 2017

kaylie-alexa left a comment

kaylie-alexa Jul 2, 2017

kaylie-alexa Jul 2, 2017

kaylie-alexa Jul 2, 2017

voxsim Jul 3, 2017

kaylie-alexa Jul 3, 2017

voxsim Jul 3, 2017

kaylie-alexa Jul 3, 2017

voxsim Jul 3, 2017

kaylie-alexa Jul 2, 2017

voxsim commented Jul 3, 2017

bestander commented Jul 3, 2017

bestander commented Jul 3, 2017

voxsim commented Jul 3, 2017

bestander commented Jul 3, 2017 •

edited

Loading

Daniel15 commented Jul 3, 2017 •

edited

Loading

Daniel15 Jul 3, 2017

Daniel15 commented Jul 3, 2017 •

edited

Loading

BYK left a comment •

edited

Loading

Daniel15 commented Jul 3, 2017

voxsim commented Jul 3, 2017 •

edited

Loading

bestander commented Jul 3, 2017

Daniel15 commented Jul 3, 2017

Use babel-polyfill and babel-preset-env #3790

Use babel-polyfill and babel-preset-env #3790

Conversation

voxsim commented Jul 1, 2017 • edited Loading

voxsim commented Jul 2, 2017

Volune commented Jul 2, 2017

kaylie-alexa 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

voxsim commented Jul 3, 2017

bestander commented Jul 3, 2017

bestander commented Jul 3, 2017

voxsim commented Jul 3, 2017

bestander commented Jul 3, 2017 • edited Loading

Daniel15 commented Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Daniel15 commented Jul 3, 2017 • edited Loading

BYK left a comment • edited Loading

Choose a reason for hiding this comment

Daniel15 commented Jul 3, 2017

voxsim commented Jul 3, 2017 • edited Loading

bestander commented Jul 3, 2017

Daniel15 commented Jul 3, 2017

voxsim commented Jul 1, 2017 •

edited

Loading

bestander commented Jul 3, 2017 •

edited

Loading

Daniel15 commented Jul 3, 2017 •

edited

Loading

Daniel15 commented Jul 3, 2017 •

edited

Loading

BYK left a comment •

edited

Loading

voxsim commented Jul 3, 2017 •

edited

Loading