From 931bc7dc89a65057d8c2443258b69ba7a8696f0a Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Sat, 2 Jun 2018 02:18:50 +0200 Subject: [PATCH 1/4] =?UTF-8?q?Don=E2=80=99t=20error=20when=20returning=20?= =?UTF-8?q?an=20empty=20Fragment?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a fragment is reconciled, we directly move onto it’s children. Since an empty `` will have children of `undefined`, this would always throw. To fix this, we bail out in those cases. --- .../react-dom/src/__tests__/ReactDOMFiber-test.js | 12 ++++++++++++ packages/react-reconciler/src/ReactChildFiber.js | 6 ++++++ 2 files changed, 18 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index a93dda6ce11e..d38ee7e68f61 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -171,6 +171,18 @@ describe('ReactDOMFiber', () => { expect(firstNode.tagName).toBe('DIV'); }); + it('renders an empty fragment', () => { + const EmptyFragment = () => ; + + let instance = null; + ReactDOM.render(, container); + + expect(container.childNodes.length).toBe(0); + + const firstNode = ReactDOM.findDOMNode(instance); + expect(firstNode).toBe(null); + }); + let svgEls, htmlEls, mathEls; const expectSVG = {ref: el => svgEls.push(el)}; const expectHTML = {ref: el => htmlEls.push(el)}; diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index ae19577aa29c..88cbd66b5968 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1215,6 +1215,12 @@ function ChildReconciler(shouldTrackSideEffects) { newChild.key === null ) { newChild = newChild.props.children; + + // In case of an empty fragment, this is undefined. Since we crash when + // undefined is reconciled, we bail out early. + if (typeof newChild === 'undefined') { + return deleteRemainingChildren(returnFiber, currentFirstChild); + } } // Handle object types From 4de25398a04f6ec1f1f01d42caf5fa2ad9315bcc Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Sat, 2 Jun 2018 10:00:33 +0200 Subject: [PATCH 2/4] Test the update path as well --- .../src/__tests__/ReactDOMFiber-test.js | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index d38ee7e68f61..911f161b0e66 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -172,15 +172,28 @@ describe('ReactDOMFiber', () => { }); it('renders an empty fragment', () => { + const Div = () =>
; const EmptyFragment = () => ; + const NonEmptyFragment = () => ( + +
+ + ); - let instance = null; ReactDOM.render(, container); + expect(container.firstChild).toBe(null); - expect(container.childNodes.length).toBe(0); + ReactDOM.render(, container); + expect(container.firstChild.tagName).toBe('DIV'); - const firstNode = ReactDOM.findDOMNode(instance); - expect(firstNode).toBe(null); + ReactDOM.render(, container); + expect(container.firstChild).toBe(null); + + ReactDOM.render(
, container); + expect(container.firstChild.tagName).toBe('DIV'); + + ReactDOM.render(, container); + expect(container.firstChild).toBe(null); }); let svgEls, htmlEls, mathEls; From 697bd71d3e4faf9fc1c48f06e94178b0518eb2bb Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Jun 2018 14:24:37 +0100 Subject: [PATCH 3/4] Reuse existing code path --- packages/react-reconciler/src/ReactChildFiber.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 88cbd66b5968..57ae2fbf5da7 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1215,11 +1215,9 @@ function ChildReconciler(shouldTrackSideEffects) { newChild.key === null ) { newChild = newChild.props.children; - - // In case of an empty fragment, this is undefined. Since we crash when - // undefined is reconciled, we bail out early. - if (typeof newChild === 'undefined') { - return deleteRemainingChildren(returnFiber, currentFirstChild); + // Allow empty without an invariant about top-level undefined. + if (newChild === undefined) { + newChild = null; } } From 6d8c3f50f48353137d877dc28f1be02aca0e7e4e Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Jun 2018 14:33:30 +0100 Subject: [PATCH 4/4] An even more explicit solution that also fixes Flow --- packages/react-reconciler/src/ReactChildFiber.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 57ae2fbf5da7..e3d1dd72ed99 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -1208,17 +1208,13 @@ function ChildReconciler(shouldTrackSideEffects) { // Handle top level unkeyed fragments as if they were arrays. // This leads to an ambiguity between <>{[...]} and <>.... // We treat the ambiguous cases above the same. - if ( + const isUnkeyedTopLevelFragment = typeof newChild === 'object' && newChild !== null && newChild.type === REACT_FRAGMENT_TYPE && - newChild.key === null - ) { + newChild.key === null; + if (isUnkeyedTopLevelFragment) { newChild = newChild.props.children; - // Allow empty without an invariant about top-level undefined. - if (newChild === undefined) { - newChild = null; - } } // Handle object types @@ -1285,7 +1281,7 @@ function ChildReconciler(shouldTrackSideEffects) { warnOnFunctionType(); } } - if (typeof newChild === 'undefined') { + if (typeof newChild === 'undefined' && !isUnkeyedTopLevelFragment) { // 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.