Skip to content

Commit

Permalink
Revert "Fix: Move destroy field to shared instance object (#26561)"
Browse files Browse the repository at this point in the history 
This reverts commit 85bb7b6.
  • Loading branch information
@kassens
kassens committed Apr 17, 2023
1 parent 481793a commit d41e5c2
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 125 deletions.
17 changes: 5 additions & 12 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -592,10 +592,9 @@ function commitHookEffectListUnmount(
do {
if ((effect.tag & flags) === flags) {
// Unmount
const inst = effect.inst;
const destroy = inst.destroy;
const destroy = effect.destroy;
effect.destroy = undefined;
if (destroy !== undefined) {
inst.destroy = undefined;
if (enableSchedulingProfiler) {
if ((flags & HookPassive) !== NoHookEffect) {
markComponentPassiveEffectUnmountStarted(finishedWork);
Expand Down Expand Up @@ -654,9 +653,7 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
setIsRunningInsertionEffect(true);
}
}
const inst = effect.inst;
const destroy = create();
inst.destroy = destroy;
effect.destroy = create();
if (__DEV__) {
if ((flags & HookInsertion) !== NoHookEffect) {
setIsRunningInsertionEffect(false);
Expand All @@ -672,6 +669,7 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
}

if (__DEV__) {
const destroy = effect.destroy;
if (destroy !== undefined && typeof destroy !== 'function') {
let hookName;
if ((effect.tag & HookLayout) !== NoFlags) {
Expand Down Expand Up @@ -2190,12 +2188,9 @@ function commitDeletionEffectsOnFiber(

let effect = firstEffect;
do {
const tag = effect.tag;
const inst = effect.inst;
const destroy = inst.destroy;
const {destroy, tag} = effect;
if (destroy !== undefined) {
if ((tag & HookInsertion) !== NoHookEffect) {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand All @@ -2208,15 +2203,13 @@ function commitDeletionEffectsOnFiber(

if (shouldProfile(deletedFiber)) {
startLayoutEffectTimer();
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
destroy,
);
recordLayoutEffectDuration(deletedFiber);
} else {
inst.destroy = undefined;
safelyCallDestroy(
deletedFiber,
nearestMountedAncestor,
Expand Down
48 changes: 13 additions & 35 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -180,29 +180,11 @@ export type Hook = {
next: Hook | null,
};

// The effect "instance" is a shared object that remains the same for the entire
// lifetime of an effect. In Rust terms, a RefCell. We use it to store the
// "destroy" function that is returned from an effect, because that is stateful.
// The field is `undefined` if the effect is unmounted, or if the effect ran
// but is not stateful. We don't explicitly track whether the effect is mounted
// or unmounted because that can be inferred by the hiddenness of the fiber in
// the tree, i.e. whether there is a hidden Offscreen fiber above it.
//
// It's unfortunate that this is stored on a separate object, because it adds
// more memory per effect instance, but it's conceptually sound. I think there's
// likely a better data structure we could use for effects; perhaps just one
// array of effect instances per fiber. But I think this is OK for now despite
// the additional memory and we can follow up with performance
// optimizations later.
type EffectInstance = {
destroy: void | (() => void),
};

export type Effect = {
tag: HookFlags,
create: () => (() => void) | void,
inst: EffectInstance,
deps: Array<mixed> | null,
destroy: (() => void) | void,
deps: Array<mixed> | void | null,
next: Effect,
};

Expand Down Expand Up @@ -1680,7 +1662,7 @@ function mountSyncExternalStore<T>(
pushEffect(
HookHasEffect | HookPassive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
createEffectInstance(),
undefined,
null,
);

Expand Down Expand Up @@ -1737,7 +1719,7 @@ function updateSyncExternalStore<T>(
pushEffect(
HookHasEffect | HookPassive,
updateStoreInstance.bind(null, fiber, inst, nextSnapshot, getSnapshot),
createEffectInstance(),
undefined,
null,
);

Expand Down Expand Up @@ -1878,13 +1860,13 @@ function rerenderState<S>(
function pushEffect(
tag: HookFlags,
create: () => (() => void) | void,
inst: EffectInstance,
deps: Array<mixed> | null,
destroy: (() => void) | void,
deps: Array<mixed> | void | null,
): Effect {
const effect: Effect = {
tag,
create,
inst,
destroy,
deps,
// Circular
next: (null: any),
Expand All @@ -1909,10 +1891,6 @@ function pushEffect(
return effect;
}

function createEffectInstance(): EffectInstance {
return {destroy: undefined};
}

let stackContainsErrorMessage: boolean | null = null;

function getCallerStackFrame(): string {
Expand Down Expand Up @@ -2016,7 +1994,7 @@ function mountEffectImpl(
hook.memoizedState = pushEffect(
HookHasEffect | hookFlags,
create,
createEffectInstance(),
undefined,
nextDeps,
);
}
Expand All @@ -2029,16 +2007,16 @@ function updateEffectImpl(
): void {
const hook = updateWorkInProgressHook();
const nextDeps = deps === undefined ? null : deps;
const effect: Effect = hook.memoizedState;
const inst = effect.inst;
let destroy = undefined;

// currentHook is null when rerendering after a render phase state update.
if (currentHook !== null) {
const prevEffect = currentHook.memoizedState;
destroy = prevEffect.destroy;
if (nextDeps !== null) {
const prevEffect: Effect = currentHook.memoizedState;
const prevDeps = prevEffect.deps;
if (areHookInputsEqual(nextDeps, prevDeps)) {
hook.memoizedState = pushEffect(hookFlags, create, inst, nextDeps);
hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
return;
}
}
Expand All @@ -2049,7 +2027,7 @@ function updateEffectImpl(
hook.memoizedState = pushEffect(
HookHasEffect | hookFlags,
create,
inst,
destroy,
nextDeps,
);
}
Expand Down
78 changes: 0 additions & 78 deletions packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -496,82 +496,4 @@ describe('ReactSuspenseEffectsSemanticsDOM', () => {
ReactDOM.render(null, container);
assertLog(['Unmount']);
});

it('does not call cleanup effects twice after a bailout', async () => {
const never = new Promise(resolve => {});
function Never() {
throw never;
}

let setSuspended;
let setLetter;

function App() {
const [suspended, _setSuspended] = React.useState(false);
setSuspended = _setSuspended;
const [letter, _setLetter] = React.useState('A');
setLetter = _setLetter;

return (
<React.Suspense fallback="Loading...">
<Child letter={letter} />
{suspended && <Never />}
</React.Suspense>
);
}

let nextId = 0;
const freed = new Set();
let setStep;

function Child({letter}) {
const [, _setStep] = React.useState(0);
setStep = _setStep;

React.useLayoutEffect(() => {
const localId = nextId++;
Scheduler.log('Did mount: ' + letter + localId);
return () => {
if (freed.has(localId)) {
throw Error('Double free: ' + letter + localId);
}
freed.add(localId);
Scheduler.log('Will unmount: ' + letter + localId);
};
}, [letter]);
}

const root = ReactDOMClient.createRoot(container);
await act(() => {
root.render(<App />);
});
assertLog(['Did mount: A0']);

await act(() => {
setStep(1);
setSuspended(false);
});
assertLog([]);

await act(() => {
setStep(1);
});
assertLog([]);

await act(() => {
setSuspended(true);
});
assertLog(['Will unmount: A0']);

await act(() => {
setSuspended(false);
setLetter('B');
});
assertLog(['Did mount: B1']);

await act(() => {
root.unmount();
});
assertLog(['Will unmount: B1']);
});
});

0 comments on commit d41e5c2

Please sign in to comment.