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

fix(hoist-plugin): hoist pure constants to support experimental JSX transform in mocks #10723

Merged
merged 9 commits into from
Nov 1, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Oct 27, 2020

Summary

Fixes #10690 (eventually, hopefully, possibly)

Test plan

Note that I don't want this test to actually land. It's a very brute force way of illustrating the bug, but a bad test to add to ensure we don't have regressions in the future pushed a better test now

@SimenB SimenB force-pushed the react-automatic-runtime-mock branch 2 times, most recently from d0d73fa to c07e8e8 Compare October 29, 2020 08:58
);

import { jsxDEV as _jsxDEV } from "react/jsx-dev-runtime";
var _jsxFileName = "";
Copy link
Member Author

Choose a reason for hiding this comment

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

one thing we could do is to wrap it in a function and replace usage - similar to what we do with _getJestObj now. then we wouldn't have to move it

@SimenB SimenB changed the title chore: add failing test for automatic react runtime transform fix(hoist-plugin): properly handle react development transform Oct 29, 2020
@SimenB SimenB force-pushed the react-automatic-runtime-mock branch from c07e8e8 to 3fba372 Compare October 31, 2020 23:18
@SimenB SimenB force-pushed the react-automatic-runtime-mock branch from 3fba372 to 3950686 Compare October 31, 2020 23:30
@SimenB
Copy link
Member Author

SimenB commented Nov 1, 2020

@nicolo-ribaudo @jeysal I pushed a commit that replaces the reference with its constant value. It works for this case, but I'd love to add some more tests to ensure it doesn't break other cases. Ideas? And if this approach is dumb, I have zero attachment to it and would love to throw it out if you've got any great ideas 😀

image

@SimenB SimenB force-pushed the react-automatic-runtime-mock branch from 41e050e to e751fbb Compare November 1, 2020 01:50
@SimenB
Copy link
Member Author

SimenB commented Nov 1, 2020

Through more trial & error than I'd like to admit I think I've gotten a function replacement working. I have no idea if that's a better approach or not. I've rebased and squashed some WIP commits so hopefully it'll be relatively simple for you to review the approaches. Commits are

  1. add a failing test
  2. add code to detect if it's a "safe" access which makes the test not throw, but don't actually do anything, so it's only safe if lazy accessed (which is the case for this test, but not necessarily true)
  3. replace the reference with its constant value
  4. inject a function which returns the constant value, and call that function instead

Again, feedback appreciated on which of 3 or 4 (if either) is the best solution 🙂

@SimenB SimenB marked this pull request as ready for review November 1, 2020 01:59
@SimenB SimenB force-pushed the react-automatic-runtime-mock branch from 0998a8b to 045af2b Compare November 1, 2020 02:46
@SimenB SimenB changed the title fix(hoist-plugin): properly handle react development transform fix(hoist-plugin): hoist pure constants to support experimental JSX transform in mocks Nov 1, 2020
@SimenB SimenB merged commit cfc90d2 into jestjs:master Nov 1, 2020
This was referenced Mar 12, 2021
@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 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 this pull request may close these issues.

Invalid variable access _jsxFileName
5 participants