Skip to content
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

Merged
merged 46 commits into from
Sep 21, 2018
Merged

[WIP] Upgrade entire suite to Babel 7 #6949

merged 46 commits into from
Sep 21, 2018

Conversation

milesj
Copy link

@milesj milesj commented Sep 5, 2018

🚨 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.

  • Resolve babel-plugin-transform-inline-imports-commonjs incompatabilities. (Switched to @babel/plugin-transform-modules-commonjs + lazy mode).
  • Resolve babel-preset-react-native incompatabilities. (Switched to metro-react-native-babel-preset).
  • Resolve react-native (Using 0.57).

Test plan

In progress.

if (exports !== this) {
throw new Error('Invalid module context');
}
// if (exports !== this) {
Copy link
Author

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());
Copy link
Member

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(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

require('../babel.config.js')

@SimenB
Copy link
Member

SimenB commented Sep 6, 2018

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?

@mjesun
Copy link
Contributor

mjesun commented Sep 6, 2018

❤️ 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'},
Copy link
Author

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 6 now.

@SimenB
Copy link
Member

SimenB commented Sep 7, 2018

Resolve babel-plugin-transform-inline-imports-commonjs incompatabilities

We can use the official babel plugin, it has a lazy mode: https://babeljs.io/docs/en/babel-plugin-transform-modules-commonjs#lazy. Not sure if the env preset supports it as an option or not

import jestPreset from 'babel-preset-jest';
import {transform as babelTransform, util as babelUtil} from 'babel-core';

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.

Copy link
Member

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';

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 ignoreed 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.

Copy link
Member

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

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(

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,
};

@milesj
Copy link
Author

milesj commented Sep 10, 2018

Been out of town the last few days but thanks for the tips! Will implement these.

Copy link
Member

@SimenB SimenB left a 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!

@SimenB
Copy link
Member

SimenB commented Sep 10, 2018

@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 =>
Copy link
Author

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.

Copy link
Member

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

@milesj
Copy link
Author

milesj commented Sep 10, 2018

I'm getting a lot of spurious test failures locally, especially for stuff like Mercurial and Haste. I've tried installing the hg binary, but that doesn't resolve it. Are there docs on setting up the test suite locally?

Also can't seem to get Rollup to build no matter what config I try.

Edit: Got Rollup working, had to set configFile: false.

@SimenB
Copy link
Member

SimenB commented Sep 10, 2018

Are there docs on setting up the test suite locally?

Not beyond installing mercury as you've already tried. What failures are you getting?

@milesj
Copy link
Author

milesj commented Sep 10, 2018

Stuff like this:

 FAIL  packages/jest-haste-map/src/__tests__/index.test.js
  ● HasteMap › builds a haste map on a fresh cache

    TypeError: Cannot read property 'clocks' of null

      at hasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:272:19)

  ● HasteMap › builds a haste map on a fresh cache with SHA-1s › uses watchman: false

    TypeError: Cannot read property 'files' of null

      at Object.<anonymous> (packages/jest-haste-map/src/__tests__/index.test.js:359:21)

  ● HasteMap › builds a haste map on a fresh cache with SHA-1s › uses watchman: true

    TypeError: Cannot read property 'files' of null

      at Object.<anonymous> (packages/jest-haste-map/src/__tests__/index.test.js:359:21)

  ● HasteMap › does not crawl native files even if requested to do so

    TypeError: Cannot read property 'map' of null

      at Object.<anonymous> (packages/jest-haste-map/src/__tests__/index.test.js:419:17)

  ● HasteMap › retains all files if `retainAllFiles` is specified

    TypeError: Cannot read property 'files' of null

      at hasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:441:19)

  ● HasteMap › warns on duplicate module ids

    TypeError: Cannot read property 'map' of null

      at HasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:492:21)

  ● HasteMap › splits up modules by platform

    TypeError: Cannot read property 'files' of null

      at HasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:542:21)

  ● HasteMap › does not access the file system on a warm cache with no changes

    TypeError: Cannot read property 'clocks' of null

      at HasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:598:25)

  ● HasteMap › only does minimal file system access when files change

    TypeError: Cannot read property 'clocks' of null

      at HasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:638:25)

  ● HasteMap › correctly handles file deletions

    TypeError: Cannot read property 'files' of null

      at HasteMap.build.then (packages/jest-haste-map/src/__tests__/index.test.js:674:46)

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 = {
Copy link
Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@orta thoughts?

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.

Copy link

@loganfsmyth loganfsmyth Sep 11, 2018

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 ignoreed 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;

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.

Copy link
Author

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,

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.

Copy link
Author

@milesj milesj Sep 12, 2018

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 statSyncd (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.

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

Copy link
Author

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.

Copy link
Author

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 statSyncd, 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?

@@ -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,

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.

Copy link
Author

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.

Copy link
Member

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

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.

@@ -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 || '';

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.

});
}
return src;
return transform(src, {
Copy link
Member

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

}
],
"presets": ["@babel/preset-env", "@babel/preset-flow"],
"plugins": ["jest-hoist"],
"retainLines": true
Copy link
Member

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

@milesj
Copy link
Author

milesj commented Sep 21, 2018

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.

@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

Ok, let's do it then.

@SimenB SimenB merged commit e47e545 into jestjs:babel-7 Sep 21, 2018
@SimenB
Copy link
Member

SimenB commented Sep 21, 2018

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 🙂

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.