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

Invalid variable access _jsxFileName #10690

Closed
renatoagds opened this issue Oct 23, 2020 · 22 comments · Fixed by #10723
Closed

Invalid variable access _jsxFileName #10690

renatoagds opened this issue Oct 23, 2020 · 22 comments · Fixed by #10723

Comments

@renatoagds
Copy link

renatoagds commented Oct 23, 2020

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

SimenB commented Oct 24, 2020

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

@SimenB
Copy link
Member

SimenB commented Oct 24, 2020

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

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

@renatoagds
Copy link
Author

renatoagds commented Oct 24, 2020

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

SimenB commented Oct 24, 2020

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

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

@SimenB
Copy link
Member

SimenB commented Oct 24, 2020

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

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

FezVrasta commented Oct 24, 2020

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

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

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

@SimenB
Copy link
Member

SimenB commented Oct 26, 2020

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

@SimenB is this specific to CRA?

@SimenB
Copy link
Member

SimenB commented Oct 27, 2020

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

SimenB commented Oct 27, 2020

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

SimenB commented Oct 27, 2020

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 #10690 (comment))

@nicolo-ribaudo
Copy link
Contributor

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

SimenB commented Oct 27, 2020

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
Copy link
Contributor

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

SimenB commented Oct 27, 2020

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

SimenB commented Nov 2, 2020

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

@github-actions
Copy link

This issue 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 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants