@samitouri / QOS-React-2 / commits / 05726d72cc

[Fix] Errors should not "unsuspend" a transition (#22423)

If an error is thrown during a transition where we would have otherwise suspended without showing a fallback (i.e. during a refresh), we should still suspend. The current behavior is that the error will force the fallback to appear, even if it's completely unrelated to the component that errored, which breaks the contract of `startTransition`.

Andrew Clark committed Sep 27, 2021 at 11:44 UTC 05726d72ccd3940a927462d520d0d2674012ad85
3 files changed +407 -4
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+3 -2
@@ -1445,7 +1445,8 @@ export function renderDidSuspend(): void {
1445 export function renderDidSuspendDelayIfPossible(): void {
1446 if (
1447 workInProgressRootExitStatus === RootIncomplete ||
1448 - workInProgressRootExitStatus === RootSuspended
1448 + workInProgressRootExitStatus === RootSuspended ||
1449 + workInProgressRootExitStatus === RootErrored
1450 ) {
1451 workInProgressRootExitStatus = RootSuspendedWithDelay;
1452 }
@@ -1469,7 +1470,7 @@ export function renderDidSuspendDelayIfPossible(): void {
1470 }
1471
1472 export function renderDidError() {
1472 - if (workInProgressRootExitStatus !== RootCompleted) {
1473 + if (workInProgressRootExitStatus !== RootSuspendedWithDelay) {
1474 workInProgressRootExitStatus = RootErrored;
1475 }
1476 }
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+3 -2
@@ -1445,7 +1445,8 @@ export function renderDidSuspend(): void {
1445 export function renderDidSuspendDelayIfPossible(): void {
1446 if (
1447 workInProgressRootExitStatus === RootIncomplete ||
1448 - workInProgressRootExitStatus === RootSuspended
1448 + workInProgressRootExitStatus === RootSuspended ||
1449 + workInProgressRootExitStatus === RootErrored
1450 ) {
1451 workInProgressRootExitStatus = RootSuspendedWithDelay;
1452 }
@@ -1469,7 +1470,7 @@ export function renderDidSuspendDelayIfPossible(): void {
1470 }
1471
1472 export function renderDidError() {
1472 - if (workInProgressRootExitStatus !== RootCompleted) {
1473 + if (workInProgressRootExitStatus !== RootSuspendedWithDelay) {
1474 workInProgressRootExitStatus = RootErrored;
1475 }
1476 }
packages/react-reconciler/src/__tests__/ReactConcurrentErrorRecovery-test.js new
+401
@@ -0,0 +1,401 @@
1 +let React;
2 +let ReactNoop;
3 +let Scheduler;
4 +let act;
5 +let Suspense;
6 +let getCacheForType;
7 +let startTransition;
8 +
9 +let caches;
10 +let seededCache;
11 +
12 +describe('ReactConcurrentErrorRecovery', () => {
13 + beforeEach(() => {
14 + jest.resetModules();
15 +
16 + React = require('react');
17 + ReactNoop = require('react-noop-renderer');
18 + Scheduler = require('scheduler');
19 + act = require('jest-react').act;
20 + Suspense = React.Suspense;
21 + startTransition = React.startTransition;
22 +
23 + getCacheForType = React.unstable_getCacheForType;
24 +
25 + caches = [];
26 + seededCache = null;
27 + });
28 +
29 + function createTextCache() {
30 + if (seededCache !== null) {
31 + // Trick to seed a cache before it exists.
32 + // TODO: Need a built-in API to seed data before the initial render (i.e.
33 + // not a refresh because nothing has mounted yet).
34 + const cache = seededCache;
35 + seededCache = null;
36 + return cache;
37 + }
38 +
39 + const data = new Map();
40 + const version = caches.length + 1;
41 + const cache = {
42 + version,
43 + data,
44 + resolve(text) {
45 + const record = data.get(text);
46 + if (record === undefined) {
47 + const newRecord = {
48 + status: 'resolved',
49 + value: text,
50 + };
51 + data.set(text, newRecord);
52 + } else if (record.status === 'pending') {
53 + const thenable = record.value;
54 + record.status = 'resolved';
55 + record.value = text;
56 + thenable.pings.forEach(t => t());
57 + }
58 + },
59 + reject(text, error) {
60 + const record = data.get(text);
61 + if (record === undefined) {
62 + const newRecord = {
63 + status: 'rejected',
64 + value: error,
65 + };
66 + data.set(text, newRecord);
67 + } else if (record.status === 'pending') {
68 + const thenable = record.value;
69 + record.status = 'rejected';
70 + record.value = error;
71 + thenable.pings.forEach(t => t());
72 + }
73 + },
74 + };
75 + caches.push(cache);
76 + return cache;
77 + }
78 +
79 + function readText(text) {
80 + const textCache = getCacheForType(createTextCache);
81 + const record = textCache.data.get(text);
82 + if (record !== undefined) {
83 + switch (record.status) {
84 + case 'pending':
85 + Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
86 + throw record.value;
87 + case 'rejected':
88 + Scheduler.unstable_yieldValue(`Error! [${text}]`);
89 + throw record.value;
90 + case 'resolved':
91 + return textCache.version;
92 + }
93 + } else {
94 + Scheduler.unstable_yieldValue(`Suspend! [${text}]`);
95 +
96 + const thenable = {
97 + pings: [],
98 + then(resolve) {
99 + if (newRecord.status === 'pending') {
100 + thenable.pings.push(resolve);
101 + } else {
102 + Promise.resolve().then(() => resolve(newRecord.value));
103 + }
104 + },
105 + };
106 +
107 + const newRecord = {
108 + status: 'pending',
109 + value: thenable,
110 + };
111 + textCache.data.set(text, newRecord);
112 +
113 + throw thenable;
114 + }
115 + }
116 +
117 + function Text({text}) {
118 + Scheduler.unstable_yieldValue(text);
119 + return text;
120 + }
121 +
122 + function AsyncText({text, showVersion}) {
123 + const version = readText(text);
124 + const fullText = showVersion ? `${text} [v${version}]` : text;
125 + Scheduler.unstable_yieldValue(fullText);
126 + return fullText;
127 + }
128 +
129 + function seedNextTextCache(text) {
130 + if (seededCache === null) {
131 + seededCache = createTextCache();
132 + }
133 + seededCache.resolve(text);
134 + }
135 +
136 + function resolveMostRecentTextCache(text) {
137 + if (caches.length === 0) {
138 + throw Error('Cache does not exist.');
139 + } else {
140 + // Resolve the most recently created cache. An older cache can by
141 + // resolved with `caches[index].resolve(text)`.
142 + caches[caches.length - 1].resolve(text);
143 + }
144 + }
145 +
146 + const resolveText = resolveMostRecentTextCache;
147 +
148 + function rejectMostRecentTextCache(text, error) {
149 + if (caches.length === 0) {
150 + throw Error('Cache does not exist.');
151 + } else {
152 + // Resolve the most recently created cache. An older cache can by
153 + // resolved with `caches[index].reject(text, error)`.
154 + caches[caches.length - 1].reject(text, error);
155 + }
156 + }
157 +
158 + const rejectText = rejectMostRecentTextCache;
159 +
160 + // @gate enableCache
161 + test('errors during a refresh transition should not force fallbacks to display (suspend then error)', async () => {
162 + class ErrorBoundary extends React.Component {
163 + state = {error: null};
164 + static getDerivedStateFromError(error) {
165 + return {error};
166 + }
167 + render() {
168 + if (this.state.error !== null) {
169 + return <Text text={this.state.error.message} />;
170 + }
171 + return this.props.children;
172 + }
173 + }
174 +
175 + function App({step}) {
176 + return (
177 + <>
178 + <Suspense fallback={<Text text="Loading..." />}>
179 + <ErrorBoundary>
180 + <AsyncText text={'A' + step} />
181 + </ErrorBoundary>
182 + </Suspense>
183 + <Suspense fallback={<Text text="Loading..." />}>
184 + <ErrorBoundary>
185 + <AsyncText text={'B' + step} />
186 + </ErrorBoundary>
187 + </Suspense>
188 + </>
189 + );
190 + }
191 +
192 + // Initial render
193 + const root = ReactNoop.createRoot();
194 + seedNextTextCache('A1');
195 + seedNextTextCache('B1');
196 + await act(async () => {
197 + root.render(<App step={1} />);
198 + });
199 + expect(Scheduler).toHaveYielded(['A1', 'B1']);
200 + expect(root).toMatchRenderedOutput('A1B1');
201 +
202 + // Start a refresh transition
203 + await act(async () => {
204 + startTransition(() => {
205 + root.render(<App step={2} />);
206 + });
207 + });
208 + expect(Scheduler).toHaveYielded([
209 + 'Suspend! [A2]',
210 + 'Loading...',
211 + 'Suspend! [B2]',
212 + 'Loading...',
213 + ]);
214 + // Because this is a refresh, we don't switch to a fallback
215 + expect(root).toMatchRenderedOutput('A1B1');
216 +
217 + // B fails to load.
218 + await act(async () => {
219 + rejectText('B2', new Error('Oops!'));
220 + });
221 +
222 + // Because we're still suspended on A, we can't show an error boundary. We
223 + // should wait for A to resolve.
224 + if (gate(flags => flags.replayFailedUnitOfWorkWithInvokeGuardedCallback)) {
225 + expect(Scheduler).toHaveYielded([
226 + 'Suspend! [A2]',
227 + 'Loading...',
228 +
229 + 'Error! [B2]',
230 + // This extra log happens when we replay the error
231 + // in invokeGuardedCallback
232 + 'Error! [B2]',
233 + 'Oops!',
234 + ]);
235 + } else {
236 + expect(Scheduler).toHaveYielded([
237 + 'Suspend! [A2]',
238 + 'Loading...',
239 + 'Error! [B2]',
240 + 'Oops!',
241 + ]);
242 + }
243 + // Remain on previous screen.
244 + expect(root).toMatchRenderedOutput('A1B1');
245 +
246 + // A finishes loading.
247 + await act(async () => {
248 + resolveText('A2');
249 + });
250 + if (gate(flags => flags.replayFailedUnitOfWorkWithInvokeGuardedCallback)) {
251 + expect(Scheduler).toHaveYielded([
252 + 'A2',
253 + 'Error! [B2]',
254 + // This extra log happens when we replay the error
255 + // in invokeGuardedCallback
256 + 'Error! [B2]',
257 + 'Oops!',
258 +
259 + 'A2',
260 + 'Error! [B2]',
261 + // This extra log happens when we replay the error
262 + // in invokeGuardedCallback
263 + 'Error! [B2]',
264 + 'Oops!',
265 + ]);
266 + } else {
267 + expect(Scheduler).toHaveYielded([
268 + 'A2',
269 + 'Error! [B2]',
270 + 'Oops!',
271 +
272 + 'A2',
273 + 'Error! [B2]',
274 + 'Oops!',
275 + ]);
276 + }
277 + // Now we can show the error boundary that's wrapped around B.
278 + expect(root).toMatchRenderedOutput('A2Oops!');
279 + });
280 +
281 + // @gate enableCache
282 + test('errors during a refresh transition should not force fallbacks to display (error then suspend)', async () => {
283 + class ErrorBoundary extends React.Component {
284 + state = {error: null};
285 + static getDerivedStateFromError(error) {
286 + return {error};
287 + }
288 + render() {
289 + if (this.state.error !== null) {
290 + return <Text text={this.state.error.message} />;
291 + }
292 + return this.props.children;
293 + }
294 + }
295 +
296 + function App({step}) {
297 + return (
298 + <>
299 + <Suspense fallback={<Text text="Loading..." />}>
300 + <ErrorBoundary>
301 + <AsyncText text={'A' + step} />
302 + </ErrorBoundary>
303 + </Suspense>
304 + <Suspense fallback={<Text text="Loading..." />}>
305 + <ErrorBoundary>
306 + <AsyncText text={'B' + step} />
307 + </ErrorBoundary>
308 + </Suspense>
309 + </>
310 + );
311 + }
312 +
313 + // Initial render
314 + const root = ReactNoop.createRoot();
315 + seedNextTextCache('A1');
316 + seedNextTextCache('B1');
317 + await act(async () => {
318 + root.render(<App step={1} />);
319 + });
320 + expect(Scheduler).toHaveYielded(['A1', 'B1']);
321 + expect(root).toMatchRenderedOutput('A1B1');
322 +
323 + // Start a refresh transition
324 + await act(async () => {
325 + startTransition(() => {
326 + root.render(<App step={2} />);
327 + });
328 + });
329 + expect(Scheduler).toHaveYielded([
330 + 'Suspend! [A2]',
331 + 'Loading...',
332 + 'Suspend! [B2]',
333 + 'Loading...',
334 + ]);
335 + // Because this is a refresh, we don't switch to a fallback
336 + expect(root).toMatchRenderedOutput('A1B1');
337 +
338 + // A fails to load.
339 + await act(async () => {
340 + rejectText('A2', new Error('Oops!'));
341 + });
342 +
343 + // Because we're still suspended on B, we can't show an error boundary. We
344 + // should wait for B to resolve.
345 + if (gate(flags => flags.replayFailedUnitOfWorkWithInvokeGuardedCallback)) {
346 + expect(Scheduler).toHaveYielded([
347 + 'Error! [A2]',
348 + // This extra log happens when we replay the error
349 + // in invokeGuardedCallback
350 + 'Error! [A2]',
351 + 'Oops!',
352 +
353 + 'Suspend! [B2]',
354 + 'Loading...',
355 + ]);
356 + } else {
357 + expect(Scheduler).toHaveYielded([
358 + 'Error! [A2]',
359 + 'Oops!',
360 + 'Suspend! [B2]',
361 + 'Loading...',
362 + ]);
363 + }
364 + // Remain on previous screen.
365 + expect(root).toMatchRenderedOutput('A1B1');
366 +
367 + // B finishes loading.
368 + await act(async () => {
369 + resolveText('B2');
370 + });
371 + if (gate(flags => flags.replayFailedUnitOfWorkWithInvokeGuardedCallback)) {
372 + expect(Scheduler).toHaveYielded([
373 + 'Error! [A2]',
374 + // This extra log happens when we replay the error
375 + // in invokeGuardedCallback
376 + 'Error! [A2]',
377 + 'Oops!',
378 + 'B2',
379 +
380 + 'Error! [A2]',
381 + // This extra log happens when we replay the error
382 + // in invokeGuardedCallback
383 + 'Error! [A2]',
384 + 'Oops!',
385 + 'B2',
386 + ]);
387 + } else {
388 + expect(Scheduler).toHaveYielded([
389 + 'Error! [A2]',
390 + 'Oops!',
391 + 'B2',
392 +
393 + 'Error! [A2]',
394 + 'Oops!',
395 + 'B2',
396 + ]);
397 + }
398 + // Now we can show the error boundary that's wrapped around B.
399 + expect(root).toMatchRenderedOutput('Oops!B2');
400 + });
401 +});