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

Allow multiple root children in test renderer traversal API #13017

Merged
merged 1 commit into from
Jun 11, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jun 11, 2018

This should fix some bugs @sahrens was seeing.

Test renderer traversal API currently treats "fragmenty" nodes (fragments, providers, modes) as passthrough. That usually works, but it breaks down at the top level. The .root node has to point to a Fiber, so if it's a "passthrough" Fiber, we currently invariant.

Ideally I think the root node should have always been a special kind of "root" whose type and props don't match the nearest "materialized" node. But that ship has sailed, and changing this would be breaking.

So for now I want to enable those use cases by only "materializing" a special root node (with type: null, props: null) when it has multiple children.

It should be mostly additive because that kind of code used to throw. It does change the behavior for a top-level non-keyed fragment with many children (we used to ignore all children but the first) but this actually sounds like a bugfix to me, especially given that subsequent children were not ignored in the toJSON() API.

In the future, we probably should change .root to always be a special node, regardless of whether it has one or more children.

@pull-bot
Copy link

Details of bundled changes.

Comparing: d480782...c99ae2e

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% +0.3% 340.42 KB 341.19 KB 73.62 KB 73.85 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.1% 45.78 KB 45.92 KB 14.2 KB 14.22 KB UMD_PROD
react-test-renderer.development.js +0.2% +0.3% 331.26 KB 332.02 KB 70.89 KB 71.12 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.3% 🔺+0.2% 44.88 KB 45.03 KB 13.74 KB 13.76 KB NODE_PROD
ReactTestRenderer-dev.js +0.2% +0.3% 337.15 KB 337.87 KB 70.48 KB 70.71 KB FB_WWW_DEV

Generated by 🚫 dangerJS

@flarnie
Copy link
Contributor

flarnie commented Jun 11, 2018

What's the issue describing the problem this fixes?

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 11, 2018

There's no particular issue report but you can try only cherry-picking the new test and apply it on master. You'll see both invariants (in some cases) and mismatches (in other cases).

Copy link
Contributor

@flarnie flarnie left a comment

Choose a reason for hiding this comment

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

:D This looks like it may fix that bug I was investigating.

yea_pokemon

]);

function getChildren(parent: Fiber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - we could explicitly flow type the return value; Array<ReactTestInstance | string>.

Also I see that this was copy-pasted from the getChildren method below. Wish there was a docblock, it's not very clear to me why this works the way it does. But that could be for the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We don't always Flow-type internal functions because those change more often. I figured that since we still have an outer annotation on the public API, we can leave this one inferred. If it returns something bad the annotation on the calling method should catch it.

if (node.child === null) {
return children;
}
node.child.return = node;
Copy link
Contributor

Choose a reason for hiding this comment

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

Granted this is copy-pasted - but wondering if you know, why do we mutate the fiber instance to set it's return here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So, this is a pattern we use everywhere we traverse fibers deeply. It's necessary because each fiber has two copies. The return pointer points to the parent but we don't know which copy (current of work in progress). If we just naïvely follow "return" to go up, we might end up in a different tree copy from the previous update.

This is why every time we descend down, no matter when, we always overwrite the .return pointer to the parent in the tree we're working on. This should have no observable side effect because we do this on every deep traversal, and we never trust the chain of return pointers to give us the "current" tree.

}
while (node.sibling === null) {
if (node.return === startingNode) {
break outer;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

20287394_1768381429856192_1934891266588278784_n

}
(node.sibling: any).return = node.return;
node = (node.sibling: any);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sure it will become clear as I continue reading, but I was surprised that this method mutates the nodes and goes down to potentially get deeply nested children.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just copy pasted from below. I didn't write it.

It works this way to skip over fragments / context consumers / other wrappers in the tree, and only "see" custom components and host nodes in traversal APIs.

@sophiebits
Copy link
Collaborator

My idea was that .root would give you a node that can be unmounted if it's swapped out, whereas the test renderer itself remains the same object no matter what updates you do. I guess this is consistent with that.

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 11, 2018

Yeah I think this is still true.

The case where you first render fragment with one child and then render it with multiple children might be a bit confusing because .root would point to different nodes between re-renders even though the state is preserved. Do you think this is too strange?

@sophiebits
Copy link
Collaborator

#7516 was where that was discussed I guess. But I think we got rid of that restriction.

@gaearon
Copy link
Collaborator Author

gaearon commented Jun 11, 2018

renderer.update(<Fragment><div /></Fragment>).root // div
renderer.update(<Fragment><div /><div /></Fragment>).root // rooty mcrootface -- is that weird?

@sophiebits
Copy link
Collaborator

I think it's OK.

@gaearon gaearon merged commit 30bc8ef into facebook:master Jun 11, 2018
@gaearon gaearon deleted the root-stuff branch June 11, 2018 19:07
@sahrens
Copy link
Contributor

sahrens commented Jun 12, 2018

Great, thanks for the fix!

bors bot referenced this pull request in mozilla/delivery-console 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](`https://github.com/facebook/react/pull/12911`))
##### React DOM

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

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

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`https://github.com/facebook/react/pull/13017`))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;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>
bors bot referenced this pull request in mythmon/corsica-tree-status 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](`https://github.com/facebook/react/pull/12911`))
##### React DOM

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

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

* Allow multiple root children in test renderer traversal API. ([@&#8203;gaearon] in [#&#8203;13017](`https://github.com/facebook/react/pull/13017`))
* Fix `getDerivedStateFromProps()` in the shallow renderer to not discard the pending state. ([@&#8203;fatfisz] in [#&#8203;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>
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.

6 participants