fix: hoist imports of @jest/globals correctly

[SimenB]

Summary

This doesn't work. Output after running babel on it is

"use strict";

_globals.jest.mock('virtual-module', function () {
  return 'kiwi';
}, {
  virtual: true
});

var _globals = require("@jest/globals");

/**
 * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */
// The virtual mock call below will be hoisted above this `require` call.
var virtualModule = require('virtual-module');

test('works with virtual modules', function () {
  expect(virtualModule).toBe('kiwi');
});

So the mock call is correctly hoisted (which is probably a bug actually, we should only hoist the "global" jest, not any variable named jest), but the require call is not high enough.

@jeysal @nicolo-ribaudo any help appreciated - I'm on very thin ground when doing AST modifications 😀

Test plan

Test added.

[SimenB]

Should probably look for a require instead (or as well?)

[SimenB]

I tried this:

const visitor: Visitor = {
  CallExpression(path) {
    const callee = path.get('callee') as NodePath;
    const callArguments = path.get('arguments');

    if (
      callee.isIdentifier() &&
      callee.node.name === 'require' &&
      callArguments.length === 1
    ) {
      const [argument] = callArguments;

      if (
        argument.isStringLiteral() &&
        argument.node.value === '@jest/globals'
      ) {
        // @ts-ignore: private, magical property
        path.node._blockHoist = Infinity;
      }
    }
  },
};

and it gets into the _blockHoist thing, but doesn't seem to actually hoist it. Halp? 😀

[SimenB]

New try:

const visitor: Visitor = {
  VariableDeclaration(path) {
    const declarations = path.get('declarations');
  
    if (declarations.length === 1) {
      const declarationInit = declarations[0].get('init');
  
      if (declarationInit.isCallExpression()) {
        const callee = declarationInit.get('callee') as NodePath;
        const callArguments = declarationInit.get('arguments') as Array<
          NodePath
        >;
  
        if (
          callee.isIdentifier() &&
          callee.node.name === 'require' &&
          callArguments.length === 1
        ) {
          const [argument] = callArguments;
  
          if (
            argument.isStringLiteral() &&
            argument.node.value === '@jest/globals'
          ) {
            // @ts-ignore: private, magical property
            path.node._blockHoist = Infinity;
          }
        }
      }
    }
  }
}

This seems to hoist the require, but not above the jest.mock call.

"use strict";

_globals.jest.mock('virtual-module', function () {
  return 'kiwi';
}, {
  virtual: true
});

var _globals = require("@jest/globals");

/**
 * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */
// The virtual mock call below will be hoisted above this `require` call.
var virtualModule = require('virtual-module');

test('works with virtual modules', function () {
  expect(virtualModule).toBe('kiwi');
});

Getting pretty close though 🙂

EDIT: wait no, that's the same as not having it at all 😅 I give up

[SimenB]

I pushed up that last one, since it feels like it's somewhere close

[SimenB]

Another issue here is that we only hoist jest, so import {jest as j} from 'jest' won't get hoisted. This might require some larger change in the plugin?

[ahnpnl]

@SimenB is there a way I can get @jest/globals package ? I think ts-jest hoisting might need to change too since it has similar logic to babel-plugin-jest-hoist.

[SimenB]

I'll publish after fixing this bug

[jeysal]

@ahnpnl thanks for taking the initiative regarding ts-jest! Even a beta release right now is probably not a good idea given that we know this is a problem, but once it's released we can ping you, I think that's the best way to go about it :)

[jeysal]

@SimenB I'll take a shot 🙃

[nicolo-ribaudo]

@SimenB I can take a look at this tomorrow!

EDIT: @jeysal feel free to ask if you need any help with the Babel part.

[jeysal]

I have done some well-deserved cleanup in jeysal@fdecf04 and went in a direction with the visitor that we might want to go in (similar to what I understand _blockHoist causes internally in Babel), but didn't quite have the time to fine tune and get all the test cases green yet.

[SimenB]

Ah, that's great @jeysal! The plugin definitely needs some TLC and if we can get rid of the _blockHoist usage at the same time that'd be wonderful. Does it make sense to land a refactor to not using _blokcHoist first, and then tackling the added complexity of hoisting the import statement once that's in?

