[RTR] Add usage warning behind flag #27903

jackpope · 2024-01-08T20:51:34Z

Summary

Moving towards deprecation of ReactTestRenderer. Log a warning on each render so we can remove the exports in a future major version.

We can enable this flag in web RTR without disrupting RN tests by flipping the flag in packages/shared/forks/ReactFeatureFlags.test-renderer.js

How did you test this change?

yarn test packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js

react-sizebot · 2024-01-08T20:56:25Z

Comparing: 90a0ae1...113bf6f

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.87 kB	176.87 kB	=	55.14 kB	55.14 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	178.97 kB	178.97 kB	=	55.77 kB	55.77 kB
facebook-www/ReactDOM-prod.classic.js	=	591.94 kB	591.94 kB	=	104.47 kB	104.47 kB
facebook-www/ReactDOM-prod.modern.js	=	575.23 kB	575.23 kB	=	101.48 kB	101.48 kB
test_utils/ReactAllWarnings.js	Deleted	67.04 kB	0.00 kB	Deleted	16.43 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/cjs/react-jsx-runtime.react-server.production.js	=	5.25 kB	5.21 kB	=	2.25 kB	2.25 kB
oss-experimental/react/cjs/react-jsx-runtime.production.js	=	5.18 kB	5.14 kB	=	2.23 kB	2.23 kB
oss-experimental/react/cjs/react-jsx-runtime.profiling.js	=	5.18 kB	5.14 kB	=	2.23 kB	2.23 kB
test_utils/ReactAllWarnings.js	Deleted	67.04 kB	0.00 kB	Deleted	16.43 kB	0.00 kB

Generated by 🚫 dangerJS against 113bf6f

rickhanlonii · 2024-01-09T01:23:43Z

packages/react-test-renderer/shallow.js

