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

Move commit passive unmount/mount to CommitWork #19599

Merged
merged 1 commit into from
Aug 13, 2020
Merged

Move commit passive unmount/mount to CommitWork #19599

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 159 additions & 4 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,14 @@ import type {FiberRoot} from './ReactInternalTypes';
import type {Lanes} from './ReactFiberLane';
import type {SuspenseState} from './ReactFiberSuspenseComponent.new';
import type {UpdateQueue} from './ReactUpdateQueue.new';
import type {FunctionComponentUpdateQueue} from './ReactFiberHooks.new';
import type {
Effect as HookEffect,
FunctionComponentUpdateQueue,
} from './ReactFiberHooks.new';
import type {Wakeable} from 'shared/ReactTypes';
import type {ReactPriorityLevel} from './ReactInternalTypes';
import type {OffscreenState} from './ReactFiberOffscreenComponent';
import type {HookEffectTag} from './ReactHookEffectTags';

import {unstable_wrap as Schedule_tracing_wrap} from 'scheduler/tracing';
import {
Expand Down Expand Up @@ -77,6 +81,8 @@ import {
getCommitTime,
recordLayoutEffectDuration,
startLayoutEffectTimer,
recordPassiveEffectDuration,
startPassiveEffectTimer,
} from './ReactProfilerTimer.new';
import {ProfileMode} from './ReactTypeOfMode';
import {commitUpdateQueue} from './ReactUpdateQueue.new';
Expand Down Expand Up @@ -121,6 +127,7 @@ import {
NoEffect as NoHookEffect,
HasEffect as HookHasEffect,
Layout as HookLayout,
Passive as HookPassive,
} from './ReactHookEffectTags';
import {didWarnAboutReassigningProps} from './ReactFiberBeginWork.new';
import {
Expand Down Expand Up @@ -308,7 +315,7 @@ function commitBeforeMutationLifeCycles(
);
}

function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
function commitHookEffectListUnmount(tag: HookEffectTag, finishedWork: Fiber) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
Expand All @@ -328,7 +335,43 @@ function commitHookEffectListUnmount(tag: number, finishedWork: Fiber) {
}
}

function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
// TODO: Remove this duplication.
function commitHookEffectListUnmount2(
// Tags to check for when deciding whether to unmount. e.g. to skip over
// layout effects
hookEffectTag: HookEffectTag,
fiber: Fiber,
): void {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
const {next, tag} = effect;
if ((tag & hookEffectTag) === hookEffectTag) {
const destroy = effect.destroy;
if (destroy !== undefined) {
effect.destroy = undefined;
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
startPassiveEffectTimer();
safelyCallDestroy(fiber, destroy);
recordPassiveEffectDuration(fiber);
} else {
safelyCallDestroy(fiber, destroy);
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 one is safely destroyed but not the other one.

}
}
}
effect = next;
} while (effect !== firstEffect);
}
}

function commitHookEffectListMount(tag: HookEffectTag, finishedWork: Fiber) {
const updateQueue: FunctionComponentUpdateQueue | null = (finishedWork.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
Expand Down Expand Up @@ -378,6 +421,83 @@ function commitHookEffectListMount(tag: number, finishedWork: Fiber) {
}
}

function invokePassiveEffectCreate(effect: HookEffect): void {
const create = effect.create;
effect.destroy = create();
}

// TODO: Remove this duplication.
function commitHookEffectListMount2(fiber: Fiber): void {
const updateQueue: FunctionComponentUpdateQueue | null = (fiber.updateQueue: any);
const lastEffect = updateQueue !== null ? updateQueue.lastEffect : null;
if (lastEffect !== null) {
const firstEffect = lastEffect.next;
let effect = firstEffect;
do {
const {next, tag} = effect;

if (
(tag & HookPassive) !== NoHookEffect &&
(tag & HookHasEffect) !== NoHookEffect
) {
if (__DEV__) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
startPassiveEffectTimer();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For layout effects we start the timer around the whole list but here we start the timer around each create function. Why the difference?

invokeGuardedCallback(
null,
invokePassiveEffectCreate,
null,
effect,
);
recordPassiveEffectDuration(fiber);
} else {
invokeGuardedCallback(
null,
invokePassiveEffectCreate,
null,
effect,
);
}
if (hasCaughtError()) {
invariant(fiber !== null, 'Should be working on an effect.');
const error = clearCaughtError();
captureCommitPhaseError(fiber, error);
}
} else {
try {
const create = effect.create;
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
try {
startPassiveEffectTimer();
effect.destroy = create();
} finally {
recordPassiveEffectDuration(fiber);
}
} else {
effect.destroy = create();
}
// TODO: This is missing the warning that exists in commitHookEffectListMount.
// The warning refers to useEffect but only applies to useLayoutEffect.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There's a warning in commitHookEffectListMount that I think we're missing here.

} catch (error) {
invariant(fiber !== null, 'Should be working on an effect.');
captureCommitPhaseError(fiber, error);
}
}
}

effect = next;
} while (effect !== firstEffect);
}
}

export function commitPassiveEffectDurations(
finishedRoot: FiberRoot,
finishedWork: Fiber,
Expand Down Expand Up @@ -1709,13 +1829,45 @@ export function isSuspenseBoundaryBeingHidden(
return false;
}

function commitResetTextContent(current: Fiber) {
function commitResetTextContent(current: Fiber): void {
if (!supportsMutation) {
return;
}
resetTextContent(current.stateNode);
}

function commitPassiveWork(finishedWork: Fiber): void {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block: {
commitHookEffectListUnmount2(HookPassive | HookHasEffect, finishedWork);
}
}
}

function commitPassiveUnmount(current: Fiber): void {
switch (current.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block:
commitHookEffectListUnmount2(HookPassive, current);
}
}

function commitPassiveLifeCycles(finishedWork: Fiber): void {
switch (finishedWork.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent:
case Block: {
commitHookEffectListMount2(finishedWork);
}
}
}

export {
commitBeforeMutationLifeCycles,
commitResetTextContent,
Expand All @@ -1725,4 +1877,7 @@ export {
commitLifeCycles,
commitAttachRef,
commitDetachRef,
commitPassiveUnmount,
commitPassiveWork,
commitPassiveLifeCycles,
};
Loading