From 9459bad9128e35a13aea82bbae4f206c4498bb58 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 12 Jan 2017 10:50:50 -0800 Subject: [PATCH] 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 36f8480baf..2316f076e1 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 18c4945ca2..0c7d4082c5 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 61efde7c9b..ea7f39da29 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 acaaff8725..c741d94bdd 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 e356056af4..cb0b5e2472 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 bee26adef8..603ab8c7fd 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 a095ee0ca4..876f268554 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.' ); });