Framework: Update code to work with Babel 7#7832
Conversation
|
Locally, one unit tests is failing:
I will further investigate tomorrow. |
91dcc4f to
84f5ce2
Compare
| "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\"", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
packages/data/src/index.js
Outdated
| export { loadAndPersist, withRehydration } from './persist'; | ||
| export { | ||
| loadAndPersist, | ||
| setPersistenceStorage, |
There was a problem hiding this comment.
setPersistenceStorage - it probably should be another PR. I added it because it wasn't exposed.
| "timers": "fake", | ||
| "transform": { | ||
| "^.+\\.jsx?$": "babel-jest", | ||
| "\\.pegjs$": "<rootDir>/node_modules/@wordpress/jest-preset-default/scripts/pegjs-transform.js" |
There was a problem hiding this comment.
pegjs is not needed outside of Gutenberg.
| @@ -1,16 +0,0 @@ | |||
| const pegjs = require( 'pegjs' ); | |||
There was a problem hiding this comment.
The same applies - pegjs transform is duplicated here and we need it only for Gutenberg.
| removeListener: () => {}, | ||
| } ); | ||
|
|
||
| global.window._wpDateSettings = { |
There was a problem hiding this comment.
We are now providing those defaults inside @wordpress/date package.
| global.window.userSettings = { uid: 1 }; | ||
|
|
||
| // Mock jQuery | ||
| global.window.jQuery = { holdReady() {} }; |
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
It's included in Babel's preset.
|
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. |
|
Chunks stats for BeforeWebpack stats: In the browser (JavaScript only): AfterWebpack stats: In the browser (JavaScript only): ThoughtsThere 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 |
hypest
left a comment
There was a problem hiding this comment.
👋 @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!
|
@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. |
It might be tricky because we started using local packages |
1de19d6 to
76c16e9
Compare
What about making @babel/runtime a peer dependency in each package? |
|
|
||
| comment: false | ||
|
|
||
| ignore: |
There was a problem hiding this comment.
Sorry, realized later it's now reflected in jest.config.json
| "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\"", |
There was a problem hiding this comment.
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).
packages/autop/CHANGELOG.md
Outdated
| @@ -1,3 +1,8 @@ | |||
| ## Unreleased (1.1.0) | |||
There was a problem hiding this comment.
I think formatting would be ## 1.1.0 (Unreleased) for consistency with other releases?
There was a problem hiding this comment.
I can update all of them 👍
| const { code } = transformSync( source, { | ||
| plugins: [ | ||
| [ plugin, options ], | ||
| 'syntax-jsx', |
There was a problem hiding this comment.
I think this needs to be updated to @babel/syntax-jsx ? I'm seeing some related failing tests.
| } ) ); | ||
| jest.mock( '@wordpress/data', () => { | ||
| return { | ||
| select: jest.fn().mockReturnValue( { |
There was a problem hiding this comment.
Did we mean to drop the ...require.requireActual ?
There was a problem hiding this comment.
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 🤷♂️
|
Observing these errors trying to run tests: |
| } ], | ||
| ].filter( Boolean ), | ||
| plugins: [ | ||
| '@babel/plugin-proposal-object-rest-spread', |
There was a problem hiding this comment.
I think these can be shortened to e.g. @babel/proposal-object-rest-spread. Maybe less explicit. Haven't confirmed.
There was a problem hiding this comment.
Yes, I think they now discourage it though 🤷♂️
|
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 Also:
|
|
@aduth - I had all those bugs fixed, but overrode by accident commit when trying to merge I recreated in 20c8db6
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
Now you need to explicitly disable searching for a config file: |
| "dependencies": { | ||
| "gettext-parser": "^1.3.1", | ||
| "lodash": "4.17.10" | ||
| "lodash": "^4.17.10" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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:
Changes
jestandjest-enzymewere upgraded to the latest version to make them work with Babel 7.See related comment from @nerrad:
#3955 (comment).
Testing
npm run devnpm run builnpm testnpm run test-e2eAll those commends should work, there should be no regressions when interacting with Gutenberg.