From 16956ed64044665a44ed7f9d2e7c3d8933188926 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sat, 14 Jan 2017 18:22:07 -0800 Subject: [PATCH 1/6] disableNewFiberFeatures bugfix: host component children arrays --- scripts/fiber/tests-passing.txt | 1 + .../dom/fiber/__tests__/ReactDOMFiber-test.js | 8 ++++++++ src/renderers/shared/fiber/ReactChildFiber.js | 19 +++++++++++++++++++ 3 files changed, 28 insertions(+) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 050dd41e1b5..9dbe80d6b3b 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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 +* still accepts arrays as host children src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 41c7f129b79..185686c398f 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1119,6 +1119,14 @@ describe('ReactDOMFiber', () => { expect(() => ReactDOM.render({999}, container)).toThrow(/You may have returned undefined/); expect(() => ReactDOM.render([
], container)).toThrow(/You may have returned undefined/); }); + + it('still accepts arrays as host children', () => { + function Render(props) { + return
{props.children}
; + } + ReactDOM.render({['foo', 'bar']}, container); + expect(container.textContent).toEqual('foobar'); + }); }); } }); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index c741d94bddf..0a3ca1d4ec1 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -1163,6 +1163,25 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } } } + + if (isArray(newChild)) { + return reconcileChildrenArray( + returnFiber, + currentFirstChild, + newChild, + priority + ); + } + + if (getIteratorFn(newChild)) { + return reconcileChildrenIterator( + returnFiber, + currentFirstChild, + newChild, + priority + ); + } + return deleteRemainingChildren(returnFiber, currentFirstChild); } From 1ac6be267784b7c42f4d478460ba374bb4626ae8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 17 Jan 2017 09:57:01 -0800 Subject: [PATCH 2/6] Add test for iterables, too --- scripts/fiber/tests-passing.txt | 2 +- src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 9dbe80d6b3b..7f080729aa4 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -554,7 +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 -* still accepts arrays as host children +* still accepts arrays and iterables as host children src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 185686c398f..baffed3ca6c 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1120,12 +1120,14 @@ describe('ReactDOMFiber', () => { expect(() => ReactDOM.render([
], container)).toThrow(/You may have returned undefined/); }); - it('still accepts arrays as host children', () => { + it('still accepts arrays and iterables as host children', () => { function Render(props) { return
{props.children}
; } ReactDOM.render({['foo', 'bar']}, container); expect(container.textContent).toEqual('foobar'); + ReactDOM.render({new Set(['foo', 'bar'])}, container); + expect(container.textContent).toEqual('foobar'); }); }); } From 5c4362dc5c30357aed829e630ca4a03bd812e482 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 17 Jan 2017 10:17:55 -0800 Subject: [PATCH 3/6] Remove outdated TODO --- src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index baffed3ca6c..7e23c864c91 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1098,9 +1098,6 @@ describe('ReactDOMFiber', () => { }); 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); From c0e5df66ecc4f98140d1089bc1feb4a78f2a32f5 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 17 Jan 2017 11:16:24 -0800 Subject: [PATCH 4/6] Make disableNewFiberFeatures = true the default when runnings tests This exposed a few more cases that I missed. --- scripts/fiber/tests-passing.txt | 1 - scripts/jest/test-framework-setup.js | 6 ++ src/renderers/dom/fiber/ReactDOMFiber.js | 15 ++++ .../dom/fiber/__tests__/ReactDOMFiber-test.js | 77 ++++++++++--------- src/renderers/shared/fiber/ReactChildFiber.js | 64 ++++++++------- .../fiber/__tests__/ReactCoroutine-test.js | 3 + .../fiber/__tests__/ReactIncremental-test.js | 4 + .../ReactIncrementalErrorHandling-test.js | 3 + .../ReactIncrementalReflection-test.js | 3 + .../ReactIncrementalScheduling-test.js | 3 + .../ReactIncrementalSideEffects-test.js | 3 + .../__tests__/ReactIncrementalUpdates-test.js | 3 + .../__tests__/ReactTopLevelFragment-test.js | 3 + .../fiber/__tests__/ReactTopLevelText-test.js | 3 + .../__tests__/ReactEmptyComponent-test.js | 24 +++--- .../__tests__/ReactStatelessComponent-test.js | 9 +-- .../__tests__/ReactTestRenderer-test.js | 6 ++ 17 files changed, 143 insertions(+), 87 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7f080729aa4..050dd41e1b5 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -554,7 +554,6 @@ 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 -* still accepts arrays and iterables as host children src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` diff --git a/scripts/jest/test-framework-setup.js b/scripts/jest/test-framework-setup.js index 6128ed6ed80..dc8fd2fc7da 100644 --- a/scripts/jest/test-framework-setup.js +++ b/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. diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 8a33133ad4b..7045a874e11 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/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, 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); }, diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 7e23c864c91..a4238218c3c 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/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,47 +1093,42 @@ describe('ReactDOMFiber', () => { container ); }); + } +}); - describe('disableNewFiberFeatures', () => { - var ReactFeatureFlags = require('ReactFeatureFlags'); - - beforeEach(() => { - ReactFeatureFlags.disableNewFiberFeatures = true; - }); +// disableNewFiberFeatures currently defaults to true in test +describe('disableNewFiberFeatures', () => { + var container; + var ReactFeatureFlags; - afterEach(() => { - ReactFeatureFlags.disableNewFiberFeatures = false; - }); + beforeEach(() => { + container = document.createElement('div'); + ReactFeatureFlags = require('ReactFeatureFlags'); + ReactFeatureFlags.disableNewFiberFeatures = true; + }); - 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([
], container)).toThrow(message, container); - }); + afterEach(() => { + ReactFeatureFlags = require('ReactFeatureFlags'); + ReactFeatureFlags.disableNewFiberFeatures = false; + }); - it('throws if something other than false, null, or an element is returned from render', () => { - function Render(props) { - return props.children; - } + 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([
], container)).toThrow(message, container); + }); - expect(() => ReactDOM.render(Hi, container)).toThrow(/You may have returned undefined/); - expect(() => ReactDOM.render({999}, container)).toThrow(/You may have returned undefined/); - expect(() => ReactDOM.render([
], container)).toThrow(/You may have returned undefined/); - }); + it('throws if something other than false, null, or an element is returned from render', () => { + function Render(props) { + return props.children; + } - it('still accepts arrays and iterables as host children', () => { - function Render(props) { - return
{props.children}
; - } - ReactDOM.render({['foo', 'bar']}, container); - expect(container.textContent).toEqual('foobar'); - ReactDOM.render({new Set(['foo', 'bar'])}, container); - expect(container.textContent).toEqual('foobar'); - }); - }); - } + expect(() => ReactDOM.render(Hi, container)).toThrow(/You may have returned undefined/); + expect(() => ReactDOM.render({999}, container)).toThrow(/You may have returned undefined/); + expect(() => ReactDOM.render([
], container)).toThrow(/You may have returned undefined/); + }); }); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 0a3ca1d4ec1..8b0dcd1ade7 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -75,7 +75,6 @@ const { NoEffect, Placement, Deletion, - Err, } = ReactTypeOfSideEffect; function coerceRef(current: ?Fiber, element: ReactElement) { @@ -1127,41 +1126,40 @@ 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; - } + 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; } } - // 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' + ); + } + } + + if (typeof newChild === 'string' || typeof newChild === 'number') { + return placeSingleChild(reconcileSingleTextNode( + returnFiber, + currentFirstChild, + '' + newChild, + priority + )); } if (isArray(newChild)) { diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index 232582b4078..4ef55100b7f 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/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', () => { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index 7c3783e7427..d174efd2212 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/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', () => { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index b3ef77c2793..bef5a4b7b24 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/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) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js index 6d4ff9d1ec8..470d0e278c8 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalReflection-test.js +++ b/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', () => { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js index 9a12abe4aa1..c856f263805 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js +++ b/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) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js index f8b46fa1866..15b51610e8a 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js +++ b/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) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js index 73f97d3db1a..9953c8db81c 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalUpdates-test.js +++ b/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', () => { diff --git a/src/renderers/shared/fiber/__tests__/ReactTopLevelFragment-test.js b/src/renderers/shared/fiber/__tests__/ReactTopLevelFragment-test.js index eaff6444762..04004e46a21 100644 --- a/src/renderers/shared/fiber/__tests__/ReactTopLevelFragment-test.js +++ b/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() { diff --git a/src/renderers/shared/fiber/__tests__/ReactTopLevelText-test.js b/src/renderers/shared/fiber/__tests__/ReactTopLevelText-test.js index 63677123f30..84a98da7301 100644 --- a/src/renderers/shared/fiber/__tests__/ReactTopLevelText-test.js +++ b/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', () => { diff --git a/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js b/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js index 69e74fd211b..ac2c884ec9b 100644 --- a/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js +++ b/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; } }); diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index b31706c66b5..d02ef0b28cd 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -152,13 +152,8 @@ describe('ReactStatelessComponent', () => { expect(function() { ReactTestUtils.renderIntoDocument(
); }).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.' ); }); diff --git a/src/renderers/testing/__tests__/ReactTestRenderer-test.js b/src/renderers/testing/__tests__/ReactTestRenderer-test.js index 584453d0861..681d48ac022 100644 --- a/src/renderers/testing/__tests__/ReactTestRenderer-test.js +++ b/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 **)'); } From 645ad6d261c0c02ab9b383ea1b73185550cfd4ae Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 17 Jan 2017 11:48:03 -0800 Subject: [PATCH 5/6] Add test for mocked render functions Treat them as if they return null --- scripts/fiber/tests-passing.txt | 1 + src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js | 7 +++++++ src/renderers/shared/fiber/ReactChildFiber.js | 4 ++-- 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 050dd41e1b5..d6733db76ea 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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` diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index a4238218c3c..e5873643bd9 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1131,4 +1131,11 @@ describe('disableNewFiberFeatures', () => { expect(() => ReactDOM.render({999}, container)).toThrow(/You may have returned undefined/); expect(() => ReactDOM.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(, container); + expect(container.textContent).toEqual(''); + }); }); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 8b0dcd1ade7..f37d6fdc2c5 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -1130,7 +1130,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { case ClassComponent: { if (__DEV__) { const instance = returnFiber.stateNode; - if (instance.render._isMockFunction) { + if (instance.render._isMockFunction && typeof newChild === 'undefined') { // We allow auto-mocks to proceed as if they're // returning null. break; @@ -1254,7 +1254,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { case ClassComponent: { if (__DEV__) { const instance = returnFiber.stateNode; - if (instance.render._isMockFunction) { + if (instance.render._isMockFunction && typeof newChild === 'undefined') { // We allow auto-mocks to proceed as if they're returning null. break; } From ba1d3829bcb19b5b015a212d4379bdaed5c48dfc Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 25 Jan 2017 11:52:30 -0800 Subject: [PATCH 6/6] Use multiple checks instead of forking the entire function Easier to follow this way, and less chance the two paths will get out of sync. --- src/renderers/shared/fiber/ReactChildFiber.js | 151 ++++++++---------- 1 file changed, 63 insertions(+), 88 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index f37d6fdc2c5..95cea880cf9 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -64,7 +64,6 @@ const { FunctionalComponent, ClassComponent, HostText, - HostRoot, HostPortal, CoroutineComponent, YieldComponent, @@ -1103,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( @@ -1116,6 +1118,40 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { priority )); + case REACT_PORTAL_TYPE: + return placeSingleChild(reconcileSinglePortal( + returnFiber, + currentFirstChild, + newChild, + priority + )); + } + } else { + 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, @@ -1125,7 +1161,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { )); } } + } + 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__) { @@ -1152,35 +1192,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { ); } } - - if (typeof newChild === 'string' || typeof newChild === 'number') { - return placeSingleChild(reconcileSingleTextNode( - returnFiber, - currentFirstChild, - '' + newChild, - priority - )); - } - - if (isArray(newChild)) { - return reconcileChildrenArray( - returnFiber, - currentFirstChild, - newChild, - priority - ); - } - - if (getIteratorFn(newChild)) { - return reconcileChildrenIterator( - returnFiber, - currentFirstChild, - newChild, - priority - ); - } - - return deleteRemainingChildren(returnFiber, currentFirstChild); } if (typeof newChild === 'string' || typeof newChild === 'number') { @@ -1192,69 +1203,33 @@ 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; - if (instance.render._isMockFunction && typeof newChild === 'undefined') { + if (instance.render._isMockFunction) { // We allow auto-mocks to proceed as if they're returning null. break; }