Skip to content

Commit

Permalink
Favor fallthrough switch instead of case statements for work tags (#1…
Browse files Browse the repository at this point in the history
…7648)

* Favor fallthrough switch instead of case statements for work tags

Currently we're inconsistently handling tags that are only relevant
for certain flags. We should throw if the tag is not part of the built
feature flags. This should also mean that the case statements can be
eliminated.

We can achieve this effect by putting the invariant outside of the switch
and always early return in the switch. We already do this in beginWork.
This PR makes this consistent in other places.

* Fail if fundamental/scope tags are discovered without the flag on
  • Loading branch information
sebmarkbage authored Dec 18, 2019
1 parent 6fef7c4 commit 4c27037
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 93 deletions.
49 changes: 23 additions & 26 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1075,7 +1075,7 @@ function mountLazyComponent(
resolvedProps,
renderExpirationTime,
);
break;
return child;
}
case ClassComponent: {
if (__DEV__) {
Expand All @@ -1090,7 +1090,7 @@ function mountLazyComponent(
resolvedProps,
renderExpirationTime,
);
break;
return child;
}
case ForwardRef: {
if (__DEV__) {
Expand All @@ -1105,7 +1105,7 @@ function mountLazyComponent(
resolvedProps,
renderExpirationTime,
);
break;
return child;
}
case MemoComponent: {
if (__DEV__) {
Expand All @@ -1130,32 +1130,29 @@ function mountLazyComponent(
updateExpirationTime,
renderExpirationTime,
);
break;
return child;
}
default: {
let hint = '';
if (__DEV__) {
if (
Component !== null &&
typeof Component === 'object' &&
Component.$$typeof === REACT_LAZY_TYPE
) {
hint = ' Did you wrap a component in React.lazy() more than once?';
}
}
// This message intentionally doesn't mention ForwardRef or MemoComponent
// because the fact that it's a separate type of work is an
// implementation detail.
invariant(
false,
'Element type is invalid. Received a promise that resolves to: %s. ' +
'Lazy element type must resolve to a class or function.%s',
Component,
hint,
);
}
let hint = '';
if (__DEV__) {
if (
Component !== null &&
typeof Component === 'object' &&
Component.$$typeof === REACT_LAZY_TYPE
) {
hint = ' Did you wrap a component in React.lazy() more than once?';
}
}
return child;
// This message intentionally doesn't mention ForwardRef or MemoComponent
// because the fact that it's a separate type of work is an
// implementation detail.
invariant(
false,
'Element type is invalid. Received a promise that resolves to: %s. ' +
'Lazy element type must resolve to a class or function.%s',
Component,
hint,
);
}

function mountIncompleteClassComponent(
Expand Down
59 changes: 27 additions & 32 deletions packages/react-reconciler/src/ReactFiberCommitWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -320,14 +320,12 @@ function commitBeforeMutationLifeCycles(
case IncompleteClassComponent:
// Nothing to do for these component types
return;
default: {
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}
}
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}

function commitHookEffectList(
Expand Down Expand Up @@ -420,7 +418,7 @@ function commitLifeCycles(
case ForwardRef:
case SimpleMemoComponent: {
commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
break;
return;
}
case ClassComponent: {
const instance = finishedWork.stateNode;
Expand Down Expand Up @@ -629,14 +627,12 @@ function commitLifeCycles(
case FundamentalComponent:
case ScopeComponent:
return;
default: {
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}
}
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}

function hideOrUnhideAllChildren(finishedWork, isHidden) {
Expand Down Expand Up @@ -785,7 +781,7 @@ function commitUnmount(
});
}
}
break;
return;
}
case ClassComponent: {
safelyDetachRef(current);
Expand Down Expand Up @@ -843,6 +839,7 @@ function commitUnmount(
if (enableScopeAPI) {
safelyDetachRef(current);
}
return;
}
}
}
Expand Down Expand Up @@ -943,14 +940,12 @@ function commitContainer(finishedWork: Fiber) {
replaceContainerChildren(containerInfo, pendingChildren);
return;
}
default: {
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}
}
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}

