Fix findDOMNode incorrectly failing inside Suspense #14198
Conversation
| } | ||
|
|
||
| ReactDOM.render(<App />, container); | ||
| // expect(log).toEqual([]); |
There was a problem hiding this comment.
I wasn't sure what's expected here. We'll need to fill those out before merging.
|
I pushed a commit that I think fixes it. If findCurrentFiberUsingSlowPath is called on a fiber that has an alternate whose parent has no alternate, it previously incorrectly thought it had reached the root. We hit this now and not before because of the Fragment we add under each Suspense child – unlike all other ancestors, it doesn't end up having an alternate. |
sophiebits
changed the title
| // This happens when we bailout on low priority: the bailed out fiber's | ||
| // child reuses the current child. | ||
| let parentB = parentA.alternate; | ||
| if (!parentB || parentA.child === parentB.child) { |
There was a problem hiding this comment.
That's probably better. I had copied the truthy check from the old code.
There was a problem hiding this comment.
Maybe Seb wanted it to be slower. Bad bad findDOMNode.
|
Could we be making similar assumptions about presence of alternate elsewhere? Such as in event system traversal logic (eg traverseEnterLeave) or DOM Component Tree? |
|
I need to take 7 hours to think about this. Not so sure about this. But also if you don’t want it to error you should test that the return value is what you expect it to be. It’s better to error than return the wrong value. |
| @@ -117,16 +117,17 @@ export function findCurrentFiberUsingSlowPath(fiber: Fiber): Fiber | null { | |||
| let b = alternate; | |||
| while (true) { | |||
| let parentA = a.return; | |||
| let parentB = parentA ? parentA.alternate : null; | |||
| if (!parentA || !parentB) { | |||
| if (!parentA) { | |||
There was a problem hiding this comment.
I don’t think the logic is designed to make assumptions about which side is which. That’s why it’s a and b instead of say current and workInProgress.
This seems to think that A has some different property than B. Why is this?
There was a problem hiding this comment.
I believe the logic should have really been !parentA && !parentB. Then we should enter the next if if either is null. But it is impossible for parentA to be null and parentB to be non-null. Hence the asymmetry. I could write it in a more symmetric way but it would have dead branches.
|
OK you're right it returns the wrong value. Dang. |
|
What even is the right value? It doesn’t have well defined behavior since conceptually the fallback/main content could be first or last or not exist in the tree. |
|
I think that in this case you need to skip past the fragment and look at the two parents of the fragment to figure out if they converge. |
|
I think so too. But is it supposed to return the hidden node? |
|
ugh the logic to support suspense properly here looks so complicated |
|
Note that Suspense isn’t even done yet. The hydration of Suspense is looking to triple the complexity of Suspense and that’s not even taking findDOMNode into account. |
This doesn't rely on checking the tag. When the alternate of a parent is missing, it assumes it's a fragment indirection and moves onto the next parent fiber.
This doesn't rely on checking the tag. When the alternate of a parent is missing, it assumes it's a fragment indirection and moves onto the next parent fiber.
|
This was fixed in #15312 |
Builds on #14197. Second test
is failingis now fixed.Based on the test in #14188 (comment) found by @arianon. Unlike the issue from #14197, this one seems like it existed even before the Suspense refactorings. So maybe it was always there.