[Fiber] Implement findDOMNode and isMounted #8083
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
iamdustan
reviewed
Oct 25, 2016
| @@ -54,17 +56,20 @@ export type HostConfig<T, P, I, TI, C> = { | |||
|
|
|||
| type OpaqueNode = Fiber; | |||
|
|
|||
| export type Reconciler<C, I> = { | |||
| export type Reconciler<C, I, TI> = { | |||
iamdustan
reviewed
Oct 25, 2016
| break; | ||
| } | ||
| case PlacementAndUpdate: { | ||
| commitInsertion(effectfulFiber); | ||
| const current = effectfulFiber.alternate; | ||
| commitWork(current, effectfulFiber); | ||
| // Clear the effect tag so that we know that this is inserted, before |
There was a problem hiding this comment.
Is this supposed to be the same comment as above? How is setting the effectTag to Update equal to clearing it?
There was a problem hiding this comment.
Clearing the "placement" part of it.
This is the naive implementation that doesn't cover the case where there are two diverging fibers, or if this tree is unmounted.
This is the naive implementation that doesn't cover the case where it has completed but not yet committed. It also doesn't deal with unmounts since they currently don't clean up the item in the ReactInstanceMap.
This will let us use these pointers to reason about a tree. Whether if it is "current" or "work in progress".
There are two cases where we have a Fiber that is not actually mounted. Either it is part of a tree that has not yet been inserted or it is part of a tree that was unmounted. For the insertion case, we can check the parents to see if there is any insertion effect pending along the parent path. For deletions, we can now check if any of the return pointers is null without actually being the root.
First we need to check if a component subtree is mounted at all. If it is, we need to search down the fiber for the first host node. However, we might be searching the "work in progress" instead of current. One realization is that it doesn't matter if we search work in progress or current if they're the same. They will generally be the same unless there is an insertion pending or something in the alternate tree was already deleted. So if we find one of those cases, we switch to look in the alternate tree instead.
These are new features that aren't covered by existing tests. It is now possible to use findDOMNode to find a text node. When a component returns a fragment, it will search to find the first host component just like element.querySelector does.
sebmarkbage
force-pushed
the
fiberfinddomnode
branch
from
4acdb8b to
8e2d7f8
Compare
|
Yay |
gaearon
reviewed
Oct 26, 2016
| @@ -156,6 +162,8 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) { | |||
| // and lastEffect since they're on every node, not just the effectful | |||
| // ones. So we have to clean everything as we reuse nodes anyway. | |||
| effectfulFiber.nextEffect = null; | |||
| // Ensure that we reset the effectTag here so that we can rely on effect | |||
| // tags to reason about the current life-cycle. | |||
There was a problem hiding this comment.
This comment doesn't apply to the line below anymore. It used to refer to effectTag assignment which has been moved.
sophiebits
reviewed
Oct 26, 2016
| node = parent = parent.alternate; | ||
| continue; | ||
| } | ||
| if (node.tag === HostComponent || node.tag === HostText) { |
There was a problem hiding this comment.
Does this mean if you render two host components as a fragment then this method will return the first?
There was a problem hiding this comment.
Yes. I figured that it would be unnecessary to keep searching for the second one to force an error. The last commit adds a test for this. 8e2d7f8#diff-7556f354e68e9c4454dbd21aedf610dfR87
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The strategy of finding the "current" through the back pointers turned out to be inefficient. You basically have to scan from the top to find them since the return pointer is active even when a child is in
progressedChildinstead ofchild.isMounted is pretty easy though since we can just cut off the return pointer when a child gets deleted.
The key realization to fix findDOMNode is the realization that it doesn't matter which one is current if there is no pending insertion / previous deletion. So we can just check for those to determine if we're looking in the wrong tree.
I'm not too worried about this being understandable unless we have other use cases for "reflection" that are less deprecated.
Also note two new features: findDOMNode in fragment returns first child rather than error/null. Just like element.querySelector. findDOMNode when a component returns a string, finds the host text node. This is the first time you could get access to a text node instance. It's not super great since if the text node merges (due to normalization) then it could actually represent multiple text nodes, but maybe that's fine. Seems useful.