diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index ebf46e20be2..b252bd53cea 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -66,16 +66,11 @@ 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 warn when stateless component returns array * 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 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 8577ca180e6..2e54a106e06 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,18 +121,35 @@ describe('ReactStatelessComponent', () => { ); }); - it('should warn when stateless component returns array', () => { - spyOn(console, 'error'); + 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() { - return [
,
]; } 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.' + }).toThrowError( + 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.' + ) ); }); @@ -257,18 +276,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.' - ); - }); }); 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 ||