From 2e3e6a9b1cc97ec91248be74565e7ccbf6946067 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 23 May 2024 12:25:23 -0400 Subject: [PATCH] Unify ReactFiberCurrentOwner and ReactCurrentFiber (#29038) We previously had two slightly different concepts for "current fiber". There's the "owner" which is set inside of class components in prod if string refs are enabled, and sometimes inside function components in DEV but not other contexts. Then we have the "current fiber" which is only set in DEV for various warnings but is enabled in a bunch of contexts. This unifies them into a single "current fiber". The concept of string refs shouldn't really exist so this should really be a DEV only concept. In the meantime, this sets the current fiber inside class render only in prod, however, in DEV it's now enabled in more contexts which can affect the string refs. That was already the case that a string ref in a Function component was only connecting to the owner in prod. Any string ref associated with any non-class won't work regardless so that's not an issue. The practical change here is that an element with a string ref created inside a life-cycle associated with a class will work in DEV but not in prod. Since we need the current fiber to be available in more contexts in DEV for the debugging purposes. That wouldn't affect any old code since it would have a broken ref anyway. New code shouldn't use string refs anyway. The other implication is that "owner" doesn't necessarily mean "rendering" since we need the "owner" to track other debug information like stacks - in other contexts like useEffect, life cycles, etc. Internally we have a separate `isRendering` flag that actually means we're rendering but even that is a very overloaded concept. So anything that uses "owner" to imply rendering might be wrong with this change. This is a first step to a larger refactor for tracking current rendering information. --------- Co-authored-by: Sebastian Silbermann --- .../src/__tests__/treeContext-test.js | 12 +++---- packages/react-dom/src/__tests__/refs-test.js | 4 +-- .../react-dom/src/client/ReactDOMRootFB.js | 7 +++-- .../src/ReactNativePublicCompat.js | 7 +++-- .../react-reconciler/src/ReactCurrentFiber.js | 16 ++++++++-- .../src/ReactFiberAsyncDispatcher.js | 2 +- .../src/ReactFiberBeginWork.js | 7 ++--- .../src/ReactFiberCommitWork.js | 8 +++-- .../src/ReactFiberCurrentOwner.js | 16 ---------- .../src/ReactFiberReconciler.js | 4 +-- .../src/ReactFiberTreeReflection.js | 4 +-- .../src/ReactFiberWorkLoop.js | 31 ++++++------------- .../src/ReactStrictModeWarnings.js | 4 +-- 13 files changed, 56 insertions(+), 66 deletions(-) delete mode 100644 packages/react-reconciler/src/ReactFiberCurrentOwner.js diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index f22b08f4d9cc3..185e11461bfe4 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2586,14 +2586,14 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] ✕ `); selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] → ✕ `); @@ -2648,14 +2648,14 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] ✕ `); selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 2, ⚠ 0 + ✕ 1, ⚠ 0 [root] → ✕ `); @@ -2705,7 +2705,7 @@ describe('TreeListContext', () => { utils.act(() => TestRenderer.create()); expect(store).toMatchInlineSnapshot(` - ✕ 3, ⚠ 0 + ✕ 2, ⚠ 0 [root] ▾ ✕ ✕ @@ -2713,7 +2713,7 @@ describe('TreeListContext', () => { selectNextErrorOrWarning(); expect(state).toMatchInlineSnapshot(` - ✕ 3, ⚠ 0 + ✕ 2, ⚠ 0 [root] → ▾ ✕ ✕ diff --git a/packages/react-dom/src/__tests__/refs-test.js b/packages/react-dom/src/__tests__/refs-test.js index 9dcaed18c2374..a678f955e5fce 100644 --- a/packages/react-dom/src/__tests__/refs-test.js +++ b/packages/react-dom/src/__tests__/refs-test.js @@ -499,8 +499,8 @@ describe('creating element with string ref in constructor', () => { } } - // @gate !disableStringRefs - it('throws an error', async () => { + // @gate !disableStringRefs && !__DEV__ + it('throws an error in prod', async () => { await expect(async function () { const container = document.createElement('div'); const root = ReactDOMClient.createRoot(container); diff --git a/packages/react-dom/src/client/ReactDOMRootFB.js b/packages/react-dom/src/client/ReactDOMRootFB.js index 0aa2306665f4e..64a1cf12ac734 100644 --- a/packages/react-dom/src/client/ReactDOMRootFB.js +++ b/packages/react-dom/src/client/ReactDOMRootFB.js @@ -61,7 +61,10 @@ import {LegacyRoot} from 'react-reconciler/src/ReactRootTags'; import getComponentNameFromType from 'shared/getComponentNameFromType'; import {has as hasInstance} from 'shared/ReactInstanceMap'; -import {currentOwner} from 'react-reconciler/src/ReactFiberCurrentOwner'; +import { + current as currentOwner, + isRendering, +} from 'react-reconciler/src/ReactCurrentFiber'; import assign from 'shared/assign'; @@ -343,7 +346,7 @@ export function findDOMNode( ): null | Element | Text { if (__DEV__) { const owner = currentOwner; - if (owner !== null && owner.stateNode !== null) { + if (owner !== null && isRendering && owner.stateNode !== null) { const warnedAboutRefsInRender = owner.stateNode._warnedAboutRefsInRender; if (!warnedAboutRefsInRender) { console.error( diff --git a/packages/react-native-renderer/src/ReactNativePublicCompat.js b/packages/react-native-renderer/src/ReactNativePublicCompat.js index 081ea6f7eb571..7d6bb49741648 100644 --- a/packages/react-native-renderer/src/ReactNativePublicCompat.js +++ b/packages/react-native-renderer/src/ReactNativePublicCompat.js @@ -25,14 +25,17 @@ import { } from 'react-reconciler/src/ReactFiberReconciler'; import {doesFiberContain} from 'react-reconciler/src/ReactFiberTreeReflection'; import getComponentNameFromType from 'shared/getComponentNameFromType'; -import {currentOwner} from 'react-reconciler/src/ReactFiberCurrentOwner'; +import { + current as currentOwner, + isRendering, +} from 'react-reconciler/src/ReactCurrentFiber'; export function findHostInstance_DEPRECATED( componentOrHandle: ?(ElementRef | number), ): ?ElementRef> { if (__DEV__) { const owner = currentOwner; - if (owner !== null && owner.stateNode !== null) { + if (owner !== null && isRendering && owner.stateNode !== null) { if (!owner.stateNode._warnedAboutRefsInRender) { console.error( '%s is accessing findNodeHandle inside its render(). ' + diff --git a/packages/react-reconciler/src/ReactCurrentFiber.js b/packages/react-reconciler/src/ReactCurrentFiber.js index 5baa696b28b53..c0ff5d16df4cb 100644 --- a/packages/react-reconciler/src/ReactCurrentFiber.js +++ b/packages/react-reconciler/src/ReactCurrentFiber.js @@ -41,21 +41,33 @@ function getCurrentFiberStackInDev(): string { return ''; } +export function resetCurrentDebugFiberInDEV() { + if (__DEV__) { + resetCurrentFiber(); + } +} + +export function setCurrentDebugFiberInDEV(fiber: Fiber | null) { + if (__DEV__) { + setCurrentFiber(fiber); + } +} + export function resetCurrentFiber() { if (__DEV__) { ReactSharedInternals.getCurrentStack = null; - current = null; isRendering = false; } + current = null; } export function setCurrentFiber(fiber: Fiber | null) { if (__DEV__) { ReactSharedInternals.getCurrentStack = fiber === null ? null : getCurrentFiberStackInDev; - current = fiber; isRendering = false; } + current = fiber; } export function getCurrentFiber(): Fiber | null { diff --git a/packages/react-reconciler/src/ReactFiberAsyncDispatcher.js b/packages/react-reconciler/src/ReactFiberAsyncDispatcher.js index f1c251965512c..d329fb369cae3 100644 --- a/packages/react-reconciler/src/ReactFiberAsyncDispatcher.js +++ b/packages/react-reconciler/src/ReactFiberAsyncDispatcher.js @@ -16,7 +16,7 @@ import {CacheContext} from './ReactFiberCacheComponent'; import {disableStringRefs} from 'shared/ReactFeatureFlags'; -import {currentOwner} from './ReactFiberCurrentOwner'; +import {current as currentOwner} from './ReactCurrentFiber'; function getCacheForType(resourceType: () => T): T { if (!enableCache) { diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index b1d639583df7f..e638580bb0da4 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -125,6 +125,7 @@ import { import { getCurrentFiberOwnerNameInDevOrNull, setIsRendering, + setCurrentFiber, } from './ReactCurrentFiber'; import { resolveFunctionForHotReloading, @@ -296,7 +297,6 @@ import { pushRootMarkerInstance, TransitionTracingMarker, } from './ReactFiberTracingMarkerComponent'; -import {setCurrentOwner} from './ReactFiberCurrentOwner'; // A special exception that's used to unwind the stack when an update flows // into a dehydrated boundary. @@ -432,7 +432,6 @@ function updateForwardRef( markComponentRenderStarted(workInProgress); } if (__DEV__) { - setCurrentOwner(workInProgress); setIsRendering(true); nextChildren = renderWithHooks( current, @@ -1150,7 +1149,6 @@ function updateFunctionComponent( markComponentRenderStarted(workInProgress); } if (__DEV__) { - setCurrentOwner(workInProgress); setIsRendering(true); nextChildren = renderWithHooks( current, @@ -1373,7 +1371,7 @@ function finishClassComponent( // Rerender if (__DEV__ || !disableStringRefs) { - setCurrentOwner(workInProgress); + setCurrentFiber(workInProgress); } let nextChildren; if ( @@ -3419,7 +3417,6 @@ function updateContextConsumer( } let newChildren; if (__DEV__) { - setCurrentOwner(workInProgress); setIsRendering(true); newChildren = render(newValue); setIsRendering(false); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index f757b7ca81b15..5c7f845b96793 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -101,8 +101,8 @@ import { } from './ReactFiberFlags'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import { - resetCurrentFiber as resetCurrentDebugFiberInDEV, - setCurrentFiber as setCurrentDebugFiberInDEV, + resetCurrentDebugFiberInDEV, + setCurrentDebugFiberInDEV, getCurrentFiber as getCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; import {resolveClassComponentProps} from './ReactFiberClassComponent'; @@ -2486,7 +2486,7 @@ export function commitMutationEffects( setCurrentDebugFiberInDEV(finishedWork); commitMutationEffectsOnFiber(finishedWork, root, committedLanes); - setCurrentDebugFiberInDEV(finishedWork); + resetCurrentDebugFiberInDEV(); inProgressLanes = null; inProgressRoot = null; @@ -3125,8 +3125,10 @@ export function commitLayoutEffects( inProgressLanes = committedLanes; inProgressRoot = root; + setCurrentDebugFiberInDEV(finishedWork); const current = finishedWork.alternate; commitLayoutEffectOnFiber(root, current, finishedWork, committedLanes); + resetCurrentDebugFiberInDEV(); inProgressLanes = null; inProgressRoot = null; diff --git a/packages/react-reconciler/src/ReactFiberCurrentOwner.js b/packages/react-reconciler/src/ReactFiberCurrentOwner.js deleted file mode 100644 index 0c1f3d6e3130d..0000000000000 --- a/packages/react-reconciler/src/ReactFiberCurrentOwner.js +++ /dev/null @@ -1,16 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import type {Fiber} from './ReactInternalTypes'; - -export let currentOwner: Fiber | null = null; - -export function setCurrentOwner(fiber: null | Fiber) { - currentOwner = fiber; -} diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 641f229b6dea3..eb1bfbb65a6cf 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -78,8 +78,8 @@ import { import { isRendering as ReactCurrentFiberIsRendering, current as ReactCurrentFiberCurrent, - resetCurrentFiber as resetCurrentDebugFiberInDEV, - setCurrentFiber as setCurrentDebugFiberInDEV, + resetCurrentDebugFiberInDEV, + setCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; import {StrictLegacyMode} from './ReactTypeOfMode'; import { diff --git a/packages/react-reconciler/src/ReactFiberTreeReflection.js b/packages/react-reconciler/src/ReactFiberTreeReflection.js index 57640bd7f3b18..e0199108693e3 100644 --- a/packages/react-reconciler/src/ReactFiberTreeReflection.js +++ b/packages/react-reconciler/src/ReactFiberTreeReflection.js @@ -24,7 +24,7 @@ import { SuspenseComponent, } from './ReactWorkTags'; import {NoFlags, Placement, Hydrating} from './ReactFiberFlags'; -import {currentOwner} from './ReactFiberCurrentOwner'; +import {current as currentOwner, isRendering} from './ReactCurrentFiber'; export function getNearestMountedFiber(fiber: Fiber): null | Fiber { let node = fiber; @@ -90,7 +90,7 @@ export function isFiberMounted(fiber: Fiber): boolean { export function isMounted(component: React$Component): boolean { if (__DEV__) { const owner = currentOwner; - if (owner !== null && owner.tag === ClassComponent) { + if (owner !== null && isRendering && owner.tag === ClassComponent) { const ownerFiber: Fiber = owner; const instance = ownerFiber.stateNode; if (!instance._warnedAboutRefsInRender) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f47f079c8a3bb..dd04ae139028a 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -206,7 +206,6 @@ import { ContextOnlyDispatcher, } from './ReactFiberHooks'; import {DefaultAsyncDispatcher} from './ReactFiberAsyncDispatcher'; -import {setCurrentOwner} from './ReactFiberCurrentOwner'; import { createCapturedValueAtFiber, type CapturedValue, @@ -232,8 +231,9 @@ import ReactStrictModeWarnings from './ReactStrictModeWarnings'; import { isRendering as ReactCurrentDebugFiberIsRenderingInDEV, current as ReactCurrentFiberCurrent, - resetCurrentFiber as resetCurrentDebugFiberInDEV, - setCurrentFiber as setCurrentDebugFiberInDEV, + resetCurrentDebugFiberInDEV, + setCurrentDebugFiberInDEV, + resetCurrentFiber, } from './ReactCurrentFiber'; import { isDevToolsPresent, @@ -1686,9 +1686,8 @@ function handleThrow(root: FiberRoot, thrownValue: any): void { // These should be reset immediately because they're only supposed to be set // when React is executing user code. resetHooksAfterThrow(); - resetCurrentDebugFiberInDEV(); if (__DEV__ || !disableStringRefs) { - setCurrentOwner(null); + resetCurrentFiber(); } if (thrownValue === SuspenseException) { @@ -2386,7 +2385,9 @@ function performUnitOfWork(unitOfWork: Fiber): void { next = beginWork(current, unitOfWork, entangledRenderLanes); } - resetCurrentDebugFiberInDEV(); + if (__DEV__ || !disableStringRefs) { + resetCurrentFiber(); + } unitOfWork.memoizedProps = unitOfWork.pendingProps; if (next === null) { // If this doesn't spawn new work, complete the current work. @@ -2394,10 +2395,6 @@ function performUnitOfWork(unitOfWork: Fiber): void { } else { workInProgress = next; } - - if (__DEV__ || !disableStringRefs) { - setCurrentOwner(null); - } } function replaySuspendedUnitOfWork(unitOfWork: Fiber): void { @@ -2408,7 +2405,6 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void { setCurrentDebugFiberInDEV(unitOfWork); let next; - setCurrentDebugFiberInDEV(unitOfWork); const isProfilingMode = enableProfilerTimer && (unitOfWork.mode & ProfileMode) !== NoMode; if (isProfilingMode) { @@ -2501,7 +2497,9 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void { // The begin phase finished successfully without suspending. Return to the // normal work loop. - resetCurrentDebugFiberInDEV(); + if (__DEV__ || !disableStringRefs) { + resetCurrentFiber(); + } unitOfWork.memoizedProps = unitOfWork.pendingProps; if (next === null) { // If this doesn't spawn new work, complete the current work. @@ -2509,10 +2507,6 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void { } else { workInProgress = next; } - - if (__DEV__ || !disableStringRefs) { - setCurrentOwner(null); - } } function throwAndUnwindWorkLoop( @@ -2902,11 +2896,6 @@ function commitRootImpl( const prevExecutionContext = executionContext; executionContext |= CommitContext; - // Reset this to null before calling lifecycles - if (__DEV__ || !disableStringRefs) { - setCurrentOwner(null); - } - // The commit phase is broken into several sub-phases. We do a separate pass // of the effect list for each phase: all mutation effects come before all // layout effects, and so on. diff --git a/packages/react-reconciler/src/ReactStrictModeWarnings.js b/packages/react-reconciler/src/ReactStrictModeWarnings.js index 2f223a0f0b832..d14a38a9020be 100644 --- a/packages/react-reconciler/src/ReactStrictModeWarnings.js +++ b/packages/react-reconciler/src/ReactStrictModeWarnings.js @@ -10,8 +10,8 @@ import type {Fiber} from './ReactInternalTypes'; import { - resetCurrentFiber as resetCurrentDebugFiberInDEV, - setCurrentFiber as setCurrentDebugFiberInDEV, + resetCurrentDebugFiberInDEV, + setCurrentDebugFiberInDEV, } from './ReactCurrentFiber'; import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber'; import {StrictLegacyMode} from './ReactTypeOfMode';