Skip to content

Commit

Permalink
Remove LayoutStatic check from commit phase
Browse files Browse the repository at this point in the history 
Remove the static flag checks (which are just optimizations) until we fix the problem with incorrectly clearing static fields somewhere. (I've left TODO comments to re-add the checks later.)
  • Loading branch information
Brian Vaughn committed Apr 13, 2021
1 parent 70678d2 commit b1eeca2
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 148 deletions.
140 changes: 66 additions & 74 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ import {
MutationMask,
LayoutMask,
PassiveMask,
LayoutStatic,
RefStatic,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -1047,16 +1045,13 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) {
if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
// This method is called during mutation; it should detach refs within a hidden subtree.
// Attaching refs should be done elsewhere though (during layout).
if ((node.flags & RefStatic) !== NoFlags) {
if (isHidden) {
safelyDetachRef(node, finishedWork);
}
// TODO (Offscreen) Also check: flags & RefStatic
if (isHidden) {
safelyDetachRef(node, finishedWork);
}

if (
(node.subtreeFlags & (RefStatic | LayoutStatic)) !== NoFlags &&
node.child !== null
) {
// TODO (Offscreen) Also check: subtreeFlags & (RefStatic | LayoutStatic)
if (node.child !== null) {
node.child.return = node;
node = node.child;
continue;
Expand All @@ -1079,43 +1074,42 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) {
// Don't search any deeper. This tree should remain hidden.
} else if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
// When a mounted Suspense subtree gets hidden again, destroy any nested layout effects.
if ((node.flags & (RefStatic | LayoutStatic)) !== NoFlags) {
switch (node.tag) {
case FunctionComponent:
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
// Note that refs are attached by the useImperativeHandle() hook, not by commitAttachRef()
if (isHidden && !wasHidden) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
node.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
commitHookEffectListUnmount(HookLayout, node, finishedWork);
} finally {
recordLayoutEffectDuration(node);
}
} else {
// TODO (Offscreen) Check: flags & (RefStatic | LayoutStatic)
switch (node.tag) {
case FunctionComponent:
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
// Note that refs are attached by the useImperativeHandle() hook, not by commitAttachRef()
if (isHidden && !wasHidden) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
node.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
commitHookEffectListUnmount(HookLayout, node, finishedWork);
} finally {
recordLayoutEffectDuration(node);
}
} else {
commitHookEffectListUnmount(HookLayout, node, finishedWork);
}
break;
}
case ClassComponent: {
if (isHidden && !wasHidden) {
if ((node.flags & RefStatic) !== NoFlags) {
safelyDetachRef(node, finishedWork);
}
const instance = node.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(node, finishedWork, instance);
}
break;
}
case ClassComponent: {
if (isHidden && !wasHidden) {
// TODO (Offscreen) Check: flags & RefStatic
safelyDetachRef(node, finishedWork);

const instance = node.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(node, finishedWork, instance);
}
break;
}
break;
}
}

Expand Down Expand Up @@ -2392,11 +2386,9 @@ function commitLayoutEffects_begin(
if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
const visibilityChanged =
!offscreenSubtreeIsHidden && offscreenSubtreeWasHidden;
if (
visibilityChanged &&
(fiber.subtreeFlags & LayoutStatic) !== NoFlags &&
firstChild !== null
) {

// TODO (Offscreen) Also check: subtreeFlags & LayoutStatic
if (visibilityChanged && firstChild !== null) {
// We've just shown or hidden a Offscreen tree that contains layout effects.
// We only enter this code path for subtrees that are updated,
// because newly mounted ones would pass the LayoutMask check above.
Expand Down Expand Up @@ -2431,42 +2423,42 @@ function commitLayoutMountEffects_complete(
// Inside of an Offscreen subtree that changed visibility during this commit.
// If this subtree was hidden, layout effects will have already been destroyed (during mutation phase)
// but if it was just shown, we need to (re)create the effects now.
if ((fiber.flags & LayoutStatic) !== NoFlags) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
} finally {
recordLayoutEffectDuration(fiber);
}
} else {
// TODO (Offscreen) Check: flags & LayoutStatic
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
} finally {
recordLayoutEffectDuration(fiber);
}
break;
} else {
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
}
case ClassComponent: {
const instance = fiber.stateNode;
break;
}
case ClassComponent: {
const instance = fiber.stateNode;
if (typeof instance.componentDidMount === 'function') {
safelyCallComponentDidMount(fiber, fiber.return, instance);
break;
}
break;
}
}

if ((fiber.flags & RefStatic) !== NoFlags) {
switch (fiber.tag) {
case ClassComponent:
case HostComponent:
safelyAttachRef(fiber, fiber.return);
break;
}
// TODO (Offscreen) Check flags & RefStatic
switch (fiber.tag) {
case ClassComponent:
case HostComponent:
safelyAttachRef(fiber, fiber.return);
break;
}
} else if ((fiber.flags & LayoutMask) !== NoFlags) {
const current = fiber.alternate;
Expand Down
140 changes: 66 additions & 74 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,6 @@ import {
MutationMask,
LayoutMask,
PassiveMask,
LayoutStatic,
RefStatic,
} from './ReactFiberFlags';
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -1047,16 +1045,13 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) {
if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
// This method is called during mutation; it should detach refs within a hidden subtree.
// Attaching refs should be done elsewhere though (during layout).
if ((node.flags & RefStatic) !== NoFlags) {
if (isHidden) {
safelyDetachRef(node, finishedWork);
}
// TODO (Offscreen) Also check: flags & RefStatic
if (isHidden) {
safelyDetachRef(node, finishedWork);
}

if (
(node.subtreeFlags & (RefStatic | LayoutStatic)) !== NoFlags &&
node.child !== null
) {
// TODO (Offscreen) Also check: subtreeFlags & (RefStatic | LayoutStatic)
if (node.child !== null) {
node.child.return = node;
node = node.child;
continue;
Expand All @@ -1079,43 +1074,42 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) {
// Don't search any deeper. This tree should remain hidden.
} else if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
// When a mounted Suspense subtree gets hidden again, destroy any nested layout effects.
if ((node.flags & (RefStatic | LayoutStatic)) !== NoFlags) {
switch (node.tag) {
case FunctionComponent:
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
// Note that refs are attached by the useImperativeHandle() hook, not by commitAttachRef()
if (isHidden && !wasHidden) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
node.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
commitHookEffectListUnmount(HookLayout, node, finishedWork);
} finally {
recordLayoutEffectDuration(node);
}
} else {
// TODO (Offscreen) Check: flags & (RefStatic | LayoutStatic)
switch (node.tag) {
case FunctionComponent:
case ForwardRef:
case MemoComponent:
case SimpleMemoComponent: {
// Note that refs are attached by the useImperativeHandle() hook, not by commitAttachRef()
if (isHidden && !wasHidden) {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
node.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
commitHookEffectListUnmount(HookLayout, node, finishedWork);
} finally {
recordLayoutEffectDuration(node);
}
} else {
commitHookEffectListUnmount(HookLayout, node, finishedWork);
}
break;
}
case ClassComponent: {
if (isHidden && !wasHidden) {
if ((node.flags & RefStatic) !== NoFlags) {
safelyDetachRef(node, finishedWork);
}
const instance = node.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(node, finishedWork, instance);
}
break;
}
case ClassComponent: {
if (isHidden && !wasHidden) {
// TODO (Offscreen) Check: flags & RefStatic
safelyDetachRef(node, finishedWork);

const instance = node.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(node, finishedWork, instance);
}
break;
}
break;
}
}

Expand Down Expand Up @@ -2392,11 +2386,9 @@ function commitLayoutEffects_begin(
if (enableSuspenseLayoutEffectSemantics && isModernRoot) {
const visibilityChanged =
!offscreenSubtreeIsHidden && offscreenSubtreeWasHidden;
if (
visibilityChanged &&
(fiber.subtreeFlags & LayoutStatic) !== NoFlags &&
firstChild !== null
) {

// TODO (Offscreen) Also check: subtreeFlags & LayoutStatic
if (visibilityChanged && firstChild !== null) {
// We've just shown or hidden a Offscreen tree that contains layout effects.
// We only enter this code path for subtrees that are updated,
// because newly mounted ones would pass the LayoutMask check above.
Expand Down Expand Up @@ -2431,42 +2423,42 @@ function commitLayoutMountEffects_complete(
// Inside of an Offscreen subtree that changed visibility during this commit.
// If this subtree was hidden, layout effects will have already been destroyed (during mutation phase)
// but if it was just shown, we need to (re)create the effects now.
if ((fiber.flags & LayoutStatic) !== NoFlags) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
} finally {
recordLayoutEffectDuration(fiber);
}
} else {
// TODO (Offscreen) Check: flags & LayoutStatic
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
if (
enableProfilerTimer &&
enableProfilerCommitHooks &&
fiber.mode & ProfileMode
) {
try {
startLayoutEffectTimer();
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
} finally {
recordLayoutEffectDuration(fiber);
}
break;
} else {
safelyCallCommitHookLayoutEffectListMount(fiber, fiber.return);
}
case ClassComponent: {
const instance = fiber.stateNode;
break;
}
case ClassComponent: {
const instance = fiber.stateNode;
if (typeof instance.componentDidMount === 'function') {
safelyCallComponentDidMount(fiber, fiber.return, instance);
break;
}
break;
}
}

if ((fiber.flags & RefStatic) !== NoFlags) {
switch (fiber.tag) {
case ClassComponent:
case HostComponent:
safelyAttachRef(fiber, fiber.return);
break;
}
// TODO (Offscreen) Check flags & RefStatic
switch (fiber.tag) {
case ClassComponent:
case HostComponent:
safelyAttachRef(fiber, fiber.return);
break;
}
} else if ((fiber.flags & LayoutMask) !== NoFlags) {
const current = fiber.alternate;
Expand Down

0 comments on commit b1eeca2

Please sign in to comment.