Jest: Invalid variable access _jsxFileName

[renatoagds]

🐛 Bug Report

After upgrading to new JSX transform, started to receive an error in my mocks that's returning a React Component/Element, saying about the _jsxFileName variable being outside of scope and could not be accessed.

To Reproduce

Just mock a React Component/Element and start using the new JSX Transform

jest.mock('./MyComponent', () => () => <div id="my-component" />)

[SimenB]

PR welcome 👍 Error is in babel-plugin-jest-hoist somewhere.

[SimenB]

That said, I'm not 100% certain what the solution is. We could add it to the whitelist, but that feels like it's addressing the symptom rather than the cause. @nicolo-ribaudo what do you think?

[renatoagds]

@SimenB For sure ... Will work on this one 👍

[renatoagds]

We could add it to the whitelist, but that feels like it's addressing the symptom rather than the cause.

I was thinking doing that (adding into the whitelist), do you have other solution?

[SimenB]

Currently the whitelist is globals and known symbols that jest controls, just adding stuff that "random" babel plugins inject seems like a slippery slope. I'm hoping we can solve it more generically via some babel magic. Might not be possible though! @rickhanlonii on the react team is gonna take a look, so let's wait and see what he says 🙂

If it proves difficult we should just add it to the whitelist to unblock, tho 👍

[FezVrasta]

Would (yet) another configuration option make sense? Today is React who needs this variable, tomorrow may be _framework x_ that needs another one.

[SimenB]

Right, which is why I wanna solve it more generically than adding to the whitelist, and hopefully without the user having to specify any config

[renatoagds]

Currently the whitelist is globals and known symbols that jest controls, just adding stuff that "random" babel plugins inject seems like a slippery slope. I'm hoping we can solve it more generically via some babel magic.

Makes sense. Agreed

If it proves difficult we should just add it to the whitelist to unblock, tho 👍

Sure. Thanks!

[FezVrasta]

Alternatively the plugin could also allow variables prefixed by an underscore (_*), so that it's generic enough for other libraries to follow the same convention if they need to. I know right now it supports mock prefixed variables, so it wouldn't be much different.

Meanwhile, anybody with the same problem can use this patch with patch-package (save it in patches/babel-plugin-jest-hoist+26.5.0.patch):

