Bugfix: Legacy Mode + DevTools "force fallback" (#19164)
DevTools has a feature to force a Suspense boundary to show a fallback. This feature causes us to skip the first render pass (where we render the primary children) and go straight to rendering the fallback. There's a Legacy Mode-only codepath that failed to take this scenario into account, instead assuming that whenever a fallback is being rendered, it was preceded by an attempt to render the primary children. SuspenseList can also cause us to skip the first pass, but the relevant branch is Legacy Mode-only, and SuspenseList is not supported in Legacy Mode. Fixes a test that I had temporarily disabled when upstreaming the Lanes implementation in #19108.
Andrew Clark committed
Jun 19, 2020 at 11:06 UTC
6ba25b96df5d4179bf8aba3c3fe1ace3dce28234
4 files changed
+151
-69
packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap
+76
@@ -255,6 +255,82 @@ exports[`Store collapseNodesByDefault:false should support nested Suspense nodes
255
<Component key="Unrelated at End">
256
`;
257
258
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 8: first and third child are suspended 1`] = `
259
+[root]
260
+ ▾ <Wrapper>
261
+ <Component key="Outside">
262
+ ▾ <Suspense>
263
+ <Component key="Unrelated at Start">
264
+ ▾ <Suspense>
265
+ <Loading key="Suspense 1 Fallback">
266
+ ▾ <Suspense>
267
+ <Component key="Suspense 2 Content">
268
+ ▾ <Suspense>
269
+ <Loading key="Suspense 3 Fallback">
270
+ <Component key="Unrelated at End">
271
+`;
272
+
273
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 9: parent is suspended 1`] = `
274
+[root]
275
+ ▾ <Wrapper>
276
+ <Component key="Outside">
277
+ ▾ <Suspense>
278
+ <Loading key="Parent Fallback">
279
+`;
280
+
281
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 10: parent is suspended 1`] = `
282
+[root]
283
+ ▾ <Wrapper>
284
+ <Component key="Outside">
285
+ ▾ <Suspense>
286
+ <Loading key="Parent Fallback">
287
+`;
288
+
289
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 11: all children are suspended 1`] = `
290
+[root]
291
+ ▾ <Wrapper>
292
+ <Component key="Outside">
293
+ ▾ <Suspense>
294
+ <Component key="Unrelated at Start">
295
+ ▾ <Suspense>
296
+ <Loading key="Suspense 1 Fallback">
297
+ ▾ <Suspense>
298
+ <Loading key="Suspense 2 Fallback">
299
+ ▾ <Suspense>
300
+ <Loading key="Suspense 3 Fallback">
301
+ <Component key="Unrelated at End">
302
+`;
303
+
304
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 12: all children are suspended 1`] = `
305
+[root]
306
+ ▾ <Wrapper>
307
+ <Component key="Outside">
308
+ ▾ <Suspense>
309
+ <Component key="Unrelated at Start">
310
+ ▾ <Suspense>
311
+ <Loading key="Suspense 1 Fallback">
312
+ ▾ <Suspense>
313
+ <Loading key="Suspense 2 Fallback">
314
+ ▾ <Suspense>
315
+ <Loading key="Suspense 3 Fallback">
316
+ <Component key="Unrelated at End">
317
+`;
318
+
319
+exports[`Store collapseNodesByDefault:false should support nested Suspense nodes: 13: third child is suspended 1`] = `
320
+[root]
321
+ ▾ <Wrapper>
322
+ <Component key="Outside">
323
+ ▾ <Suspense>
324
+ <Component key="Unrelated at Start">
325
+ ▾ <Suspense>
326
+ <Component key="Suspense 1 Content">
327
+ ▾ <Suspense>
328
+ <Component key="Suspense 2 Content">
329
+ ▾ <Suspense>
330
+ <Loading key="Suspense 3 Fallback">
331
+ <Component key="Unrelated at End">
332
+`;
333
+
334
exports[`Store collapseNodesByDefault:false should support reordering of children: 1: mount 1`] = `
335
[root]
336
▾ <Root>
packages/react-devtools-shared/src/__tests__/store-test.js
+55
-67
@@ -285,73 +285,61 @@ describe('Store', () => {
285
);
286
expect(store).toMatchSnapshot('7: only third child is suspended');
287
288
- // FIXME: The rest of the test fails. This was introduced as part of
289
- // the Lanes refactor. I'm fairly certain it's related to the layout of
290
- // the Suspense fiber: we no longer conditionally wrap the primary
291
- // children. They are always wrapped in an extra fiber.
292
- //
293
- // This landed in the new fork without triggering the test run
294
- // because we don't run the DevTools tests against both forks. I only
295
- // discovered the failure once I upstreamed the changes.
296
- //
297
- // Since this has been running in www for weeks without major issues, I'll
298
- // defer fixing this to a follow up.
299
- //
300
- // const rendererID = getRendererID();
301
- // act(() =>
302
- // agent.overrideSuspense({
303
- // id: store.getElementIDAtIndex(4),
304
- // rendererID,
305
- // forceFallback: true,
306
- // }),
307
- // );
308
- // expect(store).toMatchSnapshot('8: first and third child are suspended');
309
- // act(() =>
310
- // agent.overrideSuspense({
311
- // id: store.getElementIDAtIndex(2),
312
- // rendererID,
313
- // forceFallback: true,
314
- // }),
315
- // );
316
- // expect(store).toMatchSnapshot('9: parent is suspended');
317
- // act(() =>
318
- // ReactDOM.render(
319
- // <Wrapper
320
- // suspendParent={false}
321
- // suspendFirst={true}
322
- // suspendSecond={true}
323
- // />,
324
- // container,
325
- // ),
326
- // );
327
- // expect(store).toMatchSnapshot('10: parent is suspended');
328
- // act(() =>
329
- // agent.overrideSuspense({
330
- // id: store.getElementIDAtIndex(2),
331
- // rendererID,
332
- // forceFallback: false,
333
- // }),
334
- // );
335
- // expect(store).toMatchSnapshot('11: all children are suspended');
336
- // act(() =>
337
- // agent.overrideSuspense({
338
- // id: store.getElementIDAtIndex(4),
339
- // rendererID,
340
- // forceFallback: false,
341
- // }),
342
- // );
343
- // expect(store).toMatchSnapshot('12: all children are suspended');
344
- // act(() =>
345
- // ReactDOM.render(
346
- // <Wrapper
347
- // suspendParent={false}
348
- // suspendFirst={false}
349
- // suspendSecond={false}
350
- // />,
351
- // container,
352
- // ),
353
- // );
354
- // expect(store).toMatchSnapshot('13: third child is suspended');
288
+ const rendererID = getRendererID();
289
+ act(() =>
290
+ agent.overrideSuspense({
291
+ id: store.getElementIDAtIndex(4),
292
+ rendererID,
293
+ forceFallback: true,
294
+ }),
295
+ );
296
+ expect(store).toMatchSnapshot('8: first and third child are suspended');
297
+ act(() =>
298
+ agent.overrideSuspense({
299
+ id: store.getElementIDAtIndex(2),
300
+ rendererID,
301
+ forceFallback: true,
302
+ }),
303
+ );
304
+ expect(store).toMatchSnapshot('9: parent is suspended');
305
+ act(() =>
306
+ ReactDOM.render(
307
+ <Wrapper
308
+ suspendParent={false}
309
+ suspendFirst={true}
310
+ suspendSecond={true}
311
+ />,
312
+ container,
313
+ ),
314
+ );
315
+ expect(store).toMatchSnapshot('10: parent is suspended');
316
+ act(() =>
317
+ agent.overrideSuspense({
318
+ id: store.getElementIDAtIndex(2),
319
+ rendererID,
320
+ forceFallback: false,
321
+ }),
322
+ );
323
+ expect(store).toMatchSnapshot('11: all children are suspended');
324
+ act(() =>
325
+ agent.overrideSuspense({
326
+ id: store.getElementIDAtIndex(4),
327
+ rendererID,
328
+ forceFallback: false,
329
+ }),
330
+ );
331
+ expect(store).toMatchSnapshot('12: all children are suspended');
332
+ act(() =>
333
+ ReactDOM.render(
334
+ <Wrapper
335
+ suspendParent={false}
336
+ suspendFirst={false}
337
+ suspendSecond={false}
338
+ />,
339
+ container,
340
+ ),
341
+ );
342
+ expect(store).toMatchSnapshot('13: third child is suspended');
343
});
344
345
it('should display a partially rendered SuspenseList', () => {
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+10
-1
@@ -2082,9 +2082,18 @@ function updateSuspenseFallbackChildren(
2082
};
2083
2084
let primaryChildFragment;
2085
- if ((mode & BlockingMode) === NoMode) {
2085
+ if (
2086
// In legacy mode, we commit the primary tree as if it successfully
2087
// completed, even though it's in an inconsistent state.
2088
+ (mode & BlockingMode) === NoMode &&
2089
+ // Make sure we're on the second pass, i.e. the primary child fragment was
2090
+ // already cloned. In legacy mode, the only case where this isn't true is
2091
+ // when DevTools forces us to display a fallback; we skip the first render
2092
+ // pass entirely and go straight to rendering the fallback. (In Concurrent
2093
+ // Mode, SuspenseList can also trigger this scenario, but this is a legacy-
2094
+ // only codepath.)
2095
+ workInProgress.child !== currentPrimaryChildFragment
2096
+ ) {
2097
const progressedPrimaryFragment: Fiber = (workInProgress.child: any);
2098
primaryChildFragment = progressedPrimaryFragment;
2099
primaryChildFragment.childLanes = NoLanes;
packages/react-reconciler/src/ReactFiberBeginWork.old.js
+10
-1
@@ -2082,9 +2082,18 @@ function updateSuspenseFallbackChildren(
2082
};
2083
2084
let primaryChildFragment;
2085
- if ((mode & BlockingMode) === NoMode) {
2085
+ if (
2086
// In legacy mode, we commit the primary tree as if it successfully
2087
// completed, even though it's in an inconsistent state.
2088
+ (mode & BlockingMode) === NoMode &&
2089
+ // Make sure we're on the second pass, i.e. the primary child fragment was
2090
+ // already cloned. In legacy mode, the only case where this isn't true is
2091
+ // when DevTools forces us to display a fallback; we skip the first render
2092
+ // pass entirely and go straight to rendering the fallback. (In Concurrent
2093
+ // Mode, SuspenseList can also trigger this scenario, but this is a legacy-
2094
+ // only codepath.)
2095
+ workInProgress.child !== currentPrimaryChildFragment
2096
+ ) {
2097
const progressedPrimaryFragment: Fiber = (workInProgress.child: any);
2098
primaryChildFragment = progressedPrimaryFragment;
2099
primaryChildFragment.childLanes = NoLanes;