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

ReactDOM: Remove every test-util except act() #28541

Merged
merged 3 commits into from
Mar 27, 2024

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Mar 11, 2024

Summary

Removes every API from react-dom/test-utils except act in favor of using @testing-library/react. Removal is gated behind disableDOMTestUtils that's off for Meta builds.

The functions aren't removed but instead throw with a helpful error message. We're doing the same for ReactDOM.render.

Also reverts #28446 to restore test coverage.

Some APIs like renderIntoDocument are technically gated behind disableDOMTestUtils and disableLegacyRoot. We could just combine both flags to prevent drift.

How did you test this change?

  • yarn test ReactTestUtils-test

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

react-sizebot commented Mar 11, 2024

Comparing: 4cc0f8e...82e3562

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.92 kB 176.92 kB = 54.99 kB 54.99 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 173.10 kB 173.10 kB = 53.94 kB 53.94 kB
facebook-www/ReactDOM-prod.classic.js = 592.31 kB 592.31 kB = 103.83 kB 103.83 kB
facebook-www/ReactDOM-prod.modern.js = 573.20 kB 573.20 kB = 100.71 kB 100.71 kB
oss-experimental/react-dom/umd/react-dom-test-utils.production.min.js = 12.03 kB 8.73 kB = 4.70 kB 2.75 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.production.min.js = 11.94 kB 8.59 kB = 4.66 kB 2.67 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.production.js = 42.64 kB 26.34 kB = 12.10 kB 7.16 kB
oss-experimental/react-dom/umd/react-dom-test-utils.development.js = 47.41 kB 28.06 kB = 13.09 kB 7.34 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.development.js = 44.63 kB 26.38 kB = 12.87 kB 7.19 kB
test_utils/ReactAllWarnings.js Deleted 65.07 kB 0.00 kB Deleted 16.27 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
oss-experimental/react-dom/umd/react-dom-test-utils.production.min.js = 12.03 kB 8.73 kB = 4.70 kB 2.75 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.production.min.js = 11.94 kB 8.59 kB = 4.66 kB 2.67 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.production.js = 42.64 kB 26.34 kB = 12.10 kB 7.16 kB
oss-experimental/react-dom/umd/react-dom-test-utils.development.js = 47.41 kB 28.06 kB = 13.09 kB 7.34 kB
oss-experimental/react-dom/cjs/react-dom-test-utils.development.js = 44.63 kB 26.38 kB = 12.87 kB 7.19 kB
test_utils/ReactAllWarnings.js Deleted 65.07 kB 0.00 kB Deleted 16.27 kB 0.00 kB

Generated by 🚫 dangerJS against 82e3562

@eps1lon eps1lon force-pushed the dom/remove-test-utils branch from 5c74ee7 to 5fc850d Compare March 11, 2024 20:14
@jackpope
Copy link
Contributor

jackpope commented Mar 12, 2024

I looked into vendoring ReactTestUtils in WWW to avoiding needing additional gating here but it has too much internal usage for it to be stable.

Can we apply either gating on the utils functions themselves or a build fork of the whole module? And additionally can we keep the ReactTestUtils-test with gating for www? Let me know if you want help with the additional work here.

Ideally within a couple months we can fully remove.

@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 13, 2024

Can we apply either gating on the utils functions themselves or a build fork of the whole module?

We use special .fb entrypoints on other packages so I hope I can leverage these for test-utils and then just gate the tests behind www.

@eps1lon eps1lon force-pushed the dom/remove-test-utils branch 2 times, most recently from e36702e to f5c3394 Compare March 14, 2024 14:00
@eps1lon
Copy link
Collaborator Author

eps1lon commented Mar 14, 2024

Didn't go with the special .fb entrypoint since that implies forking for modern, classic, experimental and stable. I used a simple feature flag instead like we do for ReactDOM.render

@eps1lon eps1lon marked this pull request as ready for review March 14, 2024 14:01
@eps1lon eps1lon requested review from gnoff and jackpope March 14, 2024 14:01
@eps1lon eps1lon force-pushed the dom/remove-test-utils branch 3 times, most recently from 65cf02e to ce7a436 Compare March 23, 2024 21:23
if (disableDOMTestUtils) {
throw new Error(
'`renderIntoDocument` was removed from `react-dom/test-utils`. ' +
'See https://react.dev/warnings/react-test-renderer for more info.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO: Write docs

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we generalize this warning page to something like /warnings/testing and include sections for each separate warning between ReactTestUtils and react-test-renderer?

Copy link
Member

Choose a reason for hiding this comment

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

I'd probably have a page for test-renderer and test-utils

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if (disableDOMTestUtils) {
throw new Error(
'`renderIntoDocument` was removed from `react-dom/test-utils`. ' +
'See https://react.dev/warnings/react-test-renderer for more info.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we generalize this warning page to something like /warnings/testing and include sections for each separate warning between ReactTestUtils and react-test-renderer?

@@ -102,6 +102,7 @@ export const disableStringRefs = false;

export const enableReactTestRendererWarning = false;
export const disableLegacyMode = false;
export const disableDOMTestUtils = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this false for now so this is easier to land.

@eps1lon eps1lon force-pushed the dom/remove-test-utils branch from ce7a436 to 6f3b264 Compare March 27, 2024 11:53
@eps1lon eps1lon force-pushed the dom/remove-test-utils branch from c75f7d6 to 82e3562 Compare March 27, 2024 14:05
@eps1lon eps1lon merged commit ec4d26c into facebook:main Mar 27, 2024
38 checks passed
@eps1lon eps1lon deleted the dom/remove-test-utils branch March 27, 2024 15:12
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
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 React 19
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants