Merge pull request #8797 from acdlite/fix-arrays

[Fiber] disableNewFiberFeatures bugfix: host component children arrays

Author: acdlite

scripts/fiber/tests-passing.txt

@@ -554,6 +554,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
 * should not crash encountering low-priority tree
 * throws if non-element passed to top-level render
 * throws if something other than false, null, or an element is returned from render
+* treats mocked render functions as if they return null
 
 src/renderers/dom/shared/__tests__/CSSProperty-test.js
 * should generate browser prefixes for its `isUnitlessNumber`

scripts/jest/test-framework-setup.js

@@ -9,6 +9,12 @@ jest.mock('ReactDOMFeatureFlags', () => {
     useFiber: flags.useFiber || !!process.env.REACT_DOM_JEST_USE_FIBER,
   });
 });
+jest.mock('ReactFeatureFlags', () => {
+  const flags = require.requireActual('ReactFeatureFlags');
+  return Object.assign({}, flags, {
+    disableNewFiberFeatures: true,
+  });
+});
 
 // Error logging varies between Fiber and Stack;
 // Rather than fork dozens of tests, mock the error-logging file by default.

src/renderers/dom/fiber/ReactDOMFiber.js

@@ -18,6 +18,7 @@ import type { ReactNodeList } from 'ReactTypes';
 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
 var ReactControlledComponent = require('ReactControlledComponent');
 var ReactDOMComponentTree = require('ReactDOMComponentTree');
+var ReactFeatureFlags = require('ReactFeatureFlags');
 var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
 var ReactDOMFiberComponent = require('ReactDOMFiberComponent');
 var ReactDOMFrameScheduling = require('ReactDOMFrameScheduling');
@@ -27,6 +28,8 @@ var ReactFiberReconciler = require('ReactFiberReconciler');
 var ReactInputSelection = require('ReactInputSelection');
 var ReactInstanceMap = require('ReactInstanceMap');
 var ReactPortal = require('ReactPortal');
+var { isValidElement } = require('ReactElement');
+
 
 var findDOMNode = require('findDOMNode');
 var invariant = require('invariant');
@@ -49,6 +52,7 @@ if (__DEV__) {
   var { updatedAncestorInfo } = validateDOMNesting;
 }
 
+
 const DOCUMENT_NODE = 9;
 
 ReactDOMInjection.inject();
@@ -352,6 +356,17 @@ var ReactDOM = {
 
   render(element : ReactElement<any>, container : DOMContainerElement, callback: ?Function) {
     validateContainer(container);
+
+    if (ReactFeatureFlags.disableNewFiberFeatures) {
+      // Top-level check occurs here instead of inside child reconciler because
+      // because requirements vary between renderers. E.g. React Art
+      // allows arrays.
+      invariant(
+        isValidElement(element),
+        'render(): Invalid component element.'
+      );
+    }
+
     return renderSubtreeIntoContainer(null, element, container, callback);
   },
 

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -18,9 +18,17 @@ var ReactTestUtils = require('ReactTestUtils');
 
 describe('ReactDOMFiber', () => {
   var container;
+  var ReactFeatureFlags;
 
   beforeEach(() => {
     container = document.createElement('div');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
+  });
+
+  afterEach(() => {
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = true;
   });
 
   it('should render strings as children', () => {
@@ -1085,40 +1093,49 @@ describe('ReactDOMFiber', () => {
         container
       );
     });
+  }
+});
 
-    describe('disableNewFiberFeatures', () => {
-      var ReactFeatureFlags = require('ReactFeatureFlags');
+// disableNewFiberFeatures currently defaults to true in test
+describe('disableNewFiberFeatures', () => {
+  var container;
+  var ReactFeatureFlags;
 
-      beforeEach(() => {
-        ReactFeatureFlags.disableNewFiberFeatures = true;
-      });
+  beforeEach(() => {
+    container = document.createElement('div');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = true;
+  });
 
-      afterEach(() => {
-        ReactFeatureFlags.disableNewFiberFeatures = false;
-      });
+  afterEach(() => {
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
+  });
 
-      it('throws if non-element passed to top-level render', () => {
-        // FIXME: These assertions pass individually, but they leave React in
-        // an inconsistent state. This suggests an error-handling bug. I'll fix
-        // this in a separate PR.
-        const message = 'render(): Invalid component element.';
-        expect(() => ReactDOM.render(null, container)).toThrow(message, container);
-        expect(() => ReactDOM.render(undefined, container)).toThrow(message, container);
-        expect(() => ReactDOM.render(false, container)).toThrow(message, container);
-        expect(() => ReactDOM.render('Hi', container)).toThrow(message, container);
-        expect(() => ReactDOM.render(999, container)).toThrow(message, container);
-        expect(() => ReactDOM.render([<div />], container)).toThrow(message, container);
-      });
+  it('throws if non-element passed to top-level render', () => {
+    const message = 'render(): Invalid component element.';
+    expect(() => ReactDOM.render(null, container)).toThrow(message, container);
+    expect(() => ReactDOM.render(undefined, container)).toThrow(message, container);
+    expect(() => ReactDOM.render(false, container)).toThrow(message, container);
+    expect(() => ReactDOM.render('Hi', container)).toThrow(message, container);
+    expect(() => ReactDOM.render(999, container)).toThrow(message, container);
+    expect(() => ReactDOM.render([<div />], container)).toThrow(message, container);
+  });
 
-      it('throws if something other than false, null, or an element is returned from render', () => {
-        function Render(props) {
-          return props.children;
-        }
+  it('throws if something other than false, null, or an element is returned from render', () => {
+    function Render(props) {
+      return props.children;
+    }
 
-        expect(() => ReactDOM.render(<Render>Hi</Render>, container)).toThrow(/You may have returned undefined/);
-        expect(() => ReactDOM.render(<Render>{999}</Render>, container)).toThrow(/You may have returned undefined/);
-        expect(() => ReactDOM.render(<Render>[<div />]</Render>, container)).toThrow(/You may have returned undefined/);
-      });
-    });
-  }
+    expect(() => ReactDOM.render(<Render>Hi</Render>, container)).toThrow(/You may have returned undefined/);
+    expect(() => ReactDOM.render(<Render>{999}</Render>, container)).toThrow(/You may have returned undefined/);
+    expect(() => ReactDOM.render(<Render>[<div />]</Render>, container)).toThrow(/You may have returned undefined/);
+  });
+
+  it('treats mocked render functions as if they return null', () => {
+    class Mocked extends React.Component {}
+    Mocked.prototype.render = jest.fn();
+    ReactDOM.render(<Mocked />, container);
+    expect(container.textContent).toEqual('');
+  });
 });

src/renderers/shared/fiber/ReactChildFiber.js

@@ -64,7 +64,6 @@ const {
   FunctionalComponent,
   ClassComponent,
   HostText,
-  HostRoot,
   HostPortal,
   CoroutineComponent,
   YieldComponent,
@@ -75,7 +74,6 @@ const {
   NoEffect,
   Placement,
   Deletion,
-  Err,
 } = ReactTypeOfSideEffect;
 
 function coerceRef(current: ?Fiber, element: ReactElement) {
@@ -1104,10 +1102,31 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     // not as a fragment. Nested arrays on the other hand will be treated as
     // fragment nodes. Recursion happens at the normal flow.
 
-    if (ReactFeatureFlags.disableNewFiberFeatures) {
+    const disableNewFiberFeatures = ReactFeatureFlags.disableNewFiberFeatures;
+
+    // Handle object types
+    if (typeof newChild === 'object' && newChild !== null) {
       // Support only the subset of return types that Stack supports. Treat
       // everything else as empty, but log a warning.
-      if (typeof newChild === 'object' && newChild !== null) {
+      if (disableNewFiberFeatures) {
+        switch (newChild.$$typeof) {
+          case REACT_ELEMENT_TYPE:
+            return placeSingleChild(reconcileSingleElement(
+              returnFiber,
+              currentFirstChild,
+              newChild,
+              priority
+            ));
+
+          case REACT_PORTAL_TYPE:
+            return placeSingleChild(reconcileSinglePortal(
+              returnFiber,
+              currentFirstChild,
+              newChild,
+              priority
+            ));
+        }
+      } else {
         switch (newChild.$$typeof) {
           case REACT_ELEMENT_TYPE:
             return placeSingleChild(reconcileSingleElement(
@@ -1117,6 +1136,22 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
               priority
             ));
 
+          case REACT_COROUTINE_TYPE:
+            return placeSingleChild(reconcileSingleCoroutine(
+              returnFiber,
+              currentFirstChild,
+              newChild,
+              priority
+            ));
+
+          case REACT_YIELD_TYPE:
+            return placeSingleChild(reconcileSingleYield(
+              returnFiber,
+              currentFirstChild,
+              newChild,
+              priority
+            ));
+
           case REACT_PORTAL_TYPE:
             return placeSingleChild(reconcileSinglePortal(
               returnFiber,
@@ -1126,44 +1161,37 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
             ));
         }
       }
+    }
 
-      if (returnFiber.tag === HostRoot) {
-        // Top-level only accepts elements or portals
-        invariant(
-          // If the root has an error effect, this is an intentional unmount.
-          // Don't throw an error.
-          returnFiber.effectTag & Err,
-          'render(): Invalid component element.'
-        );
-      } else {
-        switch (returnFiber.tag) {
-          case ClassComponent: {
-            if (__DEV__) {
-              const instance = returnFiber.stateNode;
-              if (instance.render._isMockFunction) {
-                // We allow auto-mocks to proceed as if they're
-                // returning null.
-                break;
-              }
+    if (disableNewFiberFeatures) {
+      // The new child is not an element. If it's not null or false,
+      // and the return fiber is a composite component, throw an error.
+      switch (returnFiber.tag) {
+        case ClassComponent: {
+          if (__DEV__) {
+            const instance = returnFiber.stateNode;
+            if (instance.render._isMockFunction && typeof newChild === 'undefined') {
+              // We allow auto-mocks to proceed as if they're
+              // returning null.
+              break;
             }
           }
-          // Intentionally fall through to the next case, which handles both
-          // functions and classes
-          // eslint-disable-next-lined no-fallthrough
-          case FunctionalComponent: {
-            // Composites accept elements, portals, null, or false
-            const Component = returnFiber.type;
-            invariant(
-              newChild === null || newChild === false,
-              '%s.render(): A valid React element (or null) must be ' +
-              'returned. You may have returned undefined, an array or some ' +
-              'other invalid object.',
-              Component.displayName || Component.name || 'Component'
-            );
-          }
+        }
+        // Intentionally fall through to the next case, which handles both
+        // functions and classes
+        // eslint-disable-next-lined no-fallthrough
+        case FunctionalComponent: {
+          // Composites accept elements, portals, null, or false
+          const Component = returnFiber.type;
+          invariant(
+            newChild === null || newChild === false,
+            '%s(...): A valid React element (or null) must be ' +
+            'returned. You may have returned undefined, an array or some ' +
+            'other invalid object.',
+            Component.displayName || Component.name || 'Component'
+          );
         }
       }
-      return deleteRemainingChildren(returnFiber, currentFirstChild);
     }
 
     if (typeof newChild === 'string' || typeof newChild === 'number') {
@@ -1175,65 +1203,29 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       ));
     }
 
-    if (typeof newChild === 'object' && newChild !== null) {
-      switch (newChild.$$typeof) {
-        case REACT_ELEMENT_TYPE:
-          return placeSingleChild(reconcileSingleElement(
-            returnFiber,
-            currentFirstChild,
-            newChild,
-            priority
-          ));
-
-        case REACT_COROUTINE_TYPE:
-          return placeSingleChild(reconcileSingleCoroutine(
-            returnFiber,
-            currentFirstChild,
-            newChild,
-            priority
-          ));
-
-        case REACT_YIELD_TYPE:
-          return placeSingleChild(reconcileSingleYield(
-            returnFiber,
-            currentFirstChild,
-            newChild,
-            priority
-          ));
-
-        case REACT_PORTAL_TYPE:
-          return placeSingleChild(reconcileSinglePortal(
-            returnFiber,
-            currentFirstChild,
-            newChild,
-            priority
-          ));
-      }
-
-      if (isArray(newChild)) {
-        return reconcileChildrenArray(
-          returnFiber,
-          currentFirstChild,
-          newChild,
-          priority
-        );
-      }
+    if (isArray(newChild)) {
+      return reconcileChildrenArray(
+        returnFiber,
+        currentFirstChild,
+        newChild,
+        priority
+      );
+    }
 
-      if (getIteratorFn(newChild)) {
-        return reconcileChildrenIterator(
-          returnFiber,
-          currentFirstChild,
-          newChild,
-          priority
-        );
-      }
+    if (getIteratorFn(newChild)) {
+      return reconcileChildrenIterator(
+        returnFiber,
+        currentFirstChild,
+        newChild,
+        priority
+      );
     }
 
-    if (typeof newChild === 'undefined') {
+    if (!disableNewFiberFeatures && typeof newChild === 'undefined') {
+      // If the new child is undefined, and the return fiber is a composite
+      // component, throw an error. If Fiber return types are disabled,
+      // we already threw above.
       switch (returnFiber.tag) {
-        case HostRoot:
-          // TODO: Top-level render
-          break;
         case ClassComponent: {
           if (__DEV__) {
             const instance = returnFiber.stateNode;