feat(babel): migrate to babel@7 #790

daern91 · 2018-09-10T18:43:24Z

Summary

PR migrates from babel@6 to @7.
Big thanks for the pointers and inspiration from @tdeekens's PR that did the same here: tdeekens/flopflip#465

Description

One thing to notice here is the solution on how to use the config file. Babel 7 removes support for .babelrc inside of monorepos (ref).

We will now use babel.config.js instead (ref). However, please see the following quote:

The primary downside of this project-wide config is that, because it relies on the working directory, it can be more painful to use in monorepos if the working directory is not the monorepo root. For example, if you have
babel.config.js
packages/
 mod1/
   package.json
   src/index.js
 mod2/
   package.json
   src/index.js
and the individual packages are responsible for running their builds (and their working directory for Babel is the individual packages), the babel.config.js file will not be automatically loaded, and users will be required to set the path to it manually.

So, PR also changes the build script across all packages. This might not be the ideal solution so I'd love to get any thoughts and input on it. Would be great if we could find another way.

resolves #768
resolves #773
resolves #763
resolves #766

affects: @commercetools/csv-parser-discount-code, @commercetools/csv-parser-orders, @commercetools/csv-parser-price, @commercetools/csv-parser-state, @commercetools/custom-objects-exporter, @commercetools/custom-objects-importer, @commercetools/customer-groups-exporter, @commercetools/discount-code-exporter, @commercetools/discount-code-generator, @commercetools/discount-code-importer, @commercetools/get-credentials, @commercetools/inventories-exporter, @commercetools/personal-data-erasure, @commercetools/price-exporter, @commercetools/product-exporter, @commercetools/product-json-to-csv, @commercetools/state-importer

codecov · 2018-09-10T18:43:30Z

Codecov Report

Merging #790 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #790   +/-   ##
=======================================
  Coverage   98.75%   98.75%           
=======================================
  Files         107      107           
  Lines        2737     2737           
  Branches      624      624           
=======================================
  Hits         2703     2703           
  Misses         30       30           
  Partials        4        4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26f47d1...b1cdfa5. Read the comment docs.

emmenko

Looks legit on a first glance! 👌

daern91 · 2018-09-10T18:54:30Z

package.json

    "@commitlint/cli": "7.1.1",
    "@commitlint/config-conventional": "7.1.1",
-    "babel-cli": "6.26.0",
-    "babel-core": "6.26.3",
+    "babel-core": "7.0.0-bridge.0",


This one is still needed for babel-jest below (ref).

daern91 · 2018-09-10T18:57:56Z

lerna.json

@@ -1,7 +1,8 @@
 {
-  "lerna": "2.7.1",
+  "lerna": "2.11.0",


We can not yet update to lerna 3.x because of cz-lerna-changelog not being updated.

Hmm, why are we still using that?

It's used by commitizen when running the commit script.

nodejs/package.json

Line 15 in 083e559

"commit": "git-cz",

I'm not sure its very effective though, looking at the changelog it doesnt seem to be updated...
https://github.com/commercetools/nodejs/blob/master/CHANGELOG.md

Maybe we could get rid of it instead?

I would get rid of it. I think no one is using that command...?

Sounds good, will do. I'll keep commitizen itself tho as it's amazing for on-boarding students and introducing them to angular commit convention.

Ask around in the team. I am not aware of anyone using it anymore. It was good introducing the style but should not be needed anymore. I would recommend removing it in amothrr PR.

commitizen itself is so drilled into my spine that it is part of my muscle memory now.
However, if you feel that you want to keep this to ease "onboarding", or in some extent ensuring consistency in commit messages, then don't listen to us 👍

daern91 · 2018-09-10T18:59:29Z

packages/csv-parser-discount-code/package.json

@@ -28,7 +28,7 @@
    "lib"
  ],
  "scripts": {
-    "build": "babel src --out-dir lib"
+    "build": "babel src --out-dir lib --config-file '../../babel.config.js'"


Other ideas would be appreciated here. I do prefer this over having a config file inside of every package but it's definitely not ideal.

I think it's fine like this

Yes this is fine. To my knowledge the idea of a babel.config over an rc without being breaking is to be more explicit. Meaning you pass it, you import it etc.

tdeekens

💯👍🏼

tdeekens · 2018-09-10T19:51:04Z

babel.config.js

+      break
+  }
+  return {}
+}


Nicely done.

tdeekens · 2018-09-10T19:51:42Z

jest.transform.js

@@ -0,0 +1,3 @@
+const babelConfig = require('./babel.config')
+
+module.exports = require('babel-jest').createTransformer(babelConfig)


Yep. Have the same on mc-fe and flopflip. This js good and does he job.

tdeekens · 2018-09-10T19:53:01Z

lerna.json