diff --git a/node_modules/babel-plugin-jest-hoist/build/index.js b/node_modules/babel-plugin-jest-hoist/build/index.js
index 6128021..46f2824 100644
--- a/node_modules/babel-plugin-jest-hoist/build/index.js
+++ b/node_modules/babel-plugin-jest-hoist/build/index.js
@@ -141,7 +141,8 @@ FUNCTIONS.mock = args => {
       if (!found) {
         const isAllowedIdentifier =
           (scope.hasGlobal(name) && ALLOWED_IDENTIFIERS.has(name)) ||
-          /^mock/i.test(name) || // Allow istanbul's coverage variable to pass.
+          /^mock/i.test(name) ||
+          /^_/i.test(name) || // Allow istanbul's coverage variable to pass.
           /^(?:__)?cov/.test(name);

         if (!isAllowedIdentifier) {

[rickhanlonii]

If this plugin ran before the React plugin then I would expect this to work because the injected React functions would be in scope. Is there a repro repo I can test on?

[renatoagds]

@rickhanlonii I will reproduce that in a public repo and get back to you

[SimenB]

@rickhanlonii you can reproduce using CRA

$ npx create-react-app my-app
$ cd my-app
$ git apply mock.patch
$ npm test
ReferenceError: /Users/simen/repos/my-app/src/App.test.js: The module factory of `jest.mock()` is not allowed to reference any out-of-scope variables.
    Invalid variable access: _jsxFileName

mock.patch:

diff --git i/src/App.test.js w/src/App.test.js
index 1f03afe..ba15b2b 100644
--- i/src/App.test.js
+++ w/src/App.test.js
@@ -1,6 +1,8 @@
 import { render, screen } from '@testing-library/react';
 import App from './App';

+jest.mock('./MyComponent', () => () => <div id="my-component" />, { virtual: true })
+
 test('renders learn react link', () => {
   render(<App />);
   const linkElement = screen.getByText(/learn react/i);

[rickhanlonii]

@SimenB is this specific to CRA?

[SimenB]

Testing locally - it only happens with the dev mode transform, not the "normal" (prod?) transform. I.e. passing development: true to the babel preset. So the injected runtime import is not an issue for whatever reason, but the injected var _jsxFileName is.

See #10723 for a failing test

[SimenB]

So a workaround is to somehow disable "dev" mode for the transform when running Jest.

https://github.com/facebook/create-react-app/blob/027b03ba8d689e619a912ed0d72c3a11ef22ac2f/packages/babel-preset-react-app/create.js#L95

However, I'd say it's still a bug even if CRA were to work around it.

[SimenB]

One other thing to note is that it's not enough to add it to the whitelist for us, since the scope.hasGlobal check fails: https://github.com/facebook/jest/blob/7b1568d9162fafe6a630fb425f16a44998894164/packages/babel-plugin-jest-hoist/src/index.ts#L134

Would need to add it to the whole condition (similar to https://github.com/facebook/jest/issues/10690#issuecomment-715941252)

[nicolo-ribaudo]

Maybe if a mock relies on a constant pure variable in the outer scope, that variable can be hoisted to the top of the file?

[SimenB]

That sounds reasonable @nicolo-ribaudo. We'd do that in our hoist plugin, right? Seems reasonable. Any idea on how to achieve that?

[nicolo-ribaudo]

In the logic which throws about variables in the outer scope, you can do something like this:

const binding = scope.bindings[name];
if (binding.constant && scope.isPure(binding.path.node.init, true)) {
  // hoist binding.path.node
}

[SimenB]

Which scope?

https://github.com/facebook/jest/blob/7b1568d9162fafe6a630fb425f16a44998894164/packages/babel-plugin-jest-hoist/src/index.ts#L115-L130

parentScope? Or inside the while? Latter doesn't make sense I guess since then found would be set to true, so parentScope makes sense.

Something like this (I've ignored whitespace since I've changed to an early return rather than keeping track via found)?

diff --git i/packages/babel-plugin-jest-hoist/src/index.ts w/packages/babel-plugin-jest-hoist/src/index.ts
index 8fbab87dc0..15b412c594 100644
--- i/packages/babel-plugin-jest-hoist/src/index.ts
+++ w/packages/babel-plugin-jest-hoist/src/index.ts
@@ -117,19 +117,28 @@ FUNCTIONS.mock = args => {
     moduleFactory.traverse(IDVisitor, {ids});
     for (const id of ids) {
       const {name} = id.node;
-      let found = false;
       let scope = id.scope;

       while (scope !== parentScope) {
         if (scope.bindings[name]) {
-          found = true;
-          break;
+          return true;
         }

         scope = scope.parent;
       }

-      if (!found) {
+      const binding = parentScope.bindings[name];
+
+      if (
+        binding?.constant &&
+        parentScope.isPure(binding.path.node.init, true)
+      ) {
+        // @ts-expect-error: private, magical property
+        binding.path.node._blockHoist = Infinity;
+
+        return true;
+      }
+
       const isAllowedIdentifier =
         (scope.hasGlobal(name) && ALLOWED_IDENTIFIERS.has(name)) ||
         /^mock/i.test(name) ||
@@ -153,7 +162,6 @@ FUNCTIONS.mock = args => {
         );
       }
     }
-    }

     return true;
   }

_blockHoist thing doesn't seem to make an actual difference, so I don't think it's correct (i.e. the transpiled code looks identical whether or not I put it there). Also, feels like it'll give the wrong result if somebody uses shadowing? Or is that covered by the constant and isPure check? var _jsxFileName is hoisted by JS tho, so possibly that an issue? Dunno 😀 I'm on way shaky ground

The test I put together in #10723 passes with this change, fwiw.

---

As an aside, binding.path.node.init gives a type error since it "doesn't exist" but that's probably just an error in @types/babel__traverse or a missing generic on our side

[SimenB]

Fix released in https://github.com/facebook/jest/releases/tag/v26.6.2. Many thanks to @nicolo-ribaudo for helping out with this 🙏