@samitouri / QOS-React-2 / commits / f993ffc514

Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue (#24247)

* Fix infinite loop if unmemoized val passed to uDV The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value. * Temporarily disable "long nested update" warning DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.

Andrew Clark committed Apr 11, 2022 at 12:34 UTC f993ffc5141a58e2a53d4b822b15744b0542aa93
5 files changed +361 -68
packages/react-devtools-shared/src/__tests__/preprocessData-test.js
+9 -3
@@ -1463,7 +1463,9 @@ describe('Timeline profiler', () => {
1463 expect(event.warning).toBe(null);
1464 });
1465
1466 - it('should warn about long nested (state) updates during layout effects', async () => {
1466 + // This is temporarily disabled because the warning doesn't work
1467 + // with useDeferredValue
1468 + it.skip('should warn about long nested (state) updates during layout effects', async () => {
1469 function Component() {
1470 const [didMount, setDidMount] = React.useState(false);
1471 Scheduler.unstable_yieldValue(
@@ -1523,7 +1525,9 @@ describe('Timeline profiler', () => {
1525 );
1526 });
1527
1526 - it('should warn about long nested (forced) updates during layout effects', async () => {
1528 + // This is temporarily disabled because the warning doesn't work
1529 + // with useDeferredValue
1530 + it.skip('should warn about long nested (forced) updates during layout effects', async () => {
1531 class Component extends React.Component {
1532 _didMount: boolean = false;
1533 componentDidMount() {
@@ -1654,7 +1658,9 @@ describe('Timeline profiler', () => {
1658 });
1659 });
1660
1657 - it('should not warn about deferred value updates scheduled during commit phase', async () => {
1661 + // This is temporarily disabled because the warning doesn't work
1662 + // with useDeferredValue
1663 + it.skip('should not warn about deferred value updates scheduled during commit phase', async () => {
1664 function Component() {
1665 const [value, setValue] = React.useState(0);
1666 const deferredValue = React.useDeferredValue(value);
packages/react-devtools-timeline/src/import-worker/preprocessData.js
+4 -1
@@ -1124,7 +1124,10 @@ export default async function preprocessData(
1124 lane => profilerData.laneToLabelMap.get(lane) === 'Transition',
1125 )
1126 ) {
1127 - schedulingEvent.warning = WARNING_STRINGS.NESTED_UPDATE;
1127 + // FIXME: This warning doesn't account for "nested updates" that are
1128 + // spawned by useDeferredValue. Disabling temporarily until we figure
1129 + // out the right way to handle this.
1130 + // schedulingEvent.warning = WARNING_STRINGS.NESTED_UPDATE;
1131 }
1132 }
1133 });
packages/react-reconciler/src/ReactFiberHooks.new.js
+62 -32
@@ -47,6 +47,8 @@ import {
47 NoLanes,
48 isSubsetOfLanes,
49 includesBlockingLane,
50 + includesOnlyNonUrgentLanes,
51 + claimNextTransitionLane,
52 mergeLanes,
53 removeLanes,
54 intersectLanes,
@@ -1929,45 +1931,73 @@ function updateMemo<T>(
1931 }
1932
1933 function mountDeferredValue<T>(value: T): T {
1932 - const [prevValue, setValue] = mountState(value);
1933 - mountEffect(() => {
1934 - const prevTransition = ReactCurrentBatchConfig.transition;
1935 - ReactCurrentBatchConfig.transition = {};
1936 - try {
1937 - setValue(value);
1938 - } finally {
1939 - ReactCurrentBatchConfig.transition = prevTransition;
1940 - }
1941 - }, [value]);
1942 - return prevValue;
1934 + const hook = mountWorkInProgressHook();
1935 + hook.memoizedState = value;
1936 + return value;
1937 }
1938
1939 function updateDeferredValue<T>(value: T): T {
1946 - const [prevValue, setValue] = updateState(value);
1947 - updateEffect(() => {
1948 - const prevTransition = ReactCurrentBatchConfig.transition;
1949 - ReactCurrentBatchConfig.transition = {};
1950 - try {
1951 - setValue(value);
1952 - } finally {
1953 - ReactCurrentBatchConfig.transition = prevTransition;
1954 - }
1955 - }, [value]);
1956 - return prevValue;
1940 + const hook = updateWorkInProgressHook();
1941 + const resolvedCurrentHook: Hook = (currentHook: any);
1942 + const prevValue: T = resolvedCurrentHook.memoizedState;
1943 + return updateDeferredValueImpl(hook, prevValue, value);
1944 }
1945
1946 function rerenderDeferredValue<T>(value: T): T {
1960 - const [prevValue, setValue] = rerenderState(value);
1961 - updateEffect(() => {
1962 - const prevTransition = ReactCurrentBatchConfig.transition;
1963 - ReactCurrentBatchConfig.transition = {};
1964 - try {
1965 - setValue(value);
1966 - } finally {
1967 - ReactCurrentBatchConfig.transition = prevTransition;
1947 + const hook = updateWorkInProgressHook();
1948 + if (currentHook === null) {
1949 + // This is a rerender during a mount.
1950 + hook.memoizedState = value;
1951 + return value;
1952 + } else {
1953 + // This is a rerender during an update.
1954 + const prevValue: T = currentHook.memoizedState;
1955 + return updateDeferredValueImpl(hook, prevValue, value);
1956 + }
1957 +}
1958 +
1959 +function updateDeferredValueImpl<T>(hook: Hook, prevValue: T, value: T): T {
1960 + const shouldDeferValue = !includesOnlyNonUrgentLanes(renderLanes);
1961 + if (shouldDeferValue) {
1962 + // This is an urgent update. If the value has changed, keep using the
1963 + // previous value and spawn a deferred render to update it later.
1964 +
1965 + if (!is(value, prevValue)) {
1966 + // Schedule a deferred render
1967 + const deferredLane = claimNextTransitionLane();
1968 + currentlyRenderingFiber.lanes = mergeLanes(
1969 + currentlyRenderingFiber.lanes,
1970 + deferredLane,
1971 + );
1972 + markSkippedUpdateLanes(deferredLane);
1973 +
1974 + // Set this to true to indicate that the rendered value is inconsistent
1975 + // from the latest value. The name "baseState" doesn't really match how we
1976 + // use it because we're reusing a state hook field instead of creating a
1977 + // new one.
1978 + hook.baseState = true;
1979 }
1969 - }, [value]);
1970 - return prevValue;
1980 +
1981 + // Reuse the previous value
1982 + return prevValue;
1983 + } else {
1984 + // This is not an urgent update, so we can use the latest value regardless
1985 + // of what it is. No need to defer it.
1986 +
1987 + // However, if we're currently inside a spawned render, then we need to mark
1988 + // this as an update to prevent the fiber from bailing out.
1989 + //
1990 + // `baseState` is true when the current value is different from the rendered
1991 + // value. The name doesn't really match how we use it because we're reusing
1992 + // a state hook field instead of creating a new one.
1993 + if (hook.baseState) {
1994 + // Flip this back to false.
1995 + hook.baseState = false;
1996 + markWorkInProgressReceivedUpdate();
1997 + }
1998 +
1999 + return value;
2000 + }
2001 }
2002
2003 function startTransition(setPending, callback, options) {
packages/react-reconciler/src/ReactFiberHooks.old.js
+62 -32
@@ -47,6 +47,8 @@ import {
47 NoLanes,
48 isSubsetOfLanes,
49 includesBlockingLane,
50 + includesOnlyNonUrgentLanes,
51 + claimNextTransitionLane,
52 mergeLanes,
53 removeLanes,
54 intersectLanes,
@@ -1929,45 +1931,73 @@ function updateMemo<T>(
1931 }
1932
1933 function mountDeferredValue<T>(value: T): T {
1932 - const [prevValue, setValue] = mountState(value);
1933 - mountEffect(() => {
1934 - const prevTransition = ReactCurrentBatchConfig.transition;
1935 - ReactCurrentBatchConfig.transition = {};
1936 - try {
1937 - setValue(value);
1938 - } finally {
1939 - ReactCurrentBatchConfig.transition = prevTransition;
1940 - }
1941 - }, [value]);
1942 - return prevValue;
1934 + const hook = mountWorkInProgressHook();
1935 + hook.memoizedState = value;
1936 + return value;
1937 }
1938
1939 function updateDeferredValue<T>(value: T): T {
1946 - const [prevValue, setValue] = updateState(value);
1947 - updateEffect(() => {
1948 - const prevTransition = ReactCurrentBatchConfig.transition;
1949 - ReactCurrentBatchConfig.transition = {};
1950 - try {
1951 - setValue(value);
1952 - } finally {
1953 - ReactCurrentBatchConfig.transition = prevTransition;
1954 - }
1955 - }, [value]);
1956 - return prevValue;
1940 + const hook = updateWorkInProgressHook();
1941 + const resolvedCurrentHook: Hook = (currentHook: any);
1942 + const prevValue: T = resolvedCurrentHook.memoizedState;
1943 + return updateDeferredValueImpl(hook, prevValue, value);
1944 }
1945
1946 function rerenderDeferredValue<T>(value: T): T {
1960 - const [prevValue, setValue] = rerenderState(value);
1961 - updateEffect(() => {
1962 - const prevTransition = ReactCurrentBatchConfig.transition;
1963 - ReactCurrentBatchConfig.transition = {};
1964 - try {
1965 - setValue(value);
1966 - } finally {
1967 - ReactCurrentBatchConfig.transition = prevTransition;
1947 + const hook = updateWorkInProgressHook();
1948 + if (currentHook === null) {
1949 + // This is a rerender during a mount.
1950 + hook.memoizedState = value;
1951 + return value;
1952 + } else {
1953 + // This is a rerender during an update.
1954 + const prevValue: T = currentHook.memoizedState;
1955 + return updateDeferredValueImpl(hook, prevValue, value);
1956 + }
1957 +}
1958 +
1959 +function updateDeferredValueImpl<T>(hook: Hook, prevValue: T, value: T): T {
1960 + const shouldDeferValue = !includesOnlyNonUrgentLanes(renderLanes);
1961 + if (shouldDeferValue) {
1962 + // This is an urgent update. If the value has changed, keep using the
1963 + // previous value and spawn a deferred render to update it later.
1964 +
1965 + if (!is(value, prevValue)) {
1966 + // Schedule a deferred render
1967 + const deferredLane = claimNextTransitionLane();
1968 + currentlyRenderingFiber.lanes = mergeLanes(
1969 + currentlyRenderingFiber.lanes,
1970 + deferredLane,
1971 + );
1972 + markSkippedUpdateLanes(deferredLane);
1973 +
1974 + // Set this to true to indicate that the rendered value is inconsistent
1975 + // from the latest value. The name "baseState" doesn't really match how we
1976 + // use it because we're reusing a state hook field instead of creating a
1977 + // new one.
1978 + hook.baseState = true;
1979 }
1969 - }, [value]);
1970 - return prevValue;
1980 +
1981 + // Reuse the previous value
1982 + return prevValue;
1983 + } else {
1984 + // This is not an urgent update, so we can use the latest value regardless
1985 + // of what it is. No need to defer it.
1986 +
1987 + // However, if we're currently inside a spawned render, then we need to mark
1988 + // this as an update to prevent the fiber from bailing out.
1989 + //
1990 + // `baseState` is true when the current value is different from the rendered
1991 + // value. The name doesn't really match how we use it because we're reusing
1992 + // a state hook field instead of creating a new one.
1993 + if (hook.baseState) {
1994 + // Flip this back to false.
1995 + hook.baseState = false;
1996 + markWorkInProgressReceivedUpdate();
1997 + }
1998 +
1999 + return value;
2000 + }
2001 }
2002
2003 function startTransition(setPending, callback, options) {
packages/react-reconciler/src/__tests__/ReactDeferredValue-test.js new
+224
@@ -0,0 +1,224 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + */
7 +
8 +'use strict';
9 +
10 +let React;
11 +let ReactNoop;
12 +let Scheduler;
13 +let act;
14 +let startTransition;
15 +let useDeferredValue;
16 +let useMemo;
17 +let useState;
18 +
19 +describe('ReactDeferredValue', () => {
20 + beforeEach(() => {
21 + jest.resetModules();
22 +
23 + React = require('react');
24 + ReactNoop = require('react-noop-renderer');
25 + Scheduler = require('scheduler');
26 + act = require('jest-react').act;
27 + startTransition = React.startTransition;
28 + useDeferredValue = React.useDeferredValue;
29 + useMemo = React.useMemo;
30 + useState = React.useState;
31 + });
32 +
33 + function Text({text}) {
34 + Scheduler.unstable_yieldValue(text);
35 + return text;
36 + }
37 +
38 + it('does not cause an infinite defer loop if the original value isn\t memoized', async () => {
39 + function App({value}) {
40 + // The object passed to useDeferredValue is never the same as the previous
41 + // render. A naive implementation would endlessly spawn deferred renders.
42 + const {value: deferredValue} = useDeferredValue({value});
43 +
44 + const child = useMemo(() => <Text text={'Original: ' + value} />, [
45 + value,
46 + ]);
47 +
48 + const deferredChild = useMemo(
49 + () => <Text text={'Deferred: ' + deferredValue} />,
50 + [deferredValue],
51 + );
52 +
53 + return (
54 + <div>
55 + <div>{child}</div>
56 + <div>{deferredChild}</div>
57 + </div>
58 + );
59 + }
60 +
61 + const root = ReactNoop.createRoot();
62 +
63 + // Initial render
64 + await act(async () => {
65 + root.render(<App value={1} />);
66 + });
67 + expect(Scheduler).toHaveYielded(['Original: 1', 'Deferred: 1']);
68 +
69 + // If it's an urgent update, the value is deferred
70 + await act(async () => {
71 + root.render(<App value={2} />);
72 +
73 + expect(Scheduler).toFlushUntilNextPaint(['Original: 2']);
74 + // The deferred value updates in a separate render
75 + expect(Scheduler).toFlushUntilNextPaint(['Deferred: 2']);
76 + });
77 + expect(root).toMatchRenderedOutput(
78 + <div>
79 + <div>Original: 2</div>
80 + <div>Deferred: 2</div>
81 + </div>,
82 + );
83 +
84 + // But if it updates during a transition, it doesn't defer
85 + await act(async () => {
86 + startTransition(() => {
87 + root.render(<App value={3} />);
88 + });
89 + // The deferred value updates in the same render as the original
90 + expect(Scheduler).toFlushUntilNextPaint(['Original: 3', 'Deferred: 3']);
91 + });
92 + expect(root).toMatchRenderedOutput(
93 + <div>
94 + <div>Original: 3</div>
95 + <div>Deferred: 3</div>
96 + </div>,
97 + );
98 + });
99 +
100 + it('does not defer during a transition', async () => {
101 + function App({value}) {
102 + const deferredValue = useDeferredValue(value);
103 +
104 + const child = useMemo(() => <Text text={'Original: ' + value} />, [
105 + value,
106 + ]);
107 +
108 + const deferredChild = useMemo(
109 + () => <Text text={'Deferred: ' + deferredValue} />,
110 + [deferredValue],
111 + );
112 +
113 + return (
114 + <div>
115 + <div>{child}</div>
116 + <div>{deferredChild}</div>
117 + </div>
118 + );
119 + }
120 +
121 + const root = ReactNoop.createRoot();
122 +
123 + // Initial render
124 + await act(async () => {
125 + root.render(<App value={1} />);
126 + });
127 + expect(Scheduler).toHaveYielded(['Original: 1', 'Deferred: 1']);
128 +
129 + // If it's an urgent update, the value is deferred
130 + await act(async () => {
131 + root.render(<App value={2} />);
132 +
133 + expect(Scheduler).toFlushUntilNextPaint(['Original: 2']);
134 + // The deferred value updates in a separate render
135 + expect(Scheduler).toFlushUntilNextPaint(['Deferred: 2']);
136 + });
137 + expect(root).toMatchRenderedOutput(
138 + <div>
139 + <div>Original: 2</div>
140 + <div>Deferred: 2</div>
141 + </div>,
142 + );
143 +
144 + // But if it updates during a transition, it doesn't defer
145 + await act(async () => {
146 + startTransition(() => {
147 + root.render(<App value={3} />);
148 + });
149 + // The deferred value updates in the same render as the original
150 + expect(Scheduler).toFlushUntilNextPaint(['Original: 3', 'Deferred: 3']);
151 + });
152 + expect(root).toMatchRenderedOutput(
153 + <div>
154 + <div>Original: 3</div>
155 + <div>Deferred: 3</div>
156 + </div>,
157 + );
158 + });
159 +
160 + it("works if there's a render phase update", async () => {
161 + function App({value: propValue}) {
162 + const [value, setValue] = useState(null);
163 + if (value !== propValue) {
164 + setValue(propValue);
165 + }
166 +
167 + const deferredValue = useDeferredValue(value);
168 +
169 + const child = useMemo(() => <Text text={'Original: ' + value} />, [
170 + value,
171 + ]);
172 +
173 + const deferredChild = useMemo(
174 + () => <Text text={'Deferred: ' + deferredValue} />,
175 + [deferredValue],
176 + );
177 +
178 + return (
179 + <div>
180 + <div>{child}</div>
181 + <div>{deferredChild}</div>
182 + </div>
183 + );
184 + }
185 +
186 + const root = ReactNoop.createRoot();
187 +
188 + // Initial render
189 + await act(async () => {
190 + root.render(<App value={1} />);
191 + });
192 + expect(Scheduler).toHaveYielded(['Original: 1', 'Deferred: 1']);
193 +
194 + // If it's an urgent update, the value is deferred
195 + await act(async () => {
196 + root.render(<App value={2} />);
197 +
198 + expect(Scheduler).toFlushUntilNextPaint(['Original: 2']);
199 + // The deferred value updates in a separate render
200 + expect(Scheduler).toFlushUntilNextPaint(['Deferred: 2']);
201 + });
202 + expect(root).toMatchRenderedOutput(
203 + <div>
204 + <div>Original: 2</div>
205 + <div>Deferred: 2</div>
206 + </div>,
207 + );
208 +
209 + // But if it updates during a transition, it doesn't defer
210 + await act(async () => {
211 + startTransition(() => {
212 + root.render(<App value={3} />);
213 + });
214 + // The deferred value updates in the same render as the original
215 + expect(Scheduler).toFlushUntilNextPaint(['Original: 3', 'Deferred: 3']);
216 + });
217 + expect(root).toMatchRenderedOutput(
218 + <div>
219 + <div>Original: 3</div>
220 + <div>Deferred: 3</div>
221 + </div>,
222 + );
223 + });
224 +});