From 5bef02e7f43f070d273a5dacbb8263a22f4422f0 Mon Sep 17 00:00:00 2001 From: Tianyu Yao Date: Wed, 7 Feb 2024 15:32:20 -0800 Subject: [PATCH] Revert "Remove revertRemovalOfSiblingPrerendering killswitch (#26549)" This reverts commit 7b0642bb989ec659c6c9891ea16daa0420caab4d. --- .../src/ReactFiberWorkLoop.js | 50 +++++++++++++++---- packages/shared/ReactFeatureFlags.js | 5 ++ .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 6 +++ .../shared/forks/ReactFeatureFlags.www.js | 1 + 9 files changed, 57 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 597d0089941a2..706a48475e293 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -38,6 +38,7 @@ import { enableCache, enableTransitionTracing, useModernStrictMode, + revertRemovalOfSiblingPrerendering, disableLegacyContext, alwaysThrottleRetries, } from 'shared/ReactFeatureFlags'; @@ -2544,14 +2545,28 @@ function completeUnitOfWork(unitOfWork: Fiber): void { // sibling. If there are no more siblings, return to the parent fiber. let completedWork: Fiber = unitOfWork; do { - if (__DEV__) { + if (revertRemovalOfSiblingPrerendering) { if ((completedWork.flags & Incomplete) !== NoFlags) { - // NOTE: If we re-enable sibling prerendering in some cases, this branch - // is where we would switch to the unwinding path. - console.error( - 'Internal React error: Expected this fiber to be complete, but ' + - "it isn't. It should have been unwound. This is a bug in React.", - ); + // This fiber did not complete, because one of its children did not + // complete. Switch to unwinding the stack instead of completing it. + // + // The reason "unwind" and "complete" is interleaved is because when + // something suspends, we continue rendering the siblings even though + // they will be replaced by a fallback. + // TODO: Disable sibling prerendering, then remove this branch. + unwindUnitOfWork(completedWork); + return; + } + } else { + if (__DEV__) { + if ((completedWork.flags & Incomplete) !== NoFlags) { + // NOTE: If we re-enable sibling prerendering in some cases, this branch + // is where we would switch to the unwinding path. + console.error( + 'Internal React error: Expected this fiber to be complete, but ' + + "it isn't. It should have been unwound. This is a bug in React.", + ); + } } } @@ -2655,9 +2670,24 @@ function unwindUnitOfWork(unitOfWork: Fiber): void { returnFiber.deletions = null; } - // NOTE: If we re-enable sibling prerendering in some cases, here we - // would switch to the normal completion path: check if a sibling - // exists, and if so, begin work on it. + if (revertRemovalOfSiblingPrerendering) { + // If there are siblings, work on them now even though they're going to be + // replaced by a fallback. We're "prerendering" them. Historically our + // rationale for this behavior has been to initiate any lazy data requests + // in the siblings, and also to warm up the CPU cache. + // TODO: Don't prerender siblings. With `use`, we suspend the work loop + // until the data has resolved, anyway. + const siblingFiber = incompleteWork.sibling; + if (siblingFiber !== null) { + // This branch will return us to the normal work loop. + workInProgress = siblingFiber; + return; + } + } else { + // NOTE: If we re-enable sibling prerendering in some cases, this branch + // is where we would switch to the normal completion path: check if a + // sibling exists, and if so, begin work on it. + } // Otherwise, return to the parent // $FlowFixMe[incompatible-type] we bail out when we get a null diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 6cd8795052b4e..8fa4f91ba919b 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -22,6 +22,11 @@ export const enableComponentStackLocations = true; // when it rolls out to prod. We should remove these as soon as possible. // ----------------------------------------------------------------------------- +// This is phrased as a negative so that if someone forgets to add a GK, the +// default is to enable the feature. It should only be overridden if there's +// a regression in prod. +export const revertRemovalOfSiblingPrerendering = false; + // ----------------------------------------------------------------------------- // Land or remove (moderate effort) // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 2ff22ed4053cf..54db591d1f960 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -54,6 +54,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const disableLegacyContext = false; +export const revertRemovalOfSiblingPrerendering = false; export const enableTrustedTypesIntegration = false; export const disableTextareaChildren = false; export const enableSuspenseAvoidThisFallback = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 82a903537ca9f..3408d6ec6e9b6 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -36,6 +36,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const disableLegacyContext = false; +export const revertRemovalOfSiblingPrerendering = false; export const enableTrustedTypesIntegration = false; export const disableTextareaChildren = false; export const disableModulePatternComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index a09d5f1e2b156..5ef26220bcfd2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -36,6 +36,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const disableLegacyContext = false; +export const revertRemovalOfSiblingPrerendering = false; export const enableTrustedTypesIntegration = false; export const disableTextareaChildren = false; export const disableModulePatternComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index ac23fa99c85c4..aa643556477b9 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -36,6 +36,7 @@ export const enableScopeAPI = false; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = false; export const disableLegacyContext = false; +export const revertRemovalOfSiblingPrerendering = false; export const enableTrustedTypesIntegration = false; export const disableTextareaChildren = false; export const disableModulePatternComponents = false; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index c5e13f95e3d6a..0c41c040ba664 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -36,6 +36,7 @@ export const enableScopeAPI = true; export const enableCreateEventHandleAPI = false; export const enableSuspenseCallback = true; export const disableLegacyContext = false; +export const revertRemovalOfSiblingPrerendering = false; export const enableTrustedTypesIntegration = false; export const disableTextareaChildren = false; export const disableModulePatternComponents = true; diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 775dced6d1009..32a0b87da80b7 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -47,6 +47,12 @@ export const enableSchedulingProfiler = __VARIANT__; // so we don't need to use __VARIANT__ to get extra coverage. export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__; +// This flag only exists so it can be connected to a www GK that acts as a +// killswitch. We don't run our tests against the `true` value because 1) it +// affects too many tests 2) it shouldn't break anything. But it is mildly +// risky, hence this extra precaution. +export const revertRemovalOfSiblingPrerendering = false; + // TODO: These flags are hard-coded to the default values used in open source. // Update the tests so that they pass in either mode, then set these // to __VARIANT__. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 7ae2da95cc5d7..cf5b50e184754 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -18,6 +18,7 @@ export const { disableInputAttributeSyncing, disableIEWorkarounds, enableTrustedTypesIntegration, + revertRemovalOfSiblingPrerendering, replayFailedUnitOfWorkWithInvokeGuardedCallback, enableLegacyFBSupport, enableDebugTracing,