Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unify ReactFiberCurrentOwner and ReactCurrentFiber #29038

Merged
merged 2 commits into from
May 23, 2024
Merged

Unify ReactFiberCurrentOwner and ReactCurrentFiber #29038

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c1533e5
Unify ReactFiberCurrentOwner and ReactCurrentFiber
sebmarkbage May 9, 2024
599e50f
Update packages/react-reconciler/src/ReactFiberAsyncDispatcher.js
sebmarkbage May 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 6 additions & 6 deletions packages/react-devtools-shared/src/__tests__/treeContext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2586,14 +2586,14 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to log an react-test-renderer is deprecated error which shouldn't be counted towards the rendered errors. It was previously incorrectly associated to the previous tree because the "current fiber" wasn't reset after the commit phase of the previous render.

That's why the snapshots below have one less error counted.


expect(store).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
<ErrorBoundary> ✕
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
→ <ErrorBoundary> ✕
`);
Expand Down Expand Up @@ -2648,14 +2648,14 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));

expect(store).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
<ErrorBoundary> ✕
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 2, ⚠ 0
✕ 1, ⚠ 0
[root]
→ <ErrorBoundary> ✕
`);
Expand Down Expand Up @@ -2705,15 +2705,15 @@ describe('TreeListContext', () => {
utils.act(() => TestRenderer.create(<Contexts />));

expect(store).toMatchInlineSnapshot(`
✕ 3, ⚠ 0
✕ 2, ⚠ 0
[root]
▾ <ErrorBoundary> ✕
<Child> ✕
`);

selectNextErrorOrWarning();
expect(state).toMatchInlineSnapshot(`
✕ 3, ⚠ 0
✕ 2, ⚠ 0
[root]
→ ▾ <ErrorBoundary> ✕
<Child> ✕
Expand Down
4 changes: 2 additions & 2 deletions packages/react-dom/src/__tests__/refs-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it do in dev now?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use the same fields for dev and prod just to temporarily support string refs but in DEV it's active for the entire begin / complete phases for debugging purposes. Meaning that it just works in dev because the owner is set up but in prod it's not.

It still has a console.error in DEV so you shouldn't add any new callers as long as the warning actually shows up.

await expect(async function () {
const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
Expand Down
7 changes: 5 additions & 2 deletions packages/react-dom/src/client/ReactDOMRootFB.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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(
Expand Down
7 changes: 5 additions & 2 deletions packages/react-native-renderer/src/ReactNativePublicCompat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<TElementType: ElementType>(
componentOrHandle: ?(ElementRef<TElementType> | number),
): ?ElementRef<HostComponent<mixed>> {
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(). ' +
Expand Down
16 changes: 14 additions & 2 deletions packages/react-reconciler/src/ReactCurrentFiber.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberAsyncDispatcher.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {CacheContext} from './ReactFiberCacheComponent';

import {disableStringRefs} from 'shared/ReactFeatureFlags';

import {currentOwner} from './ReactFiberCurrentOwner';
import {current as currentOwner} from 'react-reconciler/src/ReactCurrentFiber';
sebmarkbage marked this conversation as resolved.
Show resolved Hide resolved

function getCacheForType<T>(resourceType: () => T): T {
if (!enableCache) {
Expand Down
7 changes: 2 additions & 5 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ import {
import {
getCurrentFiberOwnerNameInDevOrNull,
setIsRendering,
setCurrentFiber,
} from './ReactCurrentFiber';
import {
resolveFunctionForHotReloading,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -432,7 +432,6 @@ function updateForwardRef(
markComponentRenderStarted(workInProgress);
}
if (__DEV__) {
setCurrentOwner(workInProgress);
setIsRendering(true);
nextChildren = renderWithHooks(
current,
Expand Down Expand Up @@ -1150,7 +1149,6 @@ function updateFunctionComponent(
markComponentRenderStarted(workInProgress);
}
if (__DEV__) {
setCurrentOwner(workInProgress);
setIsRendering(true);
nextChildren = renderWithHooks(
current,
Expand Down Expand Up @@ -1373,7 +1371,7 @@ function finishClassComponent(

// Rerender
if (__DEV__ || !disableStringRefs) {
setCurrentOwner(workInProgress);
setCurrentFiber(workInProgress);
}
let nextChildren;
if (
Expand Down Expand Up @@ -3419,7 +3417,6 @@ function updateContextConsumer(
}
let newChildren;
if (__DEV__) {
setCurrentOwner(workInProgress);
setIsRendering(true);
newChildren = render(newValue);
setIsRendering(false);
Expand Down
8 changes: 5 additions & 3 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -2486,7 +2486,7 @@ export function commitMutationEffects(

setCurrentDebugFiberInDEV(finishedWork);
commitMutationEffectsOnFiber(finishedWork, root, committedLanes);
setCurrentDebugFiberInDEV(finishedWork);
resetCurrentDebugFiberInDEV();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was an bug before and the layout effects just reset to previous so it left the last mutation fiber as the debug fiber after commit.


inProgressLanes = null;
inProgressRoot = null;
Expand Down Expand Up @@ -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;
Expand Down
16 changes: 0 additions & 16 deletions packages/react-reconciler/src/ReactFiberCurrentOwner.js
View file
Open in desktop

This file was deleted.

4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactFiberTreeReflection.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -90,7 +90,7 @@ export function isFiberMounted(fiber: Fiber): boolean {
export function isMounted(component: React$Component<any, any>): 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) {
Expand Down
31 changes: 10 additions & 21 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,6 @@ import {
ContextOnlyDispatcher,
} from './ReactFiberHooks';
import {DefaultAsyncDispatcher} from './ReactFiberAsyncDispatcher';
import {setCurrentOwner} from './ReactFiberCurrentOwner';
import {
createCapturedValueAtFiber,
type CapturedValue,
Expand All @@ -230,8 +229,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,
Expand Down Expand Up @@ -1683,9 +1683,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) {
Expand Down Expand Up @@ -2377,18 +2376,16 @@ 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.
completeUnitOfWork(unitOfWork);
} else {
workInProgress = next;
}

if (__DEV__ || !disableStringRefs) {
setCurrentOwner(null);
}
}

function replaySuspendedUnitOfWork(unitOfWork: Fiber): void {
Expand All @@ -2399,7 +2396,6 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void {
setCurrentDebugFiberInDEV(unitOfWork);

let next;
setCurrentDebugFiberInDEV(unitOfWork);
const isProfilingMode =
enableProfilerTimer && (unitOfWork.mode & ProfileMode) !== NoMode;
if (isProfilingMode) {
Expand Down Expand Up @@ -2492,18 +2488,16 @@ 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.
completeUnitOfWork(unitOfWork);
} else {
workInProgress = next;
}

if (__DEV__ || !disableStringRefs) {
setCurrentOwner(null);
}
}

function throwAndUnwindWorkLoop(
Expand Down Expand Up @@ -2893,11 +2887,6 @@ function commitRootImpl(
const prevExecutionContext = executionContext;
executionContext |= CommitContext;

// Reset this to null before calling lifecycles
if (__DEV__ || !disableStringRefs) {
setCurrentOwner(null);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We weren't really initializing this to anything and we were always cleaning it up so I don't think this has been necessary for a while. However, since the life cycles also set the current debug fiber we get the resetting for free regardless - at least in DEV.


// 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.
Expand Down
4 changes: 2 additions & 2 deletions packages/react-reconciler/src/ReactStrictModeWarnings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down