Merge pull request #8797 from acdlite/fix-arrays

[Fiber] disableNewFiberFeatures bugfix: host component children arrays

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');
-
-      beforeEach(() => {
-        ReactFeatureFlags.disableNewFiberFeatures = true;
-      });
-
-      afterEach(() => {
-        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 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/);
-      });
-    });
   }
 });
+
+// disableNewFiberFeatures currently defaults to true in test
+describe('disableNewFiberFeatures', () => {
+  var container;
+  var ReactFeatureFlags;
+
+  beforeEach(() => {
+    container = document.createElement('div');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = true;
+  });
+
+  afterEach(() => {
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
+  });
+
+  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;
+    }
+
+    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,13 @@ 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(
@@ -1125,45 +1126,72 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
               priority
             ));
         }
-      }
-
-      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;
-              }
+        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 (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 (getIteratorFn(newChild)) {
-        return reconcileChildrenIterator(
-          returnFiber,
-          currentFirstChild,
-          newChild,
-          priority
-        );
-      }
+    if (isArray(newChild)) {
+      return reconcileChildrenArray(
+        returnFiber,
+        currentFirstChild,
+        newChild,
+        priority
+      );
     }
 
-    if (typeof newChild === 'undefined') {
+    if (getIteratorFn(newChild)) {
+      return reconcileChildrenIterator(
+        returnFiber,
+        currentFirstChild,
+        newChild,
+        priority
+      );
+    }
+
+    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;

src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js

@@ -14,6 +14,7 @@
 var React;
 var ReactNoop;
 var ReactCoroutine;
+var ReactFeatureFlags;
 
 describe('ReactCoroutine', () => {
   beforeEach(() => {
@@ -21,6 +22,8 @@ describe('ReactCoroutine', () => {
     React = require('React');
     ReactNoop = require('ReactNoop');
     ReactCoroutine = require('ReactCoroutine');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('should render a coroutine', () => {

src/renderers/shared/fiber/__tests__/ReactIncremental-test.js

@@ -13,12 +13,16 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncremental', () => {
   beforeEach(() => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('should render a simple component', () => {

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js

@@ -13,12 +13,15 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncrementalErrorHandling', () => {
   beforeEach(() => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   function div(...children) {

src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js

@@ -13,12 +13,15 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncrementalReflection', () => {
   beforeEach(() => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('handles isMounted even when the initial render is deferred', () => {

src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js

@@ -13,12 +13,15 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncrementalScheduling', () => {
   beforeEach(() => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   function span(prop) {

src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js

@@ -13,12 +13,15 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncrementalSideEffects', () => {
   beforeEach(() => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   function normalizeCodeLocInfo(str) {

src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js

@@ -13,12 +13,15 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 describe('ReactIncrementalUpdates', () => {
   beforeEach(() => {
     jest.resetModuleRegistry();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('applies updates in order of priority', () => {

src/renderers/shared/fiber/__tests__/ReactTopLevelFragment-test.js

@@ -13,6 +13,7 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 // This is a new feature in Fiber so I put it in its own test file. It could
 // probably move to one of the other test files once it is official.
@@ -21,6 +22,8 @@ describe('ReactTopLevelFragment', function() {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('should render a simple fragment at the top of a component', function() {

src/renderers/shared/fiber/__tests__/ReactTopLevelText-test.js

@@ -13,6 +13,7 @@
 
 var React;
 var ReactNoop;
+var ReactFeatureFlags;
 
 // This is a new feature in Fiber so I put it in its own test file. It could
 // probably move to one of the other test files once it is official.
@@ -21,6 +22,8 @@ describe('ReactTopLevelText', () => {
     jest.resetModules();
     React = require('React');
     ReactNoop = require('ReactNoop');
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
   });
 
   it('should render a component returning strings directly from render', () => {

src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js

@@ -247,18 +247,24 @@ describe('ReactEmptyComponent', () => {
   });
 
   it('can render null at the top level', () => {
+    var ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
     var div = document.createElement('div');
 
-    if (ReactDOMFeatureFlags.useFiber) {
-      ReactDOM.render(null, div);
-      expect(div.innerHTML).toBe('');
-    } else {
-      // Stack does not implement this.
-      expect(function() {
+    try {
+      if (ReactDOMFeatureFlags.useFiber) {
         ReactDOM.render(null, div);
-      }).toThrowError(
-        'ReactDOM.render(): Invalid component element.'
-      );
+        expect(div.innerHTML).toBe('');
+      } else {
+        // Stack does not implement this.
+        expect(function() {
+          ReactDOM.render(null, div);
+        }).toThrowError(
+          'ReactDOM.render(): Invalid component element.'
+        );
+      }
+    } finally {
+      ReactFeatureFlags.disableNewFiberFeatures = true;
     }
   });
 

src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js

@@ -152,13 +152,8 @@ describe('ReactStatelessComponent', () => {
     expect(function() {
       ReactTestUtils.renderIntoDocument(<div><NotAComponent /></div>);
     }).toThrowError(
-      ReactDOMFeatureFlags.useFiber ?
-        // Fiber gives a more specific error message for undefined because it
-        // supports more return types.
-        'NotAComponent(...): Nothing was returned from render' :
-        // Stack's message is generic.
-        'NotAComponent(...): A valid React element (or null) must be returned. ' +
-        'You may have returned undefined, an array or some other invalid object.'
+      'NotAComponent(...): A valid React element (or null) must be returned. ' +
+      'You may have returned undefined, an array or some other invalid object.'
     );
   });
 

src/renderers/testing/__tests__/ReactTestRenderer-test.js

@@ -14,8 +14,14 @@
 var React = require('React');
 var ReactTestRenderer = require('ReactTestRenderer');
 var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
+var ReactFeatureFlags;
 
 describe('ReactTestRenderer', () => {
+  beforeEach(() => {
+    ReactFeatureFlags = require('ReactFeatureFlags');
+    ReactFeatureFlags.disableNewFiberFeatures = false;
+  });
+
   function normalizeCodeLocInfo(str) {
     return str && str.replace(/\(at .+?:\d+\)/g, '(at **)');
   }