Favor fallthrough switch instead of case statements for work tags (#1…

…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
facebook · Dec 18, 2019 · 4c27037 · 4c27037
1 parent 6fef7c4
commit 4c27037
Show file tree

Hide file tree

Showing 3 changed files with 76 additions and 93 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js
@@ -1075,7 +1075,7 @@ function mountLazyComponent(
         resolvedProps,
         renderExpirationTime,
       );
-      break;
+      return child;
     }
     case ClassComponent: {
       if (__DEV__) {
@@ -1090,7 +1090,7 @@ function mountLazyComponent(
         resolvedProps,
         renderExpirationTime,
       );
-      break;
+      return child;
     }
     case ForwardRef: {
       if (__DEV__) {
@@ -1105,7 +1105,7 @@ function mountLazyComponent(
         resolvedProps,
         renderExpirationTime,
       );
-      break;
+      return child;
     }
     case MemoComponent: {
       if (__DEV__) {
@@ -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(

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -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(
@@ -420,7 +418,7 @@ function commitLifeCycles(
     case ForwardRef:
     case SimpleMemoComponent: {
       commitHookEffectList(UnmountLayout, MountLayout, finishedWork);
-      break;
+      return;
     }
     case ClassComponent: {
       const instance = finishedWork.stateNode;
@@ -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) {
@@ -785,7 +781,7 @@ function commitUnmount(
           });
         }
       }
-      break;
+      return;
     }
     case ClassComponent: {
       safelyDetachRef(current);
@@ -843,6 +839,7 @@ function commitUnmount(
       if (enableScopeAPI) {
         safelyDetachRef(current);
       }
+      return;
     }
   }
 }
@@ -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 {
@@ -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) {
@@ -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) {

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.js b/packages/react-reconciler/src/ReactFiberCompleteWork.js
@@ -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);
@@ -670,7 +674,7 @@ function completeWork(
         }
       }
       updateHostContainer(workInProgress);
-      break;
+      return null;
     }
     case HostComponent: {
       popHostContext(workInProgress);
@@ -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();
@@ -783,7 +787,7 @@ function completeWork(
           markRef(workInProgress);
         }
       }
-      break;
+      return null;
     }
     case HostText: {
       let newText = newProps;
@@ -817,10 +821,8 @@ function completeWork(
           );
         }
       }
-      break;
+      return null;
     }
-    case ForwardRef:
-      break;
     case SuspenseComponent: {
       popSuspenseContext(workInProgress);
       const nextState: null | SuspenseState = workInProgress.memoizedState;
@@ -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);
@@ -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 =
@@ -1198,7 +1190,7 @@ function completeWork(
         // Do a pass over the next row.
         return next;
       }
-      break;
+      return null;
     }
     case FundamentalComponent: {
       if (enableFundamentalAPI) {
@@ -1248,6 +1240,7 @@ function completeWork(
             markUpdate(workInProgress);
           }
         }
+        return null;
       }
       break;
     }
@@ -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};