+
+const emptyObject = {};
+
+export default class ReactShallowRendererWithWarning extends ReactShallowRenderer {


What do you think about hoisting the feature flag check to the module level so the warning class is only included in builds where the warning is on? GCC will strip that class in builds that don't use it, and this will prevent the additional stack frame in errors for React Native.

I'm simplifying this PR by dropping the changes to the shallow entrypoint. It looks like we don't have shallow configured in rollup anymore and we may want to consider dropping it altogether as its been a separate package re-exported here for a while. Will follow up on this

rickhanlonii · 2024-01-09T01:31:58Z

packages/react-test-renderer/shallow.js

+      if (enableReactTestRendererWarning === true) {
+        console.warn(
+          "React's Shallow Renderer export will be removed in a future release. " +
+            'Please use @testing-library/react instead.',


IMO we should provide a link here, that explains the deprecation better and we can keep the recommendations up to date on that page. Here's an example of where we've done this in the past: https://legacy.reactjs.org/warnings/legacy-factories.html

Updated with link to warning page

rickhanlonii · 2024-02-20T19:51:38Z

packages/react-test-renderer/src/ReactTestRenderer.js

+  if (__DEV__) {
+    if (
+      enableReactTestRendererWarning === true &&
+      global.__REACT_DEVTOOLS_GLOBAL_HOOK__ == null


Why are we checking this?

Dev tools rely on RTR a bunch within their testing. This avoid the warning there which breaks tests in CI. I can remove for now since this PR isn't turning on the flag yet.

rickhanlonii · 2024-02-20T19:52:12Z

packages/shared/ReactFeatureFlags.js

@@ -197,6 +197,9 @@ export const enableUnifiedSyncLane = true;
 // Adds an opt-in to time slicing for updates that aren't wrapped in startTransition.
 export const allowConcurrentByDefault = false;

+// Warn on any usage of ReactTestRenderer
+export const enableReactTestRendererWarning = false;


This can go in the Next Major section below.

rickhanlonii · 2024-02-22T18:50:30Z

packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js

@@ -51,6 +51,21 @@ function cleanNodeOrArray(node) {
 }

 describe('ReactTestRenderer', () => {
+  beforeEach(() => {
+    jest.resetModules();
+    ReactFeatureFlags.enableReactTestRendererWarning = false;


Can we use the @GATE pragma?

In the next PR we silence the warning to allow us to turn the flag on before all the React test cleanup is finished. So for now we don't necessarily expect a pass/fail on each side of the flag. Can probably update in the future or we'll see if we can remove the flag altogether

rickhanlonii

LGTM

## Summary Moving towards deprecation of ReactTestRenderer. Log a warning on each render so we can remove the exports in a future major version. We can enable this flag in web RTR without disrupting RN tests by flipping the flag in `packages/shared/forks/ReactFeatureFlags.test-renderer.js` ## How did you test this change? `yarn test packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js` DiffTrain build for [66c8346](66c8346)

## Summary Based on - #27903 This PR - Silence warning in React tests - Turn on flag We want to finish cleaning up internal RTR usage, but let's prioritize the deprecation process. We do this by silencing the internal warning for now. ## How did you test this change? `yarn build` `yarn test ReactHooksInspectionIntegration -b`

## Summary Moving towards deprecation of ReactTestRenderer. Log a warning on each render so we can remove the exports in a future major version. We can enable this flag in web RTR without disrupting RN tests by flipping the flag in `packages/shared/forks/ReactFeatureFlags.test-renderer.js` ## How did you test this change? `yarn test packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js`

## Summary Based on - facebook#27903 This PR - Silence warning in React tests - Turn on flag We want to finish cleaning up internal RTR usage, but let's prioritize the deprecation process. We do this by silencing the internal warning for now. ## How did you test this change? `yarn build` `yarn test ReactHooksInspectionIntegration -b`

## Summary Moving towards deprecation of ReactTestRenderer. Log a warning on each render so we can remove the exports in a future major version. We can enable this flag in web RTR without disrupting RN tests by flipping the flag in `packages/shared/forks/ReactFeatureFlags.test-renderer.js` ## How did you test this change? `yarn test packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js` DiffTrain build for commit 66c8346.

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

jackpope requested review from acdlite and kassens January 8, 2024 20:52

jackpope force-pushed the rtr-warning branch 2 times, most recently from e325338 to 05e79fb Compare January 8, 2024 22:27

rickhanlonii reviewed Jan 29, 2024

View reviewed changes

jackpope force-pushed the rtr-warning branch from afedfc5 to e24023b Compare February 16, 2024 19:59

rickhanlonii reviewed Feb 20, 2024

View reviewed changes

Jack Pope added 7 commits February 20, 2024 15:45

[RTR] Add usage warning behind flag

ed56bb0

Add test snapshot

3d3863d

Conditional shallow export, update warning msg

3765904

Move flag definition

866ea33

Update msg in test

1176aed

Skip warning msg for dev tools

6b5cc69

Remove dev tools noop for now

113bf6f

jackpope force-pushed the rtr-warning branch from 97d4ba2 to 113bf6f Compare February 20, 2024 20:50

jackpope mentioned this pull request Feb 22, 2024

[RTR] Enable warning flag #28419

Merged

rickhanlonii reviewed Feb 22, 2024

View reviewed changes

rickhanlonii approved these changes Feb 22, 2024

View reviewed changes

jackpope merged commit 66c8346 into facebook:main Feb 23, 2024
36 checks passed

jackpope deleted the rtr-warning branch February 23, 2024 16:33

huozhi mentioned this pull request Feb 26, 2024

Upgrade vendored react vercel/next.js#62549

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

[RTR] Add usage warning behind flag #27903

[RTR] Add usage warning behind flag #27903

jackpope commented Jan 8, 2024 •

edited

Loading

react-sizebot commented Jan 8, 2024 •

edited

Loading

rickhanlonii Jan 9, 2024

jackpope Feb 16, 2024

rickhanlonii Jan 9, 2024

jackpope Feb 16, 2024

rickhanlonii Feb 20, 2024

jackpope Feb 20, 2024

rickhanlonii Feb 20, 2024

rickhanlonii Feb 22, 2024

jackpope Feb 23, 2024

rickhanlonii left a comment


		const emptyObject = {};

		export default class ReactShallowRendererWithWarning extends ReactShallowRenderer {

[RTR] Add usage warning behind flag #27903

[RTR] Add usage warning behind flag #27903

Conversation

jackpope commented Jan 8, 2024 • edited Loading

Summary

How did you test this change?

react-sizebot commented Jan 8, 2024 • edited Loading

Critical size changes

Significant size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rickhanlonii left a comment

Choose a reason for hiding this comment

jackpope commented Jan 8, 2024 •

edited

Loading

react-sizebot commented Jan 8, 2024 •

edited

Loading