From 5d25752fc61f690a85bcbb0f4a6358a4b1cd4a8e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 Feb 2021 16:06:51 -0600 Subject: [PATCH 1/3] Warn if static flag is accidentally cleared "Static" fiber flags are flags that are meant to exist for the lifetime of a component. It's really important not to accidentally reset these, because we use them to decide whether or not to perform some operation on a tree (which we can do because they get bubbled via `subtreeFlags)`. We've had several bugs that were caused by this mistake, so we actually don't rely on static flags anywhere, yet. But we'd like to. So let's roll out this warning and see if it fires anywhere. Once we can confirm that there are no warnings, we can assume that it's safe to start using static flags. I did not wrap it behind a feature flag, because it's dev-only, and we can use our internal warning filter to hide this from the console. --- .../react-reconciler/src/ReactFiberHooks.new.js | 15 +++++++++++++++ .../react-reconciler/src/ReactFiberHooks.old.js | 15 +++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index cc5a8b00563b2..32144cc1b0505 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -471,6 +471,21 @@ export function renderWithHooks( currentHookNameInDev = null; hookTypesDev = null; hookTypesUpdateIndexDev = -1; + + // Confirm that a static flag was not added or removed since the last + // render. If this fires, it suggests that we incorrectly reset the static + // flags in some other part of the codebase. This has happened before, for + // example, in the SuspenseList implementation. + if ( + current !== null && + (current.flags & PassiveStaticEffect) !== + (workInProgress.flags & PassiveStaticEffect) + ) { + console.error( + 'Internal React error: Expected static flag was missing. Please ' + + 'notify the React team.', + ); + } } didScheduleRenderPhaseUpdate = false; diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index b5eefde63f715..ce0ac36636562 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -471,6 +471,21 @@ export function renderWithHooks( currentHookNameInDev = null; hookTypesDev = null; hookTypesUpdateIndexDev = -1; + + // Confirm that a static flag was not added or removed since the last + // render. If this fires, it suggests that we incorrectly reset the static + // flags in some other part of the codebase. This has happened before, for + // example, in the SuspenseList implementation. + if ( + current !== null && + (current.flags & PassiveStaticEffect) !== + (workInProgress.flags & PassiveStaticEffect) + ) { + console.error( + 'Internal React error: Expected static flag was missing. Please ' + + 'notify the React team.', + ); + } } didScheduleRenderPhaseUpdate = false; From 6e1e3df31792730f526bbd0da0abdab697c4786c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 Feb 2021 16:10:58 -0600 Subject: [PATCH 2/3] Intentionally clear static flag to test warning --- packages/react-reconciler/src/ReactFiber.new.js | 7 ++++++- packages/react-reconciler/src/ReactFiber.old.js | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index 983855ced5685..de4aaf6c0fd0e 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -289,7 +289,12 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // Reset all effects except static ones. // Static effects are not specific to a render. - workInProgress.flags = current.flags & StaticMask; + // FIXME: I intentionally broke this to confirm that it triggers a warning + // in dev. We should not reset the whole `flags` bitmask; we should preserve + // the static flags. + // workInProgress.flags = current.flags & StaticMask; + workInProgress.flags = NoFlags; + workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index ed2ae45f4ec17..8448b6877fb1a 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -289,7 +289,12 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // Reset all effects except static ones. // Static effects are not specific to a render. - workInProgress.flags = current.flags & StaticMask; + // FIXME: I intentionally broke this to confirm that it triggers a warning + // in dev. We should not reset the whole `flags` bitmask; we should preserve + // the static flags. + // workInProgress.flags = current.flags & StaticMask; + workInProgress.flags = NoFlags; + workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes; From 99dc7a93e55a63ddfdd478441f32b01f91193e52 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 11 Feb 2021 16:11:13 -0600 Subject: [PATCH 3/3] ...and fix it again --- packages/react-reconciler/src/ReactFiber.new.js | 7 +------ packages/react-reconciler/src/ReactFiber.old.js | 7 +------ 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.new.js b/packages/react-reconciler/src/ReactFiber.new.js index de4aaf6c0fd0e..983855ced5685 100644 --- a/packages/react-reconciler/src/ReactFiber.new.js +++ b/packages/react-reconciler/src/ReactFiber.new.js @@ -289,12 +289,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // Reset all effects except static ones. // Static effects are not specific to a render. - // FIXME: I intentionally broke this to confirm that it triggers a warning - // in dev. We should not reset the whole `flags` bitmask; we should preserve - // the static flags. - // workInProgress.flags = current.flags & StaticMask; - workInProgress.flags = NoFlags; - + workInProgress.flags = current.flags & StaticMask; workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes; diff --git a/packages/react-reconciler/src/ReactFiber.old.js b/packages/react-reconciler/src/ReactFiber.old.js index 8448b6877fb1a..ed2ae45f4ec17 100644 --- a/packages/react-reconciler/src/ReactFiber.old.js +++ b/packages/react-reconciler/src/ReactFiber.old.js @@ -289,12 +289,7 @@ export function createWorkInProgress(current: Fiber, pendingProps: any): Fiber { // Reset all effects except static ones. // Static effects are not specific to a render. - // FIXME: I intentionally broke this to confirm that it triggers a warning - // in dev. We should not reset the whole `flags` bitmask; we should preserve - // the static flags. - // workInProgress.flags = current.flags & StaticMask; - workInProgress.flags = NoFlags; - + workInProgress.flags = current.flags & StaticMask; workInProgress.childLanes = current.childLanes; workInProgress.lanes = current.lanes;