-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
Conversation
@@ -171,6 +171,18 @@ describe('ReactDOMFiber', () => { | |||
expect(firstNode.tagName).toBe('DIV'); | |||
}); | |||
|
|||
it('renders an empty fragment', () => { |
There was a problem hiding this comment.
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 :)
@philipp-spiess there is already a check for newChild being
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 |
@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 I just think we have to make the test at the place I made it since we'd otherwise loose the information that |
4ac335b
to
4e78ff9
Compare
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.
4e78ff9
to
931bc7d
Compare
There was a problem hiding this 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); |
There was a problem hiding this comment.
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
?
ReactDOM: size: 0.0%, gzip: -0.0% Details of bundled changes.Comparing: 36546b5...6d8c3f5 react-dom
react-art
react-native-renderer
Generated by 🚫 dangerJS |
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#​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`. ([@​bvaughn] in [#​12911](`https://github.com/facebook/react/pull/12911`)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@​spirosikmd] in [#​12135](`https://github.com/facebook/react/pull/12135`)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@​ThaddeusJiang] in [#​12996](`https://github.com/facebook/react/pull/12996`)) * Fix a range input not updating in some cases. ([@​Illu] in [#​12939](`https://github.com/facebook/react/pull/12939`)) * Fix input validation triggering unnecessarily in Firefox. ([@​nhunzaker] in [#​12925](`https://github.com/facebook/react/pull/12925`)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@​nhunzaker] in [#​12976](`https://github.com/facebook/react/pull/12976`)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@​philipp-spiess] in [#​12966](`https://github.com/facebook/react/pull/12966`)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@​ericsoderberghp] in [#​12985](`https://github.com/facebook/react/pull/12985`), [@​gaearon] in [#​13019](`https://github.com/facebook/react/pull/13019`)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@​gaearon] in [#​13017](`https://github.com/facebook/react/pull/13017`)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@​fatfisz] in [#​13030](`https://github.com/facebook/react/pull/13030`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>
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#​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`. ([@​bvaughn] in [#​12911](`https://github.com/facebook/react/pull/12911`)) ##### React DOM * Fix a crash when the input `type` changes from some other types to `text`. ([@​spirosikmd] in [#​12135](`https://github.com/facebook/react/pull/12135`)) * Fix a crash in IE11 when restoring focus to an SVG element. ([@​ThaddeusJiang] in [#​12996](`https://github.com/facebook/react/pull/12996`)) * Fix a range input not updating in some cases. ([@​Illu] in [#​12939](`https://github.com/facebook/react/pull/12939`)) * Fix input validation triggering unnecessarily in Firefox. ([@​nhunzaker] in [#​12925](`https://github.com/facebook/react/pull/12925`)) * Fix an incorrect `event.target` value for the `onChange` event in IE9. ([@​nhunzaker] in [#​12976](`https://github.com/facebook/react/pull/12976`)) * Fix a false positive error when returning an empty `<React.Fragment />` from a component. ([@​philipp-spiess] in [#​12966](`https://github.com/facebook/react/pull/12966`)) ##### React DOM Server * Fix an incorrect value being provided by new context API. ([@​ericsoderberghp] in [#​12985](`https://github.com/facebook/react/pull/12985`), [@​gaearon] in [#​13019](`https://github.com/facebook/react/pull/13019`)) ##### React Test Renderer * Allow multiple root children in test renderer traversal API. ([@​gaearon] in [#​13017](`https://github.com/facebook/react/pull/13017`)) * Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@​fatfisz] in [#​13030](`https://github.com/facebook/react/pull/13030`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com). Co-authored-by: Renovate Bot <bot@renovateapp.com>
Fixes #12964
When a fragment is reconciled, we directly move onto it’s children. Since an empty
<React.Fragment/>
will have children ofundefined
, this would always throw.To fix this, we bail out in those cases. This case now behaves like returning
null
directly.