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

Upgrade tests to use react/jsx-runtime #28252

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Feb 6, 2024

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.

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

react-sizebot commented Feb 6, 2024

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

@acdlite acdlite force-pushed the upgrade-tests-jsx-runtime branch 2 times, most recently from d677488 to 0110435 Compare February 6, 2024 00:24
@acdlite acdlite marked this pull request as ready for review February 6, 2024 00:24
@acdlite acdlite force-pushed the upgrade-tests-jsx-runtime branch 2 times, most recently from 5bec739 to 2ad22c4 Compare February 6, 2024 00:44
Copy link
Member

@rickhanlonii rickhanlonii left a comment

Choose a reason for hiding this comment

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

sick

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

Choose a reason for hiding this comment

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

to eliminate so

?

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

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

Choose a reason for hiding this comment

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

👍

});

// @gate !enableComponentStackLocations || !__DEV__
// @gate !enableComponentStackLocations
// @gate __DEV__
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mostly just because the diffs are nicer

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

@gnoff gnoff Feb 6, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@acdlite acdlite Feb 6, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.
@acdlite acdlite merged commit 952aa74 into facebook:main Feb 6, 2024
36 checks passed
hoxyq added a commit that referenced this pull request Feb 6, 2024
#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
 ```
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
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.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…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
 ```
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants