From fdf0f435c7fab25194eda86ea78d058543862897 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:14:07 +0000 Subject: [PATCH 01/11] Add a test for stateless components returning undefined We have one for arrays but it is going to become Stack-specific. --- .../__tests__/ReactStatelessComponent-test.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 060ec461373..8b3728b1981 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -138,6 +138,20 @@ describe('ReactStatelessComponent', () => { ); }); + it('should warn when stateless component returns undefined', () => { + spyOn(console, 'error'); + function NotAComponent() { + } + expect(function() { + ReactTestUtils.renderIntoDocument(
); + }).toThrow(); + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'NotAComponent(...): A valid React element (or null) must be returned. ' + + 'You may have returned undefined, an array or some other invalid object.' + ); + }); + it('should throw on string refs in pure functions', () => { function Child() { return
; From 33c3121f43bfbfa18d6ab0b98f3d447b713023a8 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:22:34 +0000 Subject: [PATCH 02/11] Remove a duplicate test This test is identical to "should warn when stateless component returns array" earlier. It was moved from another file in #5884 so it likely survived by accident. --- .../__tests__/ReactStatelessComponent-test.js | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 8b3728b1981..5525b7178e3 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -388,18 +388,4 @@ describe('ReactStatelessComponent', () => { expect(() => ReactTestUtils.renderIntoDocument()).not.toThrow(); }); - it('should warn when using non-React functions in JSX', () => { - spyOn(console, 'error'); - function NotAComponent() { - return [
,
]; - } - expect(function() { - ReactTestUtils.renderIntoDocument(
); - }).toThrow(); // has no method 'render' - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( - 'NotAComponent(...): A valid React element (or null) must be returned. You may ' + - 'have returned undefined, an array or some other invalid object.' - ); - }); }); From df0532b0c38fc3579a9993ea58abd5ed33cd16a6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:31:15 +0000 Subject: [PATCH 03/11] Remove unnecessary warning for invalid node We have an invariant that checks the same case right afterwards. The warning was originally added in #5884 with a distinct wording. However it was later changed to the same wording as the invariant in #6008. I don't see why we would want to have both since they're saying the same thing and with (almost) the same internal stack. --- .../__tests__/ReactStatelessComponent-test.js | 14 +++-------- .../reconciler/ReactCompositeComponent.js | 25 ++++++------------- 2 files changed, 11 insertions(+), 28 deletions(-) diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index 5525b7178e3..caea83e35ea 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -123,30 +123,24 @@ describe('ReactStatelessComponent', () => { ); }); - it('should warn when stateless component returns array', () => { - spyOn(console, 'error'); + it('should throw when stateless component returns array', () => { function NotAComponent() { return [
,
]; } expect(function() { ReactTestUtils.renderIntoDocument(
); - }).toThrow(); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( + }).toThrowError( 'NotAComponent(...): A valid React element (or null) must be returned. ' + 'You may have returned undefined, an array or some other invalid object.' ); }); - it('should warn when stateless component returns undefined', () => { - spyOn(console, 'error'); + it('should throw when stateless component returns undefined', () => { function NotAComponent() { } expect(function() { ReactTestUtils.renderIntoDocument(
); - }).toThrow(); - expectDev(console.error.calls.count()).toBe(1); - expectDev(console.error.calls.argsFor(0)[0]).toContain( + }).toThrowError( '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/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index 1abe88d6224..ecdccd09824 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -39,26 +39,9 @@ function StatelessComponent(Component) { StatelessComponent.prototype.render = function() { var Component = ReactInstanceMap.get(this)._currentElement.type; var element = Component(this.props, this.context, this.updater); - warnIfInvalidElement(Component, element); return element; }; -function warnIfInvalidElement(Component, element) { - if (__DEV__) { - warning( - element === null || element === false || React.isValidElement(element), - '%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' - ); - warning( - !Component.childContextTypes, - '%s(...): childContextTypes cannot be defined on a functional component.', - Component.displayName || Component.name || 'Component' - ); - } -} - function shouldConstruct(Component) { return !!(Component.prototype && Component.prototype.isReactComponent); } @@ -205,7 +188,13 @@ var ReactCompositeComponent = { // Support functional components if (!doConstruct && (inst == null || inst.render == null)) { renderedElement = inst; - warnIfInvalidElement(Component, renderedElement); + if (__DEV__) { + warning( + !Component.childContextTypes, + '%s(...): childContextTypes cannot be defined on a functional component.', + Component.displayName || Component.name || 'Component' + ); + } invariant( inst === null || inst === false || From 3ec1779837c4bccbcfb7239b80abc45edd2c7134 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:46:23 +0000 Subject: [PATCH 04/11] Make test that forbids arrays Stack-only --- .../__tests__/ReactStatelessComponent-test.js | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index caea83e35ea..a095ee0ca46 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -15,6 +15,8 @@ var React; var ReactDOM; var ReactTestUtils; +var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); + function StatelessComponent(props) { return
{props.name}
; } @@ -123,17 +125,20 @@ describe('ReactStatelessComponent', () => { ); }); - it('should throw when stateless component returns array', () => { - function NotAComponent() { - return [
,
]; - } - expect(function() { - ReactTestUtils.renderIntoDocument(
); - }).toThrowError( - 'NotAComponent(...): A valid React element (or null) must be returned. ' + - 'You may have returned undefined, an array or some other invalid object.' - ); - }); + if (!ReactDOMFeatureFlags.useFiber) { + // Stack doesn't support fragments + it('should throw when stateless component returns array', () => { + function NotAComponent() { + return [
,
]; + } + expect(function() { + ReactTestUtils.renderIntoDocument(
); + }).toThrowError( + 'NotAComponent(...): A valid React element (or null) must be returned. ' + + 'You may have returned undefined, an array or some other invalid object.' + ); + }); + } it('should throw when stateless component returns undefined', () => { function NotAComponent() { From bf117f82e1acb4e20a0fd844f3494233cb1fb907 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 17:41:13 -0800 Subject: [PATCH 05/11] Add feature flag to disable Fiber-specific features We'll enable this in www in case we need to roll back to Stack. --- src/renderers/shared/utils/ReactFeatureFlags.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/renderers/shared/utils/ReactFeatureFlags.js b/src/renderers/shared/utils/ReactFeatureFlags.js index 5bd9651aed1..ce0a442c11b 100644 --- a/src/renderers/shared/utils/ReactFeatureFlags.js +++ b/src/renderers/shared/utils/ReactFeatureFlags.js @@ -18,6 +18,7 @@ var ReactFeatureFlags = { // timeline profiles in Chrome, for example. logTopLevelRenders: false, prepareNewChildrenBeforeUnmountInStack: true, + disableNewFiberFeatures: false, }; module.exports = ReactFeatureFlags; From 86280187d7b9a0708e9268d4a17aa7c32f414da0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 17:49:26 -0800 Subject: [PATCH 06/11] Disallow Fiber-only render return types when feature flag is on Except for portals, which we use in some places. --- .../dom/fiber/__tests__/ReactDOMFiber-test.js | 28 +++++++++++++++ src/renderers/shared/fiber/ReactChildFiber.js | 34 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 9abec3eedd1..21131897a53 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1002,5 +1002,33 @@ describe('ReactDOMFiber', () => { container ); }); + + describe('disableNewFiberFeatures', () => { + var ReactFeatureFlags = require('ReactFeatureFlags'); + + beforeEach(() => { + ReactFeatureFlags.disableNewFiberFeatures = true; + }); + + afterEach(() => { + ReactFeatureFlags.disableNewFiberFeatures = false; + }); + + it('throws if something other than false, null, or an element is returned from render', () => { + function Render(props) { + return props.children; + } + + const message = ( + 'Warning: Render.render(): A valid React element (or null) must ' + + 'be returned. You may have returned undefined, an array or some ' + + 'other invalid object.' + ); + + expect(() => ReactDOM.render(Hi, container)).toThrow(message); + expect(() => ReactDOM.render({999}, container)).toThrow(message); + expect(() => ReactDOM.render([
], container)).toThrow(message); + }); + }); } }); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 9302c2ed809..115134e6b90 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -36,6 +36,7 @@ var ReactTypeOfWork = require('ReactTypeOfWork'); var emptyObject = require('emptyObject'); var getIteratorFn = require('getIteratorFn'); var invariant = require('invariant'); +var ReactFeatureFlags = require('ReactFeatureFlags'); if (__DEV__) { var { getCurrentFiberStackAddendum } = require('ReactDebugCurrentFiber'); @@ -1100,6 +1101,39 @@ 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) { + // 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) { + 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 + )); + } + } + + 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' + ); + return deleteRemainingChildren(returnFiber, currentFirstChild); + } + if (typeof newChild === 'string' || typeof newChild === 'number') { return placeSingleChild(reconcileSingleTextNode( returnFiber, From 204d3dd703f2ef21953e1509302456f3800a07db Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 17:51:27 -0800 Subject: [PATCH 07/11] Throw if undefined is returned from a composite component --- src/renderers/shared/fiber/ReactChildFiber.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 115134e6b90..1a8fc6a4599 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -61,8 +61,10 @@ const { const isArray = Array.isArray; const { + FunctionalComponent, ClassComponent, HostText, + HostRoot, HostPortal, CoroutineComponent, YieldComponent, @@ -1197,6 +1199,25 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } } + if (typeof newChild === 'undefined') { + switch (returnFiber.tag) { + case HostRoot: + // TODO: Top-level render + break; + case ClassComponent: + case FunctionalComponent: { + const Component = returnFiber.type; + invariant( + false, + '%s: Nothing was returned from render. This usually means a ' + + 'return statement is missing. Or, to render nothing, ' + + 'return null.', + Component.displayName || Component.name || 'Component' + ); + } + } + } + // Remaining cases are all treated as empty. return deleteRemainingChildren(returnFiber, currentFirstChild); } From 80cf21cae49d370cd8df07c9c354c251c924376a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 18:06:16 -0800 Subject: [PATCH 08/11] In DEV, allow treat auto-mocked render methods as if they return null Consider deprecating this in the future. --- src/renderers/shared/fiber/ReactChildFiber.js | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 1a8fc6a4599..6fab354c4eb 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -1127,8 +1127,19 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } const Component = returnFiber.type; + let validEmptyReturnType = newChild === null || newChild === false; + + if (__DEV__) { + if (!validEmptyReturnType && + returnFiber.tag === ClassComponent && + returnFiber.stateNode.render._isMockFunction) { + // We allow auto-mocks to proceed as if they're returning null. + validEmptyReturnType = true; + } + } + invariant( - newChild === null || newChild === false, + validEmptyReturnType, '%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' @@ -1204,7 +1215,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { case HostRoot: // TODO: Top-level render break; - case ClassComponent: + 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: { const Component = returnFiber.type; invariant( From 397810f844661288b22de4cf645999195993d51e Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 18:08:16 -0800 Subject: [PATCH 09/11] Top-level render only accepts a React element if feature flag is on --- .../dom/fiber/__tests__/ReactDOMFiber-test.js | 23 +++++++++++-------- .../shared/fiber/ReactFiberReconciler.js | 9 ++++++++ 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 21131897a53..61efde7c9ba 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1014,20 +1014,25 @@ describe('ReactDOMFiber', () => { 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([
], 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; } - const message = ( - 'Warning: Render.render(): A valid React element (or null) must ' + - 'be returned. You may have returned undefined, an array or some ' + - 'other invalid object.' - ); - - expect(() => ReactDOM.render(Hi, container)).toThrow(message); - expect(() => ReactDOM.render({999}, container)).toThrow(message); - expect(() => ReactDOM.render([
], container)).toThrow(message); + expect(() => ReactDOM.render(Hi, container)).toThrow(/Render\.render/); + expect(() => ReactDOM.render({999}, container)).toThrow(/Render\.render/); + expect(() => ReactDOM.render([
], container)).toThrow(/Render\.render/); }); }); } diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index 52cc90f57d3..d74d16226fc 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -17,6 +17,10 @@ import type { FiberRoot } from 'ReactFiberRoot'; import type { PriorityLevel } from 'ReactPriorityLevel'; import type { ReactNodeList } from 'ReactTypes'; +var { isValidElement } = require('ReactElement'); +var invariant = require('invariant'); +var ReactFeatureFlags = require('ReactFeatureFlags'); + var { addTopLevelUpdate, } = require('ReactFiberUpdateQueue'); @@ -134,6 +138,11 @@ module.exports = function(config : HostConfig, callback: ?Function) : void { + invariant( + !ReactFeatureFlags.disableNewFiberFeatures || isValidElement(element), + 'render(): Invalid component element.' + ); + // TODO: If this is a nested container, this won't be the root. const root : FiberRoot = (container.stateNode : any); const current = root.current; From c8adedc05499a403781668b82ea758c4eb36b374 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Jan 2017 18:24:47 -0800 Subject: [PATCH 10/11] Warn if undefined is passed to top-level render Moved the top-level check to ReactChildFiber like the other ones --- src/renderers/shared/fiber/ReactChildFiber.js | 48 ++++++++++++------- .../shared/fiber/ReactFiberReconciler.js | 9 ---- 2 files changed, 32 insertions(+), 25 deletions(-) diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index 6fab354c4eb..acaaff87250 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -1126,24 +1126,40 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { } } - const Component = returnFiber.type; - let validEmptyReturnType = newChild === null || newChild === false; - - if (__DEV__) { - if (!validEmptyReturnType && - returnFiber.tag === ClassComponent && - returnFiber.stateNode.render._isMockFunction) { - // We allow auto-mocks to proceed as if they're returning null. - validEmptyReturnType = true; + if (returnFiber.tag === HostRoot) { + // Top-level only accepts elements or portals + invariant( + false, + '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; + } + } + } + // 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' + ); + } } } - - invariant( - validEmptyReturnType, - '%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' - ); return deleteRemainingChildren(returnFiber, currentFirstChild); } diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index d74d16226fc..52cc90f57d3 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -17,10 +17,6 @@ import type { FiberRoot } from 'ReactFiberRoot'; import type { PriorityLevel } from 'ReactPriorityLevel'; import type { ReactNodeList } from 'ReactTypes'; -var { isValidElement } = require('ReactElement'); -var invariant = require('invariant'); -var ReactFeatureFlags = require('ReactFeatureFlags'); - var { addTopLevelUpdate, } = require('ReactFiberUpdateQueue'); @@ -138,11 +134,6 @@ module.exports = function(config : HostConfig, callback: ?Function) : void { - invariant( - !ReactFeatureFlags.disableNewFiberFeatures || isValidElement(element), - 'render(): Invalid component element.' - ); - // TODO: If this is a nested container, this won't be the root. const root : FiberRoot = (container.stateNode : any); const current = root.current; From 9459bad9128e35a13aea82bbae4f206c4498bb58 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 12 Jan 2017 10:50:50 -0800 Subject: [PATCH 11/11] Run test script and fix regressions Fixes the case where there's an uncaught error and the root unmounts. We implement this by rendering the root as if its child is null. Null is not usually allowed at the top level, so we need to special case it. --- scripts/fiber/tests-failing.txt | 4 ---- scripts/fiber/tests-passing.txt | 3 +++ .../dom/fiber/__tests__/ReactDOMFiber-test.js | 10 ++++++---- src/renderers/shared/fiber/ReactChildFiber.js | 7 +++++-- .../shared/fiber/__tests__/ReactIncremental-test.js | 1 + .../__tests__/ReactIncrementalErrorHandling-test.js | 1 + .../shared/__tests__/ReactStatelessComponent-test.js | 9 +++++++-- 7 files changed, 23 insertions(+), 12 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 36f8480baf3..2316f076e13 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -64,10 +64,6 @@ src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js * should reorder keyed text nodes -src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js -* should warn when stateless component returns array -* should warn when using non-React functions in JSX - src/renderers/shared/shared/__tests__/ReactUpdates-test.js * marks top-level updates diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 18c4945ca2a..0c7d4082c56 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -544,6 +544,8 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js * should bubble events from the portal to the parent * should not onMouseLeave when staying in the portal * 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 src/renderers/dom/shared/__tests__/CSSProperty-test.js * should generate browser prefixes for its `isUnitlessNumber` @@ -1536,6 +1538,7 @@ src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js * should update stateless component * should unmount stateless component * should pass context thru stateless component +* should throw when stateless component returns undefined * should throw on string refs in pure functions * should warn when given a string ref * should warn when given a function ref diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 61efde7c9ba..ea7f39da297 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -1015,8 +1015,10 @@ 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); expect(() => ReactDOM.render(false, container)).toThrow(message, container); @@ -1030,9 +1032,9 @@ describe('ReactDOMFiber', () => { return props.children; } - expect(() => ReactDOM.render(Hi, container)).toThrow(/Render\.render/); - expect(() => ReactDOM.render({999}, container)).toThrow(/Render\.render/); - expect(() => ReactDOM.render([
], container)).toThrow(/Render\.render/); + 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 acaaff87250..c741d94bddf 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -75,6 +75,7 @@ const { NoEffect, Placement, Deletion, + Err, } = ReactTypeOfSideEffect; function coerceRef(current: ?Fiber, element: ReactElement) { @@ -1129,7 +1130,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { if (returnFiber.tag === HostRoot) { // Top-level only accepts elements or portals invariant( - false, + // 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 { @@ -1247,7 +1250,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { const Component = returnFiber.type; invariant( false, - '%s: Nothing was returned from render. This usually means a ' + + '%s(...): Nothing was returned from render. This usually means a ' + 'return statement is missing. Or, to render nothing, ' + 'return null.', Component.displayName || Component.name || 'Component' diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index e356056af4c..cb0b5e24720 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -956,6 +956,7 @@ describe('ReactIncremental', () => { function Trail() { ops.push('Trail'); + return null; } function App(props) { diff --git a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js index bee26adef8f..603ab8c7fd2 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js @@ -496,6 +496,7 @@ describe('ReactIncrementalErrorHandling', () => { if (props.throw) { throw new Error('Hello'); } + return null; } function Foo() { diff --git a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index a095ee0ca46..876f2685548 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -146,8 +146,13 @@ describe('ReactStatelessComponent', () => { expect(function() { ReactTestUtils.renderIntoDocument(
); }).toThrowError( - 'NotAComponent(...): A valid React element (or null) must be returned. ' + - 'You may have returned undefined, an array or some other invalid object.' + 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.' ); });