[SimenB]

@jeysal @nicolo-ribaudo any progress? 🙏 I'd love to release this, but with this bug I'd rather revert and re-land later. If you don't have time that of course perfectly fine! 😀

[jeysal]

I was hoping to get back to it on one of the evenings this week but that didn't happen :/ maybe tonight or tomorrow

[nicolo-ribaudo]

@jeysal Please ping me when you are done, so that I can review it!

[SimenB]

Woo, thanks both!

[nicolo-ribaudo]

I think an easy way of safely fixing this is to rely on what is natively hoisted: functions. The output in the PR description would become something like this:

"use strict";

_globals().jest.mock('virtual-module', function () {
  return 'kiwi';
}, {
  virtual: true
});

function _globals() {
  var tmp = require("@jest/globals");
  _globals = () => tmp;
  return tmp;
}

/**
 * Copyright (c) Facebook, Inc. and its affiliates. All Rights Reserved.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 */
// The virtual mock call below will be hoisted above this `require` call.
var virtualModule = require('virtual-module');

test('works with virtual modules', function () {
  expect(virtualModule).toBe('kiwi');
});

By dining so, we don't have to worry about _blokcHoist or possible conflicts with other plugins which might inject other things before the require call.

[SimenB]

Ah, that's a great idea!

[SimenB]

@nicolo-ribaudo wanna open up a separate PR with that? Feel free to take the test from this one.

The whole plugin probably needs some more tlc regardless, but it's a good start, and something I feel comfortable shipping

[jeysal]

@nicolo-ribaudo also, it might be good to start from the commit I posted (jeysal@fdecf04) since there's a lot of cleanup in there. I already don't remember what exactly and not sure if you aren't gonna end up rewriting everything anyway but yeah maybe do that if you can. It also has all the tests (including new one)

[jeysal]

Oh sorry I thought you were preparing a PR based on 'looking at this right now'. But I can also definitely take over again with your guidance!
I will try to get to it tonight or tomorrow and incorporate your suggestion into my branch :)

[nicolo-ribaudo]

I wrote "looking at this right now" before seeing your comment, then I deleted it because I didn't want to "steal" it from you 😛
Feel free to go ahead, and I can review your changes when they are ready.

[jeysal]

👍 cool will do

[jeysal]

Very sorry I didn't get back to it this weekend. I'll try to make some time during the workdays next week to make sure to it asap

[SimenB]

CI seems like it wants to revert the snapshot changes you made?

[jeysal]

On my machine the 3 snapshot updates (incl. the stack trace ones) no longer try to update after a --clear-cache

[SimenB]

On my machine the 3 snapshot updates (incl. the stack trace ones) no longer try to update after a --clear-cache

We might wanna add --no-cache to those tests. Or include the version of the hoist plugin in the cache key of babel-jest

[jeysal]

Definitely the latter I think. Although just version doesn't help for development in the Jest repo?

[SimenB]

We can read its source. We include babel-jest's own source in the cache key, trivial to include the plugin as well (although it's the preset we load, so maybe not...)

[jeysal]

All greeeeeeeeen 🎉 would be great to get another look by @nicolo-ribaudo then ship 👌

[nicolo-ribaudo]

Is it a problem if I review this tomorrow instead of today?

[jeysal]

No no totally not! ❤️

[jeysal]

@nicolo-ribaudo gentle ping, if you don't have time to review just let us know, I'm mostly confident in this and we could land without 😄

[nicolo-ribaudo]

Not a blocker for this PR, but what do you think about adding some input/output tests for this plugin, as we do in the Babel repository? That way if something breaks it's easier to identify why.

[jeysal]

Def agree about the tests, I really don't like the current test suite, it's very confusing.
I even considered rewriting the test suite at one point, but time is very sparse atm :/

[SimenB]

Yeah, we should definitely add some unit tests as well that's just input=>output. I like the current tests, but they are integration tests, not unit tests. Probably not for this PR, though 😀

[jeysal]

Added the unit tests rewrite to my legendary Jest todo list 👌 😂

[SimenB]

Thanks for landing this @jeysal, and thanks for your help @nicolo-ribaudo!

[github-actions]

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.