Updates babel-jest resolution #5932

arcanis · 2018-04-05T17:27:55Z

Summary

The babel-jest default transform is currently located through Resolver.findNodeModules. This has the inconvenient that when using a custom resolver, this new custom resolver will be completely bypassed.

Given that require.resolve is the standard and safe way to obtain the requirable path to a package, I think we should use it whenever possible, at least until the custom resolver has been properly setup (ie after the config has been normalized).

Two notes:

This might be a slight breaking change if users were specifying different version of babel-jest than the default. I'm not sure it's often the case, so it doesn't feel too problematic.
The regenerator package is still located using Resolver.findNodeModules. It would be nice to fix this as well, but since this package is meant to be added by the user, only the user can use require.resolve to obtain its path (we can't use process.main because it points to the Jest cli). Since it's quite more complex, it can be done later on.

Test plan

I've updated the tests to match the behavior (ie mostly checked that the resulting configuration was matching whatever is returned by require.resolve rather than an hardcoded node_modules/babel-jest).

arcanis · 2018-04-05T17:28:09Z

cc @mjesun @cpojer

cpojer · 2018-04-05T17:34:58Z

packages/jest-config/src/__tests__/normalize.test.js

@@ -684,7 +684,9 @@ describe('babel-jest', () => {
  beforeEach(() => {
    Resolver = require('jest-resolve');
    Resolver.findNodeModule = jest.fn(
-      name => path.sep + 'node_modules' + path.sep + name,
+      name => {


can you remove the curly brace + return again? Doesn't seem necessary here and Jest's code style is to not use these when it's a single expression.

cpojer · 2018-04-05T17:35:21Z

packages/jest-config/src/__tests__/normalize.test.js

@@ -684,7 +684,9 @@ describe('babel-jest', () => {
  beforeEach(() => {
    Resolver = require('jest-resolve');
    Resolver.findNodeModule = jest.fn(
-      name => path.sep + 'node_modules' + path.sep + name,
+      name => {
+        return name.indexOf('babel-jest') === -1 ? path.sep + 'node_modules' + path.sep + name : name;


Shouldn't this be name === 'babel-jest'?

cpojer · 2018-04-05T17:36:55Z

packages/jest-config/src/normalize.js

@@ -113,24 +113,20 @@ const setupBabelJest = (options: InitialOptions) => {
    });

    if (customJSPattern) {
-      const jsTransformer = Resolver.findNodeModule(


You are removing the resolution for packages that are not babel-jest, like ts-jest or ./my-custom-file or my-custom-transformer. You still need to support those as well.

Hm, I see what you mean. Do you think it would be reasonable to have the same findNodeModule call as a fallback to avoid breaking the existing, but encourage people to use require.resolve in their configuration?

module.exports = { transform: { "*.ts": require.resolve('ts-jest') } };

An issue is that it wouldn't work as-is in the package.json configuration, since functions can't be called here, so it would have to be kept inside a js configuration file.

An alternative would be to defer resolving those paths until after the configuration has been completed, and then use the right resolver - but the same issue will occur for the resolver option, which ironically will need to be manually resolved by the user through require.resolve.

What if in Jest we call Resolver.findNodeModule(value). If it returns null, we call Resolver.findNodeModule(require.resolve(value)) (or just require.resolve(value) itself if possible). That way it doesn't affect users.

But then it will resolve them relative to jest-config, not the user project. So ts-jest for example could fail because it would not be listed as a dependency of jest-config (whereas a require.resolve from the configuration file would always work, because the configuration file would be in the toplevel project).

Oh right, yeah this is a bit of a bummer :( I'd prefer not to demand people to rewrite their config with JS.

Actually I just read the code again and I don't think this PR prevents external transformers from working - setupBabelJest is only used to detect babel jest and, if it can be found, automatically inject regenerator. Any other loader will be resolved and required later, through the regular resolver (cf here).

cpojer · 2018-04-05T17:38:12Z

I left a few comments. I think this will break resolution of custom transformers which we'll need to support. Can you fix that?

cpojer

Yes, you are right. Assuming all tests are passing, this is looks good to me actually :) Thanks!

arcanis · 2018-04-10T12:43:08Z

The failing tests are driving me crazy - if I update the snapshots they break in node 6, but if I don't they break in 8/9 :/ They don't look related to my diff either ... what do you think?

SimenB · 2018-04-10T12:53:24Z

Are you rebased on master? We use sourcemaps (provided by babel through babel-jest) to make the stacks correct, seems like it's off somehow on different nodes?

arcanis · 2018-04-10T18:02:26Z

@SimenB Yep, rebased on master. My theory is that the current codebase has an issue with Node 8/9 that was hidden in this test until now. The problem is on the following condition: https://github.com/facebook/jest/pull/5932/files#diff-8ee186ad0e07dceb57c11f7c7b5d072aR121

Before my commit, Jest was sometimes failing to locate babel-jest, probably because the tests were running inside a temporary directory. Due to this, the transpilation wasn't actually being ran at all, and the correct results were being returned on Node 8/9.

Starting from my commit, babel-jest is now always resolved (by using the one that Jest depends on), so the transpilation always run, and we apparently always hit the bug where the sourcemaps aren't correctly computed on Node 8/9.

What do you think?

codecov-io · 2018-04-10T19:51:27Z

Codecov Report

Merging #5932 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5932      +/-   ##
==========================================
+ Coverage   64.33%   64.34%   +0.01%     
==========================================
  Files         217      217              
  Lines        8286     8286              
  Branches        4        3       -1     
==========================================
+ Hits         5331     5332       +1     
+ Misses       2954     2953       -1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/normalize.js	`94.04% <100%> (+0.02%)`	⬆️
packages/jest-environment-jsdom/src/index.js	`40% <0%> (+0.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1a51672...76adee3. Read the comment docs.

SimenB · 2018-04-10T19:56:04Z

It's green now 🤷‍♂️

Edit: ah, you removed the line and column. Not sure if that's the correct fix... (Might be, have to give it a thought or two)

arcanis · 2018-04-10T20:08:49Z

Yeah, we discussed it with @mjesun and figured it would be good enough until we get to make another PR to fix the sourcemap thing (since it seems caused by something outside of the PR, and only affect projects that don't have babel-jest available, which I'm not sure can happen under normal circumstances), but if you feel otherwise let me know!

…

On Tue, Apr 10, 2018, 12:56 PM Simen Bekkhus ***@***.***> wrote: It's green now 🤷‍♂️ — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5932 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_WaxraZt_yxK5cEe7ggwRrLlMAmLvzks5tnQ5egaJpZM4TI25k> .

SimenB · 2018-04-10T20:18:31Z

I'm not comfortable moving away from working sourcemaps, even though they might be working through a happy accident (working by accident is better than not working :D)

If you can tell me how the current approach is wrong with regards to sourcemaps, I'll be happy to remove my objection (to the degree it matters)

arcanis · 2018-04-10T20:58:11Z

@SimenB this PR doesn't actually break the sourcemaps - they're already broken (to some extent). To reproduce, just create a new project with 23.0.0-alpha.5, add a testfile containing the following code, and run it. It will crash with an invalid column on Node 9 (but works on Node 6), printing 1 instead of 5:

    it('it', () => {});
    it('it foo');
    test('test bar');

I think the issue simply comes from babel just keeping track of the rows instead of rows+columns.

SimenB · 2018-04-10T21:27:42Z

Is it just broken in the alphas (meaning master) or in the stable 22 releases as well?

arcanis · 2018-04-10T23:25:05Z

@SimenB Not sure. Before the 23.0.0, not entering the test definition wasn't a hard error (it was just skipping the test altogether), and regular errors thrown from within the test definition don't seem to suffer from this issue in both 22.X and 23.X.

SimenB · 2018-04-11T07:17:55Z

Might mess up #5889 as well, then

arcanis · 2018-04-11T16:00:34Z

Possibly. Are you ok with this PR then?

…

On Wed, Apr 11, 2018, 12:18 AM Simen Bekkhus ***@***.***> wrote: Might mess up #5889 <#5889> as well, then — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#5932 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA_Waw51GDgQ7U2m8Uc46uycY3ik0VLCks5tna4tgaJpZM4TI25k> .

SimenB

If this does mess up the stack traces, we might want to revert. But the change itself is nice, so I think we should try 🙂

arcanis · 2018-04-11T20:58:42Z

👍

SimenB · 2018-04-13T01:55:12Z

This does indeed mess up #5889 (I've rebased it), but just for the globals test (same one you had to strip out the line and column info in this PR). I think I'll have to dig into why this change makes us lose column information

github-actions · 2021-05-12T17:24:11Z

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.

cpojer reviewed Apr 5, 2018

View reviewed changes

facebook-github-bot added the cla signed label Apr 5, 2018

cpojer approved these changes Apr 6, 2018

View reviewed changes

arcanis force-pushed the babel-jest-resolution branch 2 times, most recently from d2681a8 to d6d9fc8 Compare April 10, 2018 10:21

Uses require.resolve to locate babel-jest

169a91f

arcanis force-pushed the babel-jest-resolution branch from d6d9fc8 to 22f605f Compare April 10, 2018 19:30

Updates snapshots

76adee3

arcanis force-pushed the babel-jest-resolution branch from 22f605f to 76adee3 Compare April 10, 2018 19:37

This comment has been minimized.

Sign in to view

SimenB approved these changes Apr 11, 2018

View reviewed changes

arcanis merged commit 90810b1 into jestjs:master Apr 11, 2018

SimenB mentioned this pull request May 2, 2018

Revert changes for source map suppport (#5945) #6106

Merged

This was referenced Oct 5, 2018

After upgrade to CRA 2.0, tests run very slow. facebook/create-react-app#5304

Closed

Investigate speed regressions #7110

Closed

SimenB mentioned this pull request Apr 3, 2019

jest 24 with babel 6 and babel-jest 23 #8230

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates babel-jest resolution #5932

Updates babel-jest resolution #5932

arcanis commented Apr 5, 2018

arcanis commented Apr 5, 2018

cpojer Apr 5, 2018

cpojer Apr 5, 2018

cpojer Apr 5, 2018

arcanis Apr 5, 2018 •

edited

Loading

arcanis Apr 5, 2018 •

edited

Loading

cpojer Apr 5, 2018

arcanis Apr 5, 2018

cpojer Apr 5, 2018

arcanis Apr 6, 2018 •

edited

Loading

cpojer commented Apr 5, 2018

cpojer left a comment

arcanis commented Apr 10, 2018

SimenB commented Apr 10, 2018 •

edited

Loading

arcanis commented Apr 10, 2018 •

edited

Loading

codecov-io commented Apr 10, 2018

SimenB commented Apr 10, 2018 •

edited

Loading

arcanis commented Apr 10, 2018 via email

SimenB commented Apr 10, 2018

arcanis commented Apr 10, 2018 •

edited

Loading

SimenB commented Apr 10, 2018

This comment has been minimized.

arcanis commented Apr 10, 2018

SimenB commented Apr 11, 2018

arcanis commented Apr 11, 2018 via email

SimenB left a comment

arcanis commented Apr 11, 2018

SimenB commented Apr 13, 2018

github-actions bot commented May 12, 2021

Updates babel-jest resolution #5932

Updates babel-jest resolution #5932

Conversation

arcanis commented Apr 5, 2018

Summary

Test plan

arcanis commented Apr 5, 2018

cpojer Apr 5, 2018

Choose a reason for hiding this comment

cpojer Apr 5, 2018

Choose a reason for hiding this comment

cpojer Apr 5, 2018

Choose a reason for hiding this comment

arcanis Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

arcanis Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

cpojer Apr 5, 2018

Choose a reason for hiding this comment

arcanis Apr 5, 2018

Choose a reason for hiding this comment

cpojer Apr 5, 2018

Choose a reason for hiding this comment

arcanis Apr 6, 2018 • edited Loading

Choose a reason for hiding this comment

cpojer commented Apr 5, 2018

cpojer left a comment

Choose a reason for hiding this comment

arcanis commented Apr 10, 2018

SimenB commented Apr 10, 2018 • edited Loading

arcanis commented Apr 10, 2018 • edited Loading

codecov-io commented Apr 10, 2018

Codecov Report

SimenB commented Apr 10, 2018 • edited Loading

arcanis commented Apr 10, 2018 via email

SimenB commented Apr 10, 2018

arcanis commented Apr 10, 2018 • edited Loading

SimenB commented Apr 10, 2018

This comment has been minimized.

arcanis commented Apr 10, 2018

SimenB commented Apr 11, 2018

arcanis commented Apr 11, 2018 via email

SimenB left a comment

Choose a reason for hiding this comment

arcanis commented Apr 11, 2018

SimenB commented Apr 13, 2018

github-actions bot commented May 12, 2021

arcanis Apr 5, 2018 •

edited

Loading

arcanis Apr 5, 2018 •

edited

Loading

arcanis Apr 6, 2018 •

edited

Loading

SimenB commented Apr 10, 2018 •

edited

Loading

arcanis commented Apr 10, 2018 •

edited

Loading

SimenB commented Apr 10, 2018 •

edited

Loading

arcanis commented Apr 10, 2018 •

edited

Loading