function getHostParentFiber(fiber: Fiber): Fiber {
Expand Down Expand Up @@ -1408,8 +1403,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
if (enableFundamentalAPI) {
const fundamentalInstance = finishedWork.stateNode;
updateFundamentalComponent(fundamentalInstance);
return;
}
return;
break;
}
case ScopeComponent: {
if (enableScopeAPI) {
Expand All @@ -1424,17 +1420,16 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
updateDeprecatedEventListeners(nextListeners, finishedWork, null);
}
}
return;
}
return;
}
default: {
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
break;
}
}
invariant(
false,
'This unit of work tag should not have side-effects. This error is ' +
'likely caused by a bug in React. Please file an issue.',
);
}

function commitSuspenseComponent(finishedWork: Fiber) {
Expand Down
61 changes: 26 additions & 35 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -638,18 +638,22 @@ function completeWork(

switch (workInProgress.tag) {
case IndeterminateComponent:
break;
case LazyComponent:
break;
case SimpleMemoComponent:
case FunctionComponent:
break;
case ForwardRef:
case Fragment:
case Mode:
case Profiler:
case ContextConsumer:
case MemoComponent:
return null;
case ClassComponent: {
const Component = workInProgress.type;
if (isLegacyContextProvider(Component)) {
popLegacyContext(workInProgress);
}
break;
return null;
}
case HostRoot: {
popHostContainer(workInProgress);
Expand All @@ -670,7 +674,7 @@ function completeWork(
}
}
updateHostContainer(workInProgress);
break;
return null;
}
case HostComponent: {
popHostContext(workInProgress);
Expand Down Expand Up @@ -704,7 +708,7 @@ function completeWork(
'caused by a bug in React. Please file an issue.',
);
// This can happen when we abort work.
break;
return null;
}

const currentHostContext = getHostContext();
Expand Down Expand Up @@ -783,7 +787,7 @@ function completeWork(
markRef(workInProgress);
}
}
break;
return null;
}
case HostText: {
let newText = newProps;
Expand Down Expand Up @@ -817,10 +821,8 @@ function completeWork(
);
}
}
break;
return null;
}
case ForwardRef:
break;
case SuspenseComponent: {
popSuspenseContext(workInProgress);
const nextState: null | SuspenseState = workInProgress.memoizedState;
Expand Down Expand Up @@ -961,34 +963,24 @@ function completeWork(
// Always notify the callback
workInProgress.effectTag |= Update;
}
break;
return null;
}
case Fragment:
break;
case Mode:
break;
case Profiler:
break;
case HostPortal:
popHostContainer(workInProgress);
updateHostContainer(workInProgress);
break;
return null;
case ContextProvider:
// Pop provider fiber
popProvider(workInProgress);
break;
case ContextConsumer:
break;
case MemoComponent:
break;
return null;
case IncompleteClassComponent: {
// Same as class component case. I put it down here so that the tags are
// sequential to ensure this switch is compiled to a jump table.
const Component = workInProgress.type;
if (isLegacyContextProvider(Component)) {
popLegacyContext(workInProgress);
}
break;
return null;
}
case SuspenseListComponent: {
popSuspenseContext(workInProgress);
Expand All @@ -999,7 +991,7 @@ function completeWork(
if (renderState === null) {
// We're running in the default, "independent" mode. We don't do anything
// in this mode.
break;
return null;
}

let didSuspendAlready =
Expand Down Expand Up @@ -1198,7 +1190,7 @@ function completeWork(
// Do a pass over the next row.
return next;
}
break;
return null;
}
case FundamentalComponent: {
if (enableFundamentalAPI) {
Expand Down Expand Up @@ -1248,6 +1240,7 @@ function completeWork(
markUpdate(workInProgress);
}
}
return null;
}
break;
}
Expand Down Expand Up @@ -1296,19 +1289,17 @@ function completeWork(
markRef(workInProgress);
}
}
return null;
}
break;
}
default:
invariant(
false,
'Unknown unit of work tag (%s). This error is likely caused by a bug in ' +
'React. Please file an issue.',
workInProgress.tag,
);
}

return null;
invariant(
false,
'Unknown unit of work tag (%s). This error is likely caused by a bug in ' +
'React. Please file an issue.',
workInProgress.tag,
);
}

export {completeWork};

0 comments on commit 4c27037

Please sign in to comment.