Remove extra loop (?) #11889
Remove extra loop (?) #11889
Conversation
| let inst = node[internalInstanceKey]; | ||
| if (inst.tag === HostComponent || inst.tag === HostText) { | ||
| // In Fiber, this will always be the deepest root. | ||
| return inst; | ||
| } | ||
| for (; node && (inst = node[internalInstanceKey]); node = parents.pop()) { | ||
| closest = inst; | ||
| } |
There was a problem hiding this comment.
I've tried to check back through the history to see what was going on here, and at least originally it used to try and precache nodes: 6d20556#diff-a2fd175216a5aa6425363e6773426f3c
But at some point that was removed but I can't find git blame is not helping :/
Then
There was a problem hiding this comment.
When does line 50 trigger?
There was a problem hiding this comment.
Sorry, I mean like what code path causes closest to be assigned to the instance? And what codepath even lets this function get past return inst on line 47?
There was a problem hiding this comment.
I think in practical usage it doesn't get past 47, but I guess theoretically it could, if the fiber was a different type?
My guess tho is that this was for both stack and fiber and never got removed. Though even if it could get past 47 I don't think the loop does anything other than run one iteration, and return inst
There was a problem hiding this comment.
Cool. I'd love to drop this if the code path no longer fires after Stack's removal. I'm afraid to ping anyone to bug them on holiday, but I think it's worth investigating.
There was a problem hiding this comment.
no rush clearly. :)
There was a problem hiding this comment.
It was hard to find due to a couple project reorganizations nuking the file history, but this was removed in #10798. I think the most likely explanation is that the whole loop could have been removed as well, but it wasn't clear that it was stack-only.
It's a little confusing, but at first glance it seems safe to remove 😄
|
|
||
| return closest; |
There was a problem hiding this comment.
Does it make sense to return null for invalid fiber types? Maybe it should throw if it's not a HostComponent or HostText? I'm not sure if there are any situations where this should be called with any other type of node.
There was a problem hiding this comment.
that's a good question. I think your probably right, however i was trying to keep with the spirit (for lack of a better term) of the method by failing softly.
I do think tho that a situation where its not a component or text is kind of a different sort of error and maybe should fail hard. Are roots or portals cached on elements? Presumable you should hit a component before that tho
|
This seems safe to me, thanks. |
|
(We should probably follow this up with an invariant) |
* Remove extra loop (?) * prettier
* Remove extra loop (?) * prettier
Am I just off base here? It looks like the
forloop's condition is the inverse of the while loop. Presumablyparentswill have no other elements in it with cached instances since that's the condition for building said array?