Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don’t error when returning an empty Fragment #12966

Merged
merged 4 commits into from Jun 11, 2018

Conversation

philipp-spiess
Copy link
Contributor

Fixes #12964

When a fragment is reconciled, we directly move onto it’s children. Since an empty <React.Fragment/> will have children of undefined, this would always throw.

To fix this, we bail out in those cases. This case now behaves like returning null directly.

@@ -171,6 +171,18 @@ describe('ReactDOMFiber', () => {
expect(firstNode.tagName).toBe('DIV');
});

it('renders an empty fragment', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea if that is the right place for the test :)

@aweary
Copy link
Contributor

aweary commented Jun 2, 2018

@philipp-spiess there is already a check for newChild being undefined here:

if (typeof newChild === 'undefined') {

Seems like this would break that check, preventing the "Nothing was returned from render..." error from being thrown. We need a more explicit check for Fragments. We could probably exit early (return deleteRemainingChildren) and avoid the rest of the type checking too.

@philipp-spiess
Copy link
Contributor Author

@aweary Thanks for looking at this! I got interested by your bug report and had to look into that 🙈

My thought was that we could set newChild to null to explicitly avoid the undefined special handling later. Bailing out using deleteRemainingChildren sounds even better (somehow I overlooked that option).

I just think we have to make the test at the place I made it since we'd otherwise loose the information that newChild was a fragment (that is, if we don't introduce a new variable to store that).

When a fragment is reconciled, we directly move onto it’s children.
Since an empty `<React.Fragment/>` will have children of `undefined`,
this would always throw.

To fix this, we bail out in those cases.
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ll also need tests around the update path. Update from empty Fragment to non-empty Fragment and back. Update from empty to a div and back.


expect(container.childNodes.length).toBe(0);

const firstNode = ReactDOM.findDOMNode(instance);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just check container.firstChild?

@pull-bot
Copy link

ReactDOM: size: 0.0%, gzip: -0.0%

Details of bundled changes.

Comparing: 36546b5...6d8c3f5

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js 0.0% 0.0% 626.32 KB 626.41 KB 145.82 KB 145.84 KB UMD_DEV
react-dom.production.min.js 0.0% -0.0% 94.16 KB 94.17 KB 30.5 KB 30.5 KB UMD_PROD
react-dom.development.js 0.0% 0.0% 610.68 KB 610.78 KB 141.81 KB 141.83 KB NODE_DEV
react-dom.production.min.js 0.0% 0.0% 92.66 KB 92.67 KB 29.48 KB 29.48 KB NODE_PROD
ReactDOM-dev.js 0.0% 0.0% 620.2 KB 620.28 KB 141.21 KB 141.23 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.2% 🔺+0.1% 269.19 KB 269.71 KB 50.57 KB 50.61 KB FB_WWW_PROD

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js 0.0% 0.0% 407.9 KB 407.99 KB 91.04 KB 91.06 KB UMD_DEV
react-art.production.min.js 0.0% -0.0% 81.05 KB 81.06 KB 25 KB 24.99 KB UMD_PROD
react-art.development.js 0.0% 0.0% 333.75 KB 333.84 KB 72.13 KB 72.15 KB NODE_DEV
react-art.production.min.js 0.0% 0.0% 45.41 KB 45.42 KB 14.14 KB 14.14 KB NODE_PROD
ReactART-dev.js 0.0% 0.0% 326.38 KB 326.46 KB 68.06 KB 68.08 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.4% 🔺+0.2% 141.93 KB 142.46 KB 24.25 KB 24.3 KB FB_WWW_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js 0.0% 0.0% 457.79 KB 457.87 KB 100.06 KB 100.08 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.3% 🔺+0.1% 205.68 KB 206.21 KB 35.94 KB 35.98 KB RN_FB_PROD
ReactNativeRenderer-dev.js 0.0% 0.0% 457.45 KB 457.53 KB 99.99 KB 100.01 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.3% 🔺+0.1% 193.67 KB 194.19 KB 33.9 KB 33.96 KB RN_OSS_PROD
ReactFabric-dev.js 0.0% 0.0% 448.7 KB 448.79 KB 97.82 KB 97.84 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.3% 🔺+0.2% 185.92 KB 186.44 KB 32.54 KB 32.6 KB RN_FB_PROD
ReactFabric-dev.js 0.0% 0.0% 448.74 KB 448.83 KB 97.84 KB 97.86 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.3% 🔺+0.2% 185.95 KB 186.48 KB 32.56 KB 32.61 KB RN_OSS_PROD

Generated by 🚫 dangerJS

@gaearon gaearon merged commit d480782 into facebook:master Jun 11, 2018
@philipp-spiess philipp-spiess deleted the fix-empty-fragment branch June 11, 2018 13:44
bors bot added a commit to mozilla/delivery-console that referenced this pull request Jun 13, 2018
193: Update dependency react to v16.4.1 r=magopian a=renovate[bot]

This Pull Request updates dependency [react](https://github.com/facebook/react) from `v16.4.0` to `v16.4.1`



<details>
<summary>Release Notes</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
bors bot added a commit to mythmon/corsica-tree-status that referenced this pull request Jun 14, 2018
25: Update react monorepo to v16.4.1 r=renovate[bot] a=renovate[bot]

This Pull Request renovates the package group "react monorepo".


-   [react-dom](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`
-   [react](https://github.com/facebook/react) (`dependencies`): from `16.4.0` to `16.4.1`

# Release Notes
<details>
<summary>facebook/react</summary>

### [`v16.4.1`](https://github.com/facebook/react/blob/master/CHANGELOG.md#&#8203;1641-June-13-2018)
[Compare Source](facebook/react@v16.4.0...v16.4.1)
##### React

* You can now assign `propTypes` to components returned by `React.ForwardRef`. ([@&#8203;bvaughn] in [#&#8203;12911](`facebook/react#12911))
##### React DOM

* Fix a crash when the input `type` changes from some other types to `text`. ([@&#8203;spirosikmd] in [#&#8203;12135](`facebook/react#12135))
* Fix a crash in IE11 when restoring focus to an SVG element. ([@&#8203;ThaddeusJiang] in [#&#8203;12996](`facebook/react#12996))
* Fix a range input not updating in some cases. ([@&#8203;Illu] in [#&#8203;12939](`facebook/react#12939))
* Fix input validation triggering unnecessarily in Firefox. ([@&#8203;nhunzaker] in [#&#8203;12925](`facebook/react#12925))
* Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@&#8203;nhunzaker] in [#&#8203;12976](`facebook/react#12976))
* Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@&#8203;philipp-spiess] in [#&#8203;12966](`facebook/react#12966))
##### React DOM Server

* Fix an incorrect value being provided by new context API. ([@&#8203;ericsoderberghp] in [#&#8203;12985](`facebook/react#12985), [@&#8203;gaearon] in [#&#8203;13019](`facebook/react#13019))
##### React Test Renderer

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`facebook/react#13017))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;13030](`facebook/react#13030))

---


</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).

Co-authored-by: Renovate Bot <bot@renovateapp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Returning an empty fragment throws a confusing error
5 participants