@@ -1,7 +1,8 @@
 {
-  "lerna": "2.7.1",
+  "lerna": "2.11.0",


Ask around in the team. I am not aware of anyone using it anymore. It was good introducing the style but should not be needed anymore. I would recommend removing it in amothrr PR.

tdeekens · 2018-09-10T19:53:12Z

package.json

    "@commitlint/cli": "7.1.1",
    "@commitlint/config-conventional": "7.1.1",
-    "babel-cli": "6.26.0",
-    "babel-core": "6.26.3",
+    "babel-core": "7.0.0-bridge.0",


tdeekens · 2018-09-10T19:54:08Z

packages/csv-parser-discount-code/package.json

@@ -28,7 +28,7 @@
    "lib"
  ],
  "scripts": {
-    "build": "babel src --out-dir lib"
+    "build": "babel src --out-dir lib --config-file '../../babel.config.js'"


Yes this is fine. To my knowledge the idea of a babel.config over an rc without being breaking is to be more explicit. Meaning you pass it, you import it etc.

tdeekens · 2018-09-10T19:56:13Z

packages/state-importer/package.json

@@ -4,7 +4,7 @@
  "description": "Import states to the commercetools platform",
  "main": "lib/main.js",
  "scripts": {
-    "build": "babel src --out-dir lib"
+    "build": "babel src --out-dir lib --config-file '../../babel.config.js'"


You could maybe also pass it as an env var fro, the host project. BABEL_CONFIG=„blub“ and use it here via interpolation. I may have seen that somewhere. Not sure it serves a bug benefit however as it never moves etc.

tdeekens · 2018-09-10T19:57:33Z

rollup.config.js

-    }),
+    babel(
+      Object.assign(
+        {


I thought you could spread here. Not sure what the nvmrc says in the repo these days 🎃

I believe that this file is not getting transpiled by babel and as we support node from version v6 we should not use any of new syntax here. Also, the spread operator based on ES6 works only with arrays.

Yup, it doesn't.

I tried ES7 Object spread first but it failed on Node6 :(

junajan

Hi @daern91 .. you did really a good job here in this PR.. I added just two nitpicks

junajan · 2018-09-11T08:06:30Z

babel.config.js

+    default:
+      break
+  }
+  return {}


Just a nitpick: this return {} can be in default branch instead of break.

Ahh, good find, fixed here 4fbe7d3

junajan · 2018-09-11T08:13:39Z

rollup.config.js

-    }),
+    babel(
+      Object.assign(
+        {


I believe that this file is not getting transpiled by babel and as we support node from version v6 we should not use any of new syntax here. Also, the spread operator based on ES6 works only with arrays.

daern91 added 4 commits September 10, 2018 16:15

fix(tests): transform unit tests

6a2ad93

fix(tests): use built-in object.entries

efc84bc

refactor(babel): remove unnecessary polyfill

e9ba36e

daern91 added Type: Maintenance Type: Question labels Sep 10, 2018

daern91 self-assigned this Sep 10, 2018

daern91 requested review from adnasa, junajan, emmenko, tdeekens and Babanila September 10, 2018 18:43

emmenko reviewed Sep 10, 2018

View reviewed changes

daern91 commented Sep 10, 2018

View reviewed changes

tdeekens approved these changes Sep 10, 2018

View reviewed changes

junajan reviewed Sep 11, 2018

View reviewed changes

daern91 added 2 commits September 11, 2018 10:38

refactor: move return into default switch

4fbe7d3

chore: update deps

8bdbca7

daern91 force-pushed the migrate-to-babel-7 branch from 602231c to 8bdbca7 Compare September 11, 2018 09:08

Merge branch 'master' into migrate-to-babel-7

b1cdfa5

daern91 merged commit bc6a3ee into master Sep 11, 2018

daern91 deleted the migrate-to-babel-7 branch September 11, 2018 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(babel): migrate to babel@7 #790

feat(babel): migrate to babel@7 #790

daern91 commented Sep 10, 2018 •

edited

codecov bot commented Sep 10, 2018 •

edited

emmenko left a comment

daern91 Sep 10, 2018

tdeekens Sep 10, 2018

daern91 Sep 10, 2018 •

edited

emmenko Sep 10, 2018

daern91 Sep 10, 2018 •

edited

emmenko Sep 10, 2018

daern91 Sep 10, 2018

tdeekens Sep 10, 2018

adnasa Sep 11, 2018

daern91 Sep 10, 2018

emmenko Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens left a comment

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

tdeekens Sep 10, 2018

junajan Sep 11, 2018

daern91 Sep 11, 2018

junajan left a comment

junajan Sep 11, 2018

daern91 Sep 11, 2018 •

edited

junajan Sep 11, 2018

		@@ -0,0 +1,3 @@
		const babelConfig = require('./babel.config')

		module.exports = require('babel-jest').createTransformer(babelConfig)

feat(babel): migrate to babel@7 #790

feat(babel): migrate to babel@7 #790

Conversation

daern91 commented Sep 10, 2018 • edited

Summary

Description

codecov bot commented Sep 10, 2018 • edited

Codecov Report

emmenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daern91 Sep 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daern91 Sep 10, 2018 • edited

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

tdeekens 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

junajan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daern91 Sep 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daern91 commented Sep 10, 2018 •

edited

codecov bot commented Sep 10, 2018 •

edited

daern91 Sep 10, 2018 •

edited

daern91 Sep 10, 2018 •

edited

daern91 Sep 11, 2018 •

edited