From da3dc0354dc37af431f59ac04a29e694515511cd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:14:07 +0000 Subject: [PATCH 1/6] 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 8577ca180e6..3a64a26d9cb 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -134,6 +134,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 243d24542354eae55a77ce68ea556e04a13967be Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:22:34 +0000 Subject: [PATCH 2/6] 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 3a64a26d9cb..2dd8158ec7e 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -271,18 +271,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 2646272e8ac55a15bbfde0dc49b0e5294b7cd5fd Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:31:15 +0000 Subject: [PATCH 3/6] 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 2dd8158ec7e..76cd926fe71 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -119,30 +119,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 8fdaec8c1ba..9afbe90a2f4 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -44,26 +44,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); } @@ -210,7 +193,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 8c4b2a64dbb848bd6659821b0c3af771f283e03c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:46:23 +0000 Subject: [PATCH 4/6] 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 76cd926fe71..f233b2641da 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}
; } @@ -119,17 +121,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 297d89b6dee9d74c12d50cc77087a08e94425495 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:51:49 +0000 Subject: [PATCH 5/6] Record Fiber tests --- scripts/fiber/tests-failing.txt | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index ebf46e20be2..b54f0620af9 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -73,9 +73,8 @@ 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 throw when stateless component returns undefined * should throw on string refs in pure functions -* should warn when using non-React functions in JSX src/renderers/shared/shared/__tests__/ReactUpdates-test.js * marks top-level updates From 5643a2c6272ca27cff5c4b739638cce266ee1dc6 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Wed, 28 Dec 2016 00:09:06 +0000 Subject: [PATCH 6/6] Throw on undefined component output I changed the message to remove the wording about arrays since they're allowed in Fiber. I kept component name only in DEV since it's mostly useless with functions and classes in prod anyway when they are mangled. Alternatively we could change getComponentName() to work in DEV. I didn't want to copy and paste that snippet since, whether we include it or not, we should do it consistently in all warnings and invariants. I had to fix up a few tests that forgot to return null. --- scripts/fiber/tests-failing.txt | 4 - scripts/fiber/tests-passing.txt | 2 + .../shared/fiber/ReactFiberBeginWork.js | 73 ++++++++++++++++++- .../fiber/__tests__/ReactIncremental-test.js | 1 + .../ReactIncrementalErrorHandling-test.js | 1 + .../__tests__/ReactEmptyComponent-test.js | 14 +++- .../__tests__/ReactStatelessComponent-test.js | 10 ++- 7 files changed, 93 insertions(+), 12 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index b54f0620af9..b252bd53cea 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -66,14 +66,10 @@ src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js * should update refs if shouldComponentUpdate gives false -src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js -* should still throw when rendering to undefined - src/renderers/shared/shared/__tests__/ReactMultiChildText-test.js * should reorder keyed text nodes src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js -* should throw when stateless component returns undefined * should throw on string refs in pure functions src/renderers/shared/shared/__tests__/ReactUpdates-test.js diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index cb3f467467f..3a8e25e8e9f 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1400,6 +1400,7 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js * should not produce child DOM nodes for null and false +* should still throw when rendering to undefined * should be able to switch between rendering null and a normal tag * should be able to switch in a list of children * should distinguish between a script placeholder and an actual script tag @@ -1526,6 +1527,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 provide a null ref * should use correct name in key warning * should support default props and prop types diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 4cfb1f4bb1b..063abc8791a 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -19,12 +19,19 @@ import type { FiberRoot } from 'ReactFiberRoot'; import type { HostConfig } from 'ReactFiberReconciler'; import type { PriorityLevel } from 'ReactPriorityLevel'; +var { + isValidElement, +} = require('React'); var { mountChildFibersInPlace, reconcileChildFibers, reconcileChildFibersInPlace, cloneChildFibers, } = require('ReactChildFiber'); +var { + isCoroutine, + isYield, +} = require('ReactCoroutine'); var { beginUpdateQueue, } = require('ReactFiberUpdateQueue'); @@ -49,6 +56,9 @@ var { YieldComponent, Fragment, } = ReactTypeOfWork; +var { + isPortal, +} = require('ReactPortal'); var { NoWork, OffscreenPriority, @@ -62,6 +72,8 @@ var { var ReactCurrentOwner = require('ReactCurrentOwner'); var ReactFiberClassComponent = require('ReactFiberClassComponent'); +var invariant = require('invariant'); + if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); } @@ -88,6 +100,46 @@ module.exports = function( updateClassInstance, } = ReactFiberClassComponent(scheduleUpdate, getPriorityContext); + function isValidNode(node) { + switch (typeof node) { + case 'string': + case 'number': + return true; + case 'boolean': + return node === false; + case 'object': + return ( + node === null || + Array.isArray(node) || + isValidElement(node) || + isPortal(node) || + isCoroutine(node) || + isYield(node) + ); + case 'undefined': + return false; + default: + return false; + } + } + + function throwInvalidNodeError() { + // TODO: report the node type and/or object keys. + var info = ''; + if (__DEV__) { + var name = ReactDebugCurrentFiber.getCurrentFiberOwnerName(); + if (name) { + info += ' Check the render method of `' + name + '`.'; + } + } + invariant( + false, + 'A valid React element, null, or an array must be returned. ' + + 'You may have returned undefined or some other invalid object.%s', + info + ); + } + function markChildAsProgressed(current, workInProgress, priorityLevel) { // We now have clones. Let's store them as the currently progressed work. workInProgress.progressedChild = workInProgress.child; @@ -211,6 +263,9 @@ module.exports = function( } else { nextChildren = fn(nextProps, context); } + if (!isValidNode(nextChildren)) { + throwInvalidNodeError(); + } reconcileChildren(current, workInProgress, nextChildren); return workInProgress.child; } @@ -263,7 +318,19 @@ module.exports = function( // Rerender const instance = workInProgress.stateNode; ReactCurrentOwner.current = workInProgress; - const nextChildren = instance.render(); + let nextChildren = instance.render(); + if (__DEV__) { + // We allow auto-mocks to proceed as if they're returning null. + if (nextChildren === undefined && + instance.render._isMockFunction) { + // This is probably bad practice. Consider warning here and + // deprecating this convenience. + nextChildren = null; + } + } + if (!isValidNode(nextChildren)) { + throwInvalidNodeError(); + } reconcileChildren(current, workInProgress, nextChildren); // The context might have changed so we need to recalculate it. @@ -433,11 +500,13 @@ module.exports = function( adoptClassInstance(workInProgress, value); mountClassInstance(workInProgress, priorityLevel); return finishClassComponent(current, workInProgress, true, hasContext); - } else { + } else if (isValidNode(value)) { // Proceed under the assumption that this is a functional component workInProgress.tag = FunctionalComponent; reconcileChildren(current, workInProgress, value); return workInProgress.child; + } else { + throwInvalidNodeError(); } } diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index defd5e672b2..3da660dbbfd 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 c22d644fddd..9fa6394864d 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__/ReactEmptyComponent-test.js b/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js index 69e74fd211b..dc504804298 100644 --- a/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactEmptyComponent-test.js @@ -72,15 +72,21 @@ describe('ReactEmptyComponent', () => { }); it('should still throw when rendering to undefined', () => { - class Component extends React.Component { + class MyComponent extends React.Component { render() {} } expect(function() { - ReactTestUtils.renderIntoDocument(); + ReactTestUtils.renderIntoDocument(); }).toThrowError( - 'Component.render(): A valid React element (or null) must be returned. You may ' + - 'have returned undefined, an array or some other invalid object.' + ReactDOMFeatureFlags.useFiber ? ( + 'A valid React element, null, or an array must be returned. ' + + 'You may have returned undefined or some other invalid object. ' + + 'Check the render method of `MyComponent`.' + ) : ( + 'MyComponent.render(): 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/shared/__tests__/ReactStatelessComponent-test.js b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js index f233b2641da..2e54a106e06 100644 --- a/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js @@ -142,8 +142,14 @@ 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 ? ( + 'A valid React element, null, or an array must be returned. ' + + 'You may have returned undefined or some other invalid object. ' + + 'Check the render method of `NotAComponent`.' + ) : ( + 'NotAComponent(...): A valid React element (or null) must be returned. You may ' + + 'have returned undefined, an array or some other invalid object.' + ) ); });