Upgrade tests to use react/jsx-runtime #28252

acdlite · 2024-02-06T00:07:23Z

Instead of createElement.

We should have done this when we initially released jsx-runtime but better late than never. The general principle is that our tests should be written using the most up-to-date idioms that we recommend for users, except when explicitly testing an edge case or legacy behavior, like for backwards compatibility.

Most of the diff is related to tweaking test output and isn't very interesting.

I did have to workaround an issue related to component stacks. The component stack logic depends on shared state that lives in the React module. The problem is that most of our tests reset the React module state and re-require a fresh instance of React, React DOM, etc. However, the JSX runtime is not re-required because it's injected by the compiler as a static import. This means its copy of the shared state is no longer the same as the one used by React, causing any warning logged by the JSX runtime to not include a component stack. (This same issue also breaks string refs, but since we're removing those soon I'm not so concerned about that.) The solution I went with for now is to mock the JSX runtime with a proxy that re-requires the module on every function invocation. I don't love this but it will have to do for now. What we should really do is migrate our tests away from manually resetting the module state and use import syntax instead.

react-sizebot · 2024-02-06T00:12:31Z

Comparing: 4728548...987f515

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	176.37 kB	176.37 kB	=	54.96 kB	54.96 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.37 kB	178.37 kB	=	55.54 kB	55.54 kB
facebook-www/ReactDOM-prod.classic.js	=	590.79 kB	590.79 kB	=	104.26 kB	104.26 kB
facebook-www/ReactDOM-prod.modern.js	=	574.55 kB	574.55 kB	=	101.36 kB	101.36 kB
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
test_utils/ReactAllWarnings.js	Deleted	67.02 kB	0.00 kB	Deleted	16.42 kB	0.00 kB

Generated by 🚫 dangerJS against 987f515

rickhanlonii

sick

rickhanlonii · 2024-02-06T01:51:30Z

packages/react/src/__tests__/ReactJSXRuntime-test.js

-// A lot of these tests are pulled from ReactElement-test because
-// this api is meant to be backwards compatible.
-describe('ReactElement.jsx', () => {
+// NOTE: Prefer to call the JSXRuntime directly in these tests to eliminate so


to eliminate so

?

rickhanlonii · 2024-02-06T01:52:24Z

packages/react/src/__tests__/ReactJSXTransformIntegration-test.js

@@ -32,6 +40,19 @@ describe('ReactJSXElement', () => {
    };
  });

+  it('sanity check: test environment is configured to compile JSX to the jsx() runtime', async () => {


rickhanlonii · 2024-02-06T01:52:57Z

packages/shared/__tests__/describeComponentFrame-test.js

  });

-  // @gate !enableComponentStackLocations || !__DEV__
+  // @gate !enableComponentStackLocations
+  // @gate __DEV__


i like it, have you started preferring this over conditionals?

Mostly just because the diffs are nicer

gnoff · 2024-02-06T02:01:59Z

scripts/jest/devtools/setupEnv.js

+    {},
+    {
+      get(_, prop) {
+        return jest.requireActual('react/jsx-dev-runtime')[prop];


Shouldn't we proxy the functions exported by the runtime? the jsx bindings may not reevaluated after a reset still? I mean i must be working presumably since your tests are getting stacks but I wonder if this has edge cases that trapping the calls themselves wouldn't have?

this effectively does the same thing because this code runs each time any of the functions are called. So even though the bindings themselves never change, this inner code always reads the current module from the cache.

oh nvm I see the confusion... I thought I was already doing what your comment suggests but that's not how I implemented it.

Now I'm confused by why it's working.

Ah it works because the compiled output is still a method call. Which is unfortunate but not really our issue, it's Babel's.

I'll update it to proxy the function itself anyway. That's what I had intended to do, I just forget the extra layer and then didn't notice because it worked anyway.

cool sounds good

Instead of createElement. We should have done this when we initially released jsx-runtime but better late than never. The general principle is that our tests should b written using the most up-to-date idioms that we recommend for users, except when explicitly testing an edge case or legacy behavior, like for backwards compatibility. Most of the diff is related to tweaking test output and isn't very interesting. I did have to workaround an issue related to component stacks. The component stack logic depends on shared state that lives in the React module. The problem is that most of our tests reset the React module state and re-require a fresh instance of React, React DOM, etc. However, the JSX runtime is not re-required because it's injected by the compiler as a static import. This means its copy of the shared state is no longer the same as the one used by React, causing any warning logged by the JSX runtime to not include a component stack. (This same issue also breaks string refs, but since we're removing those soon I'm not so concerned about that.) The solution I went with for now is to mock the JSX runtime with a proxy that re-requires the module on every function invocation. I don't love this but it will have to do for now. What we should really do is migrate our tests away from manually resetting the module state and use import syntax instead.

#28252 broke RDT tests with React 16.x. These changes gate the `jsx-runtime` upgrade only for cases when we are testing against React >= 17. Validated with: ``` ./scripts/circleci/download_devtools_regression_build.js 16.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.5 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.5 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.8 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.8 --ci && ./scripts/circleci/download_devtools_regression_build.js 17.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 17.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 18.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 18.0 --ci ```

Instead of createElement. We should have done this when we initially released jsx-runtime but better late than never. The general principle is that our tests should be written using the most up-to-date idioms that we recommend for users, except when explicitly testing an edge case or legacy behavior, like for backwards compatibility. Most of the diff is related to tweaking test output and isn't very interesting. I did have to workaround an issue related to component stacks. The component stack logic depends on shared state that lives in the React module. The problem is that most of our tests reset the React module state and re-require a fresh instance of React, React DOM, etc. However, the JSX runtime is not re-required because it's injected by the compiler as a static import. This means its copy of the shared state is no longer the same as the one used by React, causing any warning logged by the JSX runtime to not include a component stack. (This same issue also breaks string refs, but since we're removing those soon I'm not so concerned about that.) The solution I went with for now is to mock the JSX runtime with a proxy that re-requires the module on every function invocation. I don't love this but it will have to do for now. What we should really do is migrate our tests away from manually resetting the module state and use import syntax instead.

…ook#28256) facebook#28252 broke RDT tests with React 16.x. These changes gate the `jsx-runtime` upgrade only for cases when we are testing against React >= 17. Validated with: ``` ./scripts/circleci/download_devtools_regression_build.js 16.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.5 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.5 --ci && ./scripts/circleci/download_devtools_regression_build.js 16.8 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 16.8 --ci && ./scripts/circleci/download_devtools_regression_build.js 17.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 17.0 --ci && ./scripts/circleci/download_devtools_regression_build.js 18.0 --replaceBuild && node ./scripts/jest/jest-cli.js --build --project devtools --release-channel=experimental --reactVersion 18.0 --ci ```

Instead of createElement. We should have done this when we initially released jsx-runtime but better late than never. The general principle is that our tests should be written using the most up-to-date idioms that we recommend for users, except when explicitly testing an edge case or legacy behavior, like for backwards compatibility. Most of the diff is related to tweaking test output and isn't very interesting. I did have to workaround an issue related to component stacks. The component stack logic depends on shared state that lives in the React module. The problem is that most of our tests reset the React module state and re-require a fresh instance of React, React DOM, etc. However, the JSX runtime is not re-required because it's injected by the compiler as a static import. This means its copy of the shared state is no longer the same as the one used by React, causing any warning logged by the JSX runtime to not include a component stack. (This same issue also breaks string refs, but since we're removing those soon I'm not so concerned about that.) The solution I went with for now is to mock the JSX runtime with a proxy that re-requires the module on every function invocation. I don't love this but it will have to do for now. What we should really do is migrate our tests away from manually resetting the module state and use import syntax instead. DiffTrain build for commit 952aa74.

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Feb 6, 2024

acdlite force-pushed the upgrade-tests-jsx-runtime branch 2 times, most recently from d677488 to 0110435 Compare February 6, 2024 00:24

acdlite marked this pull request as ready for review February 6, 2024 00:24

acdlite force-pushed the upgrade-tests-jsx-runtime branch 2 times, most recently from 5bec739 to 2ad22c4 Compare February 6, 2024 00:44

acdlite requested review from rickhanlonii and gnoff February 6, 2024 00:49

rickhanlonii approved these changes Feb 6, 2024

View reviewed changes

gnoff reviewed Feb 6, 2024

View reviewed changes

acdlite force-pushed the upgrade-tests-jsx-runtime branch from 2ad22c4 to 987f515 Compare February 6, 2024 02:29

acdlite merged commit 952aa74 into facebook:main Feb 6, 2024
36 checks passed

hoxyq mentioned this pull request Feb 6, 2024

fix: gate react/jsx-runtime upgrade only for React >= 17 tests #28256

Merged

ztanner mentioned this pull request Feb 9, 2024

Update React from 2bc7d336a to ba5e6a832 vercel/next.js#61837

Merged

ztanner mentioned this pull request Mar 19, 2024

(backport) Update React from 60a927d04 to 14898b6a9 vercel/next.js#63442

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade tests to use react/jsx-runtime #28252

Upgrade tests to use react/jsx-runtime #28252

acdlite commented Feb 6, 2024 •

edited

react-sizebot commented Feb 6, 2024 •

edited

rickhanlonii left a comment

rickhanlonii Feb 6, 2024

rickhanlonii Feb 6, 2024

rickhanlonii Feb 6, 2024

acdlite Feb 6, 2024

gnoff Feb 6, 2024 •

edited

acdlite Feb 6, 2024

acdlite Feb 6, 2024 •

edited

acdlite Feb 6, 2024

gnoff Feb 6, 2024

Upgrade tests to use react/jsx-runtime #28252

Upgrade tests to use react/jsx-runtime #28252

Conversation

acdlite commented Feb 6, 2024 • edited

react-sizebot commented Feb 6, 2024 • edited

Critical size changes

Significant size changes

rickhanlonii left a comment

Choose a reason for hiding this comment

rickhanlonii Feb 6, 2024

Choose a reason for hiding this comment

rickhanlonii Feb 6, 2024

Choose a reason for hiding this comment

rickhanlonii Feb 6, 2024

Choose a reason for hiding this comment

acdlite Feb 6, 2024

Choose a reason for hiding this comment

gnoff Feb 6, 2024 • edited

Choose a reason for hiding this comment

acdlite Feb 6, 2024

Choose a reason for hiding this comment

acdlite Feb 6, 2024 • edited

Choose a reason for hiding this comment

acdlite Feb 6, 2024

Choose a reason for hiding this comment

gnoff Feb 6, 2024

Choose a reason for hiding this comment

acdlite commented Feb 6, 2024 •

edited

react-sizebot commented Feb 6, 2024 •

edited

gnoff Feb 6, 2024 •

edited

acdlite Feb 6, 2024 •

edited