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

Framework: Update code to work with Babel 7 #7832

Merged
merged 9 commits into from Jul 11, 2018

Conversation

@gziolo
Member

gziolo commented Jul 9, 2018

Moved from WordPress/packages#136.

This PR follows the migration docs for Babel 7 migration: https://new.babeljs.io/docs/en/next/v7-migration.html.

It is highly influenced by Calypso migration done a few weeks back on a much larger codebase with the help from Babel maintainers: Automattic/wp-calypso#23424.

The most useful hint:

  • Updated babel-core to the bridge which is necessary for anything having babel as a peer dependency -- aka jest.

Changes

  1. jest and jest-enzyme were upgraded to the latest version to make them work with Babel 7.
  2. This PR fixes the following Babel 6 issue we discovered when publishing packages to npm:
export * from './components';
export * from './api';

transpiles to commonjs with the unexpected:

import _Object$defineProperty from 'babel-runtime/core-js/object/define-property';
import _Object$keys from 'babel-runtime/core-js/object/keys';

See related comment from @nerrad:
#3955 (comment).

Testing

  • npm run dev
  • npm run buil
  • npm test
  • npm run test-e2e

All those commends should work, there should be no regressions when interacting with Gutenberg.

@gziolo gziolo added this to the 3.3 milestone Jul 9, 2018

@gziolo gziolo self-assigned this Jul 9, 2018

@gziolo gziolo requested review from aduth, ntwb, nerrad and WordPress/gutenberg-core Jul 9, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 9, 2018

Locally, one unit tests is failing:

packages/core-data/src/test/selectors.js
● Test suite failed to run

TypeError: Cannot read property 'loadAndPersist' of undefined

  29 |     return _persist.loadAndPersist;
  30 |   }
> 31 | });
     |              ^
  32 | Object.defineProperty(exports, "withRehydration", {
  33 |   enumerable: true,
  34 |   get: function get() {

  at Object.get [as loadAndPersist] (packages/data/build/index.js:31:21)
  at packages/core-data/src/test/selectors.js:23:452
      at Array.forEach (<anonymous>)
  at _objectSpread (packages/core-data/src/test/selectors.js:23:392)
  at Object.user:/Users/gziolo/PhpstormProjects/gutenberg/packages/data/build/index.js: (packages/core-data/src/test/selectors.js:4:10)
  at Object.<anonymous> (packages/data/build/store/index.js:12:9)
  at Object.<anonymous> (packages/data/build/index.js:84:37)
  at Object.user:/Users/gziolo/PhpstormProjects/gutenberg/packages/data/build/index.js: (packages/core-data/src/test/selectors.js:4:36)
  at Object.<anonymous> (packages/core-data/src/selectors.js:29:13)
  at Object.<anonymous> (packages/core-data/src/test/selectors.js:17:18)

I will further investigate tomorrow.

@@ -178,7 +150,7 @@
"build:packages": "cross-env EXCLUDE_PACKAGES=babel-plugin-import-jsx-pragma node ./bin/packages/build.js",
"build": "npm run build:packages && cross-env NODE_ENV=production webpack",
"check-engines": "check-node-version --package",
"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
"ci": "concurrently \"npm run lint\" \"npm run test-unit:coverage-ci\"",

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

npm run build may accidentally regenerate jest-console build files which are used in framework setup script for each test suite. We run it on the node which runs e2e tests anyways.

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

I'm having a hard time tracking down why we had npm run build here in the first place. If I were to guess, seems as though it were to test to ensure that we'd have no errors during a build. Could still be useful, though I'm not entirely sure it's critical (at least weighted against the time cost / complications).

export { loadAndPersist, withRehydration } from './persist';
export {
loadAndPersist,
setPersistenceStorage,

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

setPersistenceStorage - it probably should be another PR. I added it because it wasn't exposed.

@@ -22,8 +22,7 @@
],
"timers": "fake",
"transform": {
"^.+\\.jsx?$": "babel-jest",
"\\.pegjs$": "<rootDir>/node_modules/@wordpress/jest-preset-default/scripts/pegjs-transform.js"

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

pegjs is not needed outside of Gutenberg.

@@ -1,16 +0,0 @@
const pegjs = require( 'pegjs' );

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

The same applies - pegjs transform is duplicated here and we need it only for Gutenberg.

@@ -13,35 +13,6 @@ global.window.matchMedia = () => ( {
removeListener: () => {},
} );
global.window._wpDateSettings = {

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

We are now providing those defaults inside @wordpress/date package.

@@ -51,6 +22,3 @@ global.window.localStorage = {
// UserSettings global
global.window.userSettings = { uid: 1 };
// Mock jQuery
global.window.jQuery = { holdReady() {} };

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

We no longer use holdReady in the codebase, besides it isn't general enough to be here.

},
"preset": "@wordpress/jest-preset-default",
"setupFiles": [
"core-js/fn/symbol/async-iterator",

This comment has been minimized.

@gziolo

gziolo Jul 10, 2018

Member

It's included in Babel's preset.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 10, 2018

Travis is happy about the current shape of this PR, I am as well. It should be ready to be landed once it is tested and all review feedback gets addressed.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 10, 2018

Chunks stats for npm run build:

Before

Webpack stats:

          ./build/viewport/index.js   12.1 KiB      19  [emitted]         viewport
         ./build/shortcode/index.js   8.77 KiB       0  [emitted]         shortcode
          ./build/keycodes/index.js   12.7 KiB       2  [emitted]         keycodes
  ./build/is-shallow-equal/index.js   6.24 KiB       3  [emitted]         isShallowEqual
              ./build/i18n/index.js     24 KiB       4  [emitted]         i18n
             ./build/hooks/index.js   10.1 KiB       5  [emitted]         hooks
           ./build/element/index.js   31.4 KiB       6  [emitted]         element
         ./build/dom-ready/index.js  741 bytes       7  [emitted]         domReady
               ./build/dom/index.js     17 KiB       8  [emitted]         dom
        ./build/deprecated/index.js   6.66 KiB       9  [emitted]         deprecated
              ./build/date/index.js    187 KiB      10  [emitted]         date
              ./build/data/index.js   52.3 KiB      11  [emitted]         data
         ./build/core-data/index.js   45.1 KiB      12  [emitted]         coreData
              ./build/blob/index.js  909 bytes      13  [emitted]         blob
       ./build/api-request/index.js   8.45 KiB      14  [emitted]         apiRequest
              ./build/a11y/index.js    1.8 KiB      15  [emitted]         a11y
               ./build/nux/index.js   18.5 KiB      16  [emitted]         nux
       ./build/core-blocks/index.js    168 KiB      17  [emitted]         coreBlocks
         ./build/edit-post/index.js   80.6 KiB      18  [emitted]         editPost
           ./build/plugins/index.js     22 KiB       1  [emitted]         plugins
             ./build/utils/index.js   24.1 KiB      20  [emitted]         utils
            ./build/editor/index.js    322 KiB      21  [emitted]  [big]  editor
        ./build/components/index.js    484 KiB      22  [emitted]  [big]  components
            ./build/blocks/index.js    264 KiB      23  [emitted]  [big]  blocks

In the browser (JavaScript only):
screen shot 2018-07-10 at 11 38 16

After

Webpack stats:

                              Asset       Size  Chunks                    Chunk Names
          ./build/viewport/index.js   24.2 KiB      19  [emitted]         viewport
         ./build/shortcode/index.js   16.1 KiB       0  [emitted]         shortcode
          ./build/keycodes/index.js   17.7 KiB       2  [emitted]         keycodes
  ./build/is-shallow-equal/index.js   6.39 KiB       3  [emitted]         isShallowEqual
              ./build/i18n/index.js   27.4 KiB       4  [emitted]         i18n
             ./build/hooks/index.js   19.2 KiB       5  [emitted]         hooks
           ./build/element/index.js     42 KiB       6  [emitted]         element
         ./build/dom-ready/index.js  741 bytes       7  [emitted]         domReady
               ./build/dom/index.js   22.8 KiB       8  [emitted]         dom
        ./build/deprecated/index.js   4.89 KiB       9  [emitted]         deprecated
              ./build/date/index.js    192 KiB      10  [emitted]         date
              ./build/data/index.js   60.6 KiB      11  [emitted]         data
         ./build/core-data/index.js   54.5 KiB      12  [emitted]         coreData
              ./build/blob/index.js  909 bytes      13  [emitted]         blob
       ./build/api-request/index.js     21 KiB      14  [emitted]         apiRequest
              ./build/a11y/index.js   5.15 KiB      15  [emitted]         a11y
               ./build/nux/index.js   23.8 KiB      16  [emitted]         nux
       ./build/core-blocks/index.js    185 KiB      17  [emitted]         coreBlocks
         ./build/edit-post/index.js   91.7 KiB      18  [emitted]         editPost
           ./build/plugins/index.js   24.5 KiB       1  [emitted]         plugins
             ./build/utils/index.js   40.9 KiB      20  [emitted]         utils
            ./build/editor/index.js    352 KiB      21  [emitted]  [big]  editor
        ./build/components/index.js    502 KiB      22  [emitted]  [big]  components
            ./build/blocks/index.js    302 KiB      23  [emitted]  [big]  blocks

In the browser (JavaScript only):
screen shot 2018-07-10 at 11 18 45

Thoughts

There is a slight increase in the size of all bundles that use polyfills or Babel helpers. It's sort of expected given that we enable new useBuiltIns: 'usage' option. This should allow us to iterate later and move all those require statements to its own bundle and hopefully reduced the total size of JavaScript code sent to the browser.

@gziolo gziolo added this to In Progress in API freeze via automation Jul 10, 2018

@hypest

hypest approved these changes Jul 10, 2018

👋 @gziolo , gave the PR (commit 1de19d6, and after merging from GB master) a try against this RN app branch ( and it seems that the RN app builds and runs just fine. In other words, upgrading GB to Babel 7 doesn't seem to affect the current state of the RN app in a negative way. So, fwiw, 👍 from me!

@Shelob9

This comment has been minimized.

Contributor

Shelob9 commented Jul 11, 2018

@gziolo This is : 🔥

Is there any way to test the packages before the PR is merged? I was thinking build Gutenberg locally and then use npm link, but not sure if that works with this setup.

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 11, 2018

Is there any way to test the packages before the PR is merged? I was thinking build Gutenberg locally and then use npm link, but not sure if that works with this setup.

It might be tricky because we started using local packages file:. We can always fix everything after next alpha is published to npm. I still don't know what it the proper way to inform consumers of packages that they should have @babel/runtime installed locally. Should we put this in the readme file or set as an explicit dependency in each package?

@nerrad

This comment has been minimized.

Contributor

nerrad commented Jul 11, 2018

I still don't know what it the proper way to inform consumers of packages that they should have @babel/runtime installed locally. Should we put this in the readme file or set as an explicit dependency in each package?

What about making @babel/runtime a peer dependency in each package?

@@ -7,7 +7,3 @@ coverage:
patch: off
comment: false
ignore:

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

Why is this being removed here?

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

Sorry, realized later it's now reflected in jest.config.json

@@ -178,7 +150,7 @@
"build:packages": "cross-env EXCLUDE_PACKAGES=babel-plugin-import-jsx-pragma node ./bin/packages/build.js",
"build": "npm run build:packages && cross-env NODE_ENV=production webpack",
"check-engines": "check-node-version --package",
"ci": "concurrently \"npm run lint && npm run build\" \"npm run test-unit:coverage-ci\"",
"ci": "concurrently \"npm run lint\" \"npm run test-unit:coverage-ci\"",

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

I'm having a hard time tracking down why we had npm run build here in the first place. If I were to guess, seems as though it were to test to ensure that we'd have no errors during a build. Could still be useful, though I'm not entirely sure it's critical (at least weighted against the time cost / complications).

@@ -1,3 +1,8 @@
## Unreleased (1.1.0)

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

I think formatting would be ## 1.1.0 (Unreleased) for consistency with other releases?

This comment has been minimized.

@gziolo

gziolo Jul 11, 2018

Member

I can update all of them 👍

@@ -10,7 +10,7 @@ import plugin from '../src';
describe( 'babel-plugin-import-jsx-pragma', () => {
function getTransformedCode( source, options = {} ) {
const { code } = transform( source, {
const { code } = transformSync( source, {
plugins: [
[ plugin, options ],
'syntax-jsx',

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

I think this needs to be updated to @babel/syntax-jsx ? I'm seeing some related failing tests.

This comment has been minimized.

@gziolo
} ) );
jest.mock( '@wordpress/data', () => {
return {
select: jest.fn().mockReturnValue( {

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

Did we mean to drop the ...require.requireActual ?

This comment has been minimized.

@gziolo

gziolo Jul 11, 2018

Member

I think I commented elsewhere, yes, it was failing when using requireActual because of some Babel + Enzyme internal things. requireActual isn't going through moduleNameMapper flow so it tried to load Babel build, but couldn't import it properly 🤷‍♂️

@aduth

This comment has been minimized.

Member

aduth commented Jul 11, 2018

Observing these errors trying to run tests:

Configuration error:
    
    Could not locate module /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/node_modules/@wordpress/babel-plugin-import-jsx-pragma/build/index.js mapped as:
    packages/babel-plugin-import-jsx-pragma/build/index.js/src.
    
    Please check your configuration for these entries:
    {
      "moduleNameMapper": {
        "/@wordpress\/(.*)$/": "packages/$1/src"
      },
      "resolver": null
    }
} ],
].filter( Boolean ),
plugins: [
'@babel/plugin-proposal-object-rest-spread',

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

I think these can be shortened to e.g. @babel/proposal-object-rest-spread. Maybe less explicit. Haven't confirmed.

This comment has been minimized.

@gziolo

gziolo Jul 11, 2018

Member

Yes, I think they now discourage it though 🤷‍♂️

@aduth

This comment has been minimized.

Member

aduth commented Jul 11, 2018

Got closer to passing tests with this diff:

diff --git a/packages/babel-plugin-import-jsx-pragma/test/index.js b/packages/babel-plugin-import-jsx-pragma/test/index.js
index ede36f878..355a698f7 100644
--- a/packages/babel-plugin-import-jsx-pragma/test/index.js
+++ b/packages/babel-plugin-import-jsx-pragma/test/index.js
@@ -13,7 +13,7 @@ describe( 'babel-plugin-import-jsx-pragma', () => {
 		const { code } = transformSync( source, {
 			plugins: [
 				[ plugin, options ],
-				'syntax-jsx',
+				'@babel/syntax-jsx',
 			],
 		} );
 
diff --git a/test/unit/jest.config.json b/test/unit/jest.config.json
index 71c618443..5a4daa0fc 100644
--- a/test/unit/jest.config.json
+++ b/test/unit/jest.config.json
@@ -12,7 +12,7 @@
 	],
 	"moduleNameMapper": {
 		"@wordpress\\/(blocks|components|editor|utils|edit-post|viewport|core-blocks|nux)$": "$1",
-		"@wordpress\\/(.*)$": "packages/$1/src"
+		"@wordpress\\/(.*)$": "packages/$1"
 	},
 	"preset": "@wordpress/jest-preset-default",
 	"setupFiles": [

But still seeing failures as though it's doing much more transforms than expected in import-jsx-pragma tests:

    Expected: "import { createElement } from \"@wordpress/element\";
    let foo = <bar />;"
    Received: "\"use strict\";
    
    var _element = require(\"@wordpress/element\");
    
    var foo = (0, _element.createElement)(\"bar\", null);"

Also:

  • Guessing we don't want to target the built files in the test mapping (for debugging?)
  • Are we inheriting Babel configuration now in these tests? This was meant to be isolated . Tried some options to disable .babelrc from the transform with no luck.
@gziolo

This comment has been minimized.

Member

gziolo commented Jul 11, 2018

@aduth - I had all those bugs fixed, but overrode by accident commit when trying to merge package-lock.json with node-sass upgrade ... 😱

I recreated in 20c8db6

Guessing we don't want to target the built files in the test mapping (for debugging?)

Yes, we agreed about that in the past, that we want to test using source code whenever possible. It also helps us to avoid running npm run build:packages before each npm test run. Not mentioning watch mode where it gets even more tricky.

Are we inheriting Babel configuration now in these tests? This was meant to be isolated . Tried some options to disable .babelrc from the transform with no luck.

Now you need to explicitly disable searching for a config file:
https://github.com/WordPress/gutenberg/pull/7832/files#diff-356b7086329f67b5a3335e713d48ad9bR14

@aduth

aduth approved these changes Jul 11, 2018

Looks great 👍

"main": "build/index.js",
"module": "build-module/index.js",
"dependencies": {
"gettext-parser": "^1.3.1",
"lodash": "4.17.10"
"lodash": "^4.17.10"

This comment has been minimized.

@aduth

aduth Jul 11, 2018

Member

May have been mentioned already, but noting we're adding loose ranges for dependencies, which while not strictly within the scope of the effort is sensible given that these are libraries.

This comment has been minimized.

@gziolo

gziolo Jul 11, 2018

Member

Yes, I had to regenerate lock file because of the amount of changes Babel introduced after they started using namespaced packages so I thought we can also include this change for everything that we consider libraries for consistency. We were mixing ranges and exact versions before.

@aduth aduth referenced this pull request Jul 11, 2018

Closed

Use a Babel polyfill script #746

@aduth aduth merged commit f3b6379 into master Jul 11, 2018

2 checks passed

codecov/project Absolute coverage decreased by -0.5% but relative coverage increased by +50.06% compared to 804db6d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

API freeze automation moved this from In Progress to Done Jul 11, 2018

@aduth aduth deleted the update/babel7-upgrade branch Jul 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment