-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[WIP] Upgrade entire suite to Babel 7 #6949
Conversation
if (exports !== this) { | ||
throw new Error('Invalid module context'); | ||
} | ||
// if (exports !== this) { |
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.
TODO, bring back.
@@ -8,3 +8,4 @@ | |||
|
|||
// prettier-ignore | |||
test('escape strings', () => expect('one: \\\'').toMatchSnapshot()); | |||
test('escape strings two', () => expect('two: \'"').toMatchSnapshot()); |
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.
should not be commited
scripts/build.js
Outdated
const transformOptions = JSON.parse( | ||
fs.readFileSync(path.resolve(__dirname, '..', '.babelrc'), 'utf8') | ||
); | ||
const transformOptions = require(path.resolve( |
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.
require('../babel.config.js')
I'm a fan of this, thanks for working on it! Would be nice to just drop babel 6 and not worry about it in a major. @cpojer @mjesun @thymikee @rickhanlonii thoughts on that? Not sure if that works out for FB? |
❤️ We're on 7, so feel free to go! That'd probably be a major, so we can also open the gates for breaking changes 😆 |
'@babel/preset-env', | ||
{ | ||
shippedProposals: true, | ||
targets: {node: '6'}, |
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 the lowest supported node version for Jest?
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.
It's 6 now.
We can use the official babel plugin, it has a |
packages/babel-jest/src/index.js
Outdated
import jestPreset from 'babel-preset-jest'; | ||
import {transform as babelTransform, util as babelUtil} from 'babel-core'; |
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.
Is it possible for process
to be async? Babel now exposes an async version of this function so that in the future we can start optionally allowing plugins some amount of async initialization. The more tooling that uses the async transforms, the better.
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.
In our case no, as we call it through require
which must be synchronous
@@ -20,7 +20,7 @@ import path from 'path'; | |||
import vm from 'vm'; | |||
import {createDirectory} from 'jest-util'; | |||
import fs from 'graceful-fs'; | |||
import {transform as babelTransform} from 'babel-core'; | |||
import {transform as babelTransform} from '@babel/core'; |
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.
FYI, babelTransform
may return null
if the file being compiled has been ignore
ed in Babel's config. It looks like this transformer doesn't handle that case. Looks like you'd want to return content;
in that case.
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.
Submitting this again since GitHub's review system is still pretty wonky...
I think we handle it, see https://github.com/facebook/jest/blob/025c6af5e4bd2a971f295fe9408f3d8c77ad187b/packages/jest-runtime/src/script_transformer.js#L214-L234
We only assign to transformed
if the transform returns something
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.
Oh you're right. The line I was worried about is https://github.com/facebook/jest/blob/025c6af5e4bd2a971f295fe9408f3d8c77ad187b/packages/jest-runtime/src/script_transformer.js#L155, but that actually shouldn't ever have any ignores anyway. There is an issue with that call though, so I'll add a comment down there.
scripts/build.js
Outdated
|
||
const transformOptions = JSON.parse( |
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.
Here, my recommendation would be
const transformOptions = {
cwd: path.resolve(__dirname, ".."),
// configFile: "babel.config.js" // optional, since the `cwd` is the root and it will autodetect.
babelrc: false,
};
Been out of town the last few days but thanks for the tips! Will implement these. |
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.
Thanks for chiming in @loganfsmyth!
@milesj 2 changes I'd love to see (which don't have to come as part of this PR) is:
@loganfsmyth thoughts on those 2? |
delete options.cacheDirectory; | ||
delete options.filename; | ||
|
||
const loadBabelOptions = filename => |
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.
@SimenB Updated to use loadPartialConfig
.
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.
please add supportsStaticESM: false
as well
I'm getting a lot of spurious test failures locally, especially for stuff like Mercurial and Haste. I've tried installing the Also can't seem to get Rollup to build no matter what config I try. Edit: Got Rollup working, had to set |
Not beyond installing mercury as you've already tried. What failures are you getting? |
Stuff like this:
|
import {Expect, ItBlock} from './parser_nodes'; | ||
// TODO: Change import to @babel/parser once types exist | ||
import type {File as BabylonFile} from 'babylon'; | ||
|
||
export type BabylonParserResult = { |
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.
@loganfsmyth @SimenB In Babylon, we were passing plugins: ['*']
, but this was removed in Babel 7. What's the best way to infer the plugin list from the Babel config? Since I can't hardcode all of these plugins because flow vs typescript, etc.
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.
@orta thoughts?
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've added a babel.parse
that takes all of Babel's normal options and will load your plugins and such to decide how to parse, so that's the recommendation for this type of thing now.
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.
One thing to note is, like my other comment, this will return null
if the file is ignore
ed in the Babel config. Your call if you want to then fall back to something else or parse with @babel/parser
with no plugins
or what.
}).code; | ||
}); | ||
|
||
return result ? result.code : content; |
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.
As noted in #6949 (comment), I was actually mistaken about needing to handle null
values here.
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.
Turns out this is required. Some tests broke after not checking for null.
@@ -152,9 +152,10 @@ export default class ScriptTransformer { | |||
} | |||
|
|||
_instrumentFile(filename: Path, content: string): string { | |||
return babelTransform(content, { | |||
const result = babelTransform(content, { | |||
auxiliaryCommentBefore: ' istanbul ignore next ', | |||
babelrc: false, |
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 call should have a configFile: false
alongside the babelrc: false
on it or else it risks applying the user's project-wide config during instrumentation.
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.
@loganfsmyth Perhaps you can help me debug the failing test for this, as it's currently throwing this.
● ScriptTransformer › writes source map if preprocessor supplies it
TypeError: Cannot read property 'mtime' of undefined
at fileMtime (node_modules/@babel/core/lib/config/files/utils.js:39:45)
While debugging, I noticed that console logs before the babelTransform
would display, but not after. I also noticed that /fruits/package.json
is being statSync
d (via makeStaticFileCache
), yet that path is NOT defined within the tests. That leads me to believe that it's caused by Babel, or the Istanbul plugin, but after debugging the plugin, it's never even hit. At this point I'm leaning towards it being a Babel config loading problem, but both babelrc
and configFile
are false, so I'm a bit lost here.
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.
Is something mocking statSync
maybe? That function should always return an object, or throw: https://github.com/babel/babel/blob/13798feefbf19389cb692aa0b5157276ce7cdf4f/packages/babel-core/src/config/files/utils.js#L21
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.
@loganfsmyth We do mock statSync
, but the weird thing is that /fruits/package.json
returns undefined while all other calls to statSync
return the mock. It's very confusing.
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.
Documenting for future debugging.
When I console log fs
before this line https://github.com/babel/babel/blob/13798feefbf19389cb692aa0b5157276ce7cdf4f/packages/babel-core/src/config/files/utils.js#L21, I can successfully see the entire module being mocked. However, when /fruits/package.json
is statSync
d, the value is no longer mocked and undefined is returned. I'm leaning towards this being an issue with the make*Cache
calls. Perhaps the cache is interfering?
packages/babel-jest/src/index.js
Outdated
@@ -18,75 +18,34 @@ import type { | |||
import crypto from 'crypto'; | |||
import fs from 'fs'; | |||
import path from 'path'; | |||
import { | |||
transform as babelTransform, | |||
util as babelUtil, |
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.
These utils don't exist in 7.x FYI.
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.
@SimenB Remove the util functionality completely? I noticed the canCompile
function being set in a few places.
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 suppose? It was probably an optimization, but I guess it doesn't matter as it's been removed
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.
All it did was check our list of suggested list of extensions, so we just exposed that instead: https://github.com/babel/babel/blob/master/packages/babel-core/src/index.js#L37 I'm not sure it's that useful for Jest anyway though, since you already limit it to .js
files by default.
packages/babel-jest/src/index.js
Outdated
@@ -95,19 +54,22 @@ const createTransformer = (options: any): Transformer => { | |||
configString: string, | |||
{instrument, rootDir}: CacheKeyOptions, | |||
): string { | |||
const babelOptions = loadBabelOptions(filename); | |||
const configPath = babelOptions.config || babelOptions.babelrc || ''; |
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.
Both of these could exist at the same time, so probably best to just hash both.
docs/TutorialReact.md
Outdated
}); | ||
} | ||
return src; | ||
return transform(src, { |
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 the return-value is falsy, this should return src
e2e/babel-plugin-jest-hoist/.babelrc
Outdated
} | ||
], | ||
"presets": ["@babel/preset-env", "@babel/preset-flow"], | ||
"plugins": ["jest-hoist"], | ||
"retainLines": true |
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.
can you remove this? shouldn't be needed
I'll be out of town for the next few weeks, so won't be around to work on this. Feel free to merge it into the branch. |
Ok, let's do it then. |
Opened up #7016 to have a place to keep discussing the code in context. Anyone wanting to help can open up PRs against the same branch, and it'll show up in that PR as we merge 🙂 |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
🚨 Note that this PR is not needed for projects using Jest to use Babel 7 🚨
Summary
Had a bit of downtime so thought I would look into this a bit. At the moment, the migration was easy, but there are a few troublesome deps that are blocking this.
babel-plugin-transform-inline-imports-commonjs
incompatabilities. (Switched to@babel/plugin-transform-modules-commonjs
+ lazy mode).babel-preset-react-native
incompatabilities. (Switched tometro-react-native-babel-preset
).react-native
(Using 0.57).Test plan
In progress.