-
Notifications
You must be signed in to change notification settings - Fork 28
Conversation
package.json
Outdated
"jest": { | ||
"collectCoverageFrom": [ | ||
"packages/*/**/*.js" | ||
], | ||
"preset": "@wordpress/jest-preset-default" | ||
"preset": "@wordpress/jest-preset-default", | ||
"transform": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to find a better way to apply this transform behind the scenes because we will have to apply a similar fix for Gutenberg otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this transform about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a comment in the description:
I have some issues with
babel-jest
which is no longer working as I would expect. I had to apply"^.+\\.jsx?$": "<rootDir>/packages/scripts/config/babel-transform.js"
in the mainpackage.json
to make it work. Ideally, it shouldn't be possible at all when overriding Jest config.
The thing is Jest is still on Babel 6 and we try to make it work with Babel 7, but something is broken and I don't see Babel transforms applied properly.
"@babel/plugin-transform-react-jsx": "^7.0.0-beta.49", | ||
"@babel/plugin-transform-runtime": "^7.0.0-beta.49", | ||
"@babel/preset-env": "^7.0.0-beta.49", | ||
"@babel/runtime": "^7.0.0-beta.49", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we shouldn't add @babel/runtime
to all package.json
file that belong to packages which have it referenced in transpiled code after Babel does its magic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can't just remove the runtime since we're using the polyfills of babel-env
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, @babel/runtime
and core-js
are used for requiring code in the transpiled code when using env
preset. The latter is the dependency of the previous one.
@@ -1,16 +0,0 @@ | |||
const pegjs = require( 'pegjs' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it outside of Gutenberg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this have a breaking effect that needs to be documented if someone is using @wordpress/jest-preset-default ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include it in the changelog. We can also bump version to 2.0.0 to follow the semantic versioning. I don't think that other code that blocks
module in Gutenberg would ever need to use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nerrad, can you confirm it can be removed or should we keep it as long as tests using blocks
depend on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this babel7 change needs a bump to 2.0.0
anyway. I'm wondering if we should update the pragma at the same time to avoid the need for a 3.0.0
later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, we can flip the pragma handling in Gutenberg, good idea 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't totally grok what pegjs is used for (Does it provide the grammar rules for the custom html comments for blocks?). Assuming that's what its for, and it only is required by GB blocks
- plugins writing tests can probably just mock any GB blocks in their tests.
@@ -13,35 +13,6 @@ global.window.matchMedia = () => ( { | |||
removeListener: () => {}, | |||
} ); | |||
|
|||
global.window._wpDateSettings = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wordpress/date
provides its own defaults after @youknowriad did refactoring.
packages/scripts/package.json
Outdated
@@ -34,8 +34,9 @@ | |||
"@wordpress/babel-preset-default": "^1.3.0", | |||
"@wordpress/jest-preset-default": "^1.0.6", | |||
"@wordpress/npm-package-json-lint-config": "^1.0.0", | |||
"babel-jest": "^23.0.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed, it's already handled in the preset.
scripts/build.js
Outdated
@@ -30,15 +30,15 @@ const ERROR = chalk.reset.inverse.bold.red( ' ERROR ' ); | |||
/** | |||
* Babel Configuration | |||
*/ | |||
const babelDefaultConfig = require( '../packages/babel-preset-default' ); | |||
const babelDefaultConfig = require( '../packages/babel-preset-default' )(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Babel expects functions in presets in 7.x line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see:
const isTestEnv = api.env() === 'test';
TypeError: Cannot read property 'env' of undefined
because we need to pass api
object to this preset ...
@@ -19,14 +19,14 @@ | |||
"rimraf": "^2.6.1", | |||
"symlink-or-copy": "^1.2.0" | |||
}, | |||
"babel": { | |||
"presets": "@wordpress/default" | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we removing this, is it useless?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jest can read Babel from its config, and the script which generates the distribution folders for packages uses the same preset internally. So it isn't necessary anymore - it uses defaults :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to add babel.config.js
instead to make everything work 😄
package.json
Outdated
"jest": { | ||
"collectCoverageFrom": [ | ||
"packages/*/**/*.js" | ||
], | ||
"preset": "@wordpress/jest-preset-default" | ||
"preset": "@wordpress/jest-preset-default", | ||
"transform": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this transform about?
targets: { | ||
browsers: [ 'extends @wordpress/browserslist-config' ], | ||
}, | ||
useBuiltIns: 'usage', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we can make this work, this would be a big improvement ❤️
"@babel/plugin-transform-react-jsx": "^7.0.0-beta.49", | ||
"@babel/plugin-transform-runtime": "^7.0.0-beta.49", | ||
"@babel/preset-env": "^7.0.0-beta.49", | ||
"@babel/runtime": "^7.0.0-beta.49", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we can't just remove the runtime since we're using the polyfills of babel-env
@@ -1,16 +0,0 @@ | |||
const pegjs = require( 'pegjs' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this babel7 change needs a bump to 2.0.0
anyway. I'm wondering if we should update the pragma at the same time to avoid the need for a 3.0.0
later
} ], | ||
].filter( Boolean ), | ||
plugins: [ | ||
'@babel/plugin-proposal-object-rest-spread', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the Babel core has been updated for it yet, but Rest/Spread was ratified as part of ES2018, so it might be possible to remove this.
https://github.com/tc39/proposals/blob/master/finished-proposals.md
scripts/build.js
Outdated
chalk.green( ' \u21D2 ' ) + | ||
path.relative( PACKAGES_DIR, destPath ) + | ||
'\n' | ||
path.relative( PACKAGES_DIR, file ) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops :)
Codecov Report
@@ Coverage Diff @@
## master #136 +/- ##
==========================================
- Coverage 67.04% 66.16% -0.89%
==========================================
Files 58 58
Lines 698 727 +29
Branches 145 151 +6
==========================================
+ Hits 468 481 +13
- Misses 187 202 +15
- Partials 43 44 +1
Continue to review full report at Codecov.
|
Closing this per @gziolo recommendation (work to begin shortly on this directly in the Gutenberg repo) |
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:
It is still not working. Some tests are failing. @aduth or @youknowriad , do you have any quick guess what is causing those failures when running
npm run test packages/babel-plugin-makepot/
I have some issues with
babel-jest
which is no longer working as I would expect. I had to apply"^.+\\.jsx?$": "<rootDir>/packages/scripts/config/babel-transform.js"
in the mainpackage.json
to make it work. Ideally, it shouldn't be possible at all when overriding Jest config.