Further update children re-rentrancy problems. #315
Conversation
| if config.tempFixUpdateChildrenReEntrancy then | ||
| if not virtualNode.wasUnmounted then | ||
| unmountVirtualNode(virtualNode) | ||
| end |
There was a problem hiding this comment.
So I take this to mean that the re-entrancy was, in some cases, causing the same node to be unmounted more than once, is that the case? Can we get a comment here that explains the scenario that might trigger it, the way we have with the other blocks you added?
There was a problem hiding this comment.
Added a comment for this
There was a problem hiding this comment.
I wonder if this could be related to #235? Would it fix that issue?
There was a problem hiding this comment.
I don't think this would fix that issue, seems to be more about extra renders than extra unmounts.
Co-authored-by: Paul Doyle <37384169+ZoteTheMighty@users.noreply.github.com>
|
The wasUnmounted logic is a fix for something that could already happen. I was initially worried that double unmounting could be caused by this new logic (and still am) but it does actually already happen in the original bug. Old way this could happen: |
| }) | ||
|
|
||
| self.onDidMountCallback = function() | ||
| if self.state.count < 5 then |
There was a problem hiding this comment.
Is there a specific reason why 5? To me it looks like onDidMountCallback should be called only once, so self.state.count should be 1 and then turn into 2. If that is correct then maybe we could add an explicit assertion here that shows that (i.e. expect(self.state.count).to.equal(1)).
There was a problem hiding this comment.
No specific reason why 5, just want to make sure there isn't a scenario where this test starts looping an unreasonable amount of times. Before the bug fix, onDidMountCallback would actually be called multiple times due to the duplicate components being mounted. I'll add the expect here
There was a problem hiding this comment.
Actually, onDidMountCallback is still called twice here. Basically, two different LowestComponent are still mounted so this is actually called twice. The first LowestComponent to be mounted is unmounted to prevent the duplication issue.
|
|
||
| self.onDidMountCallback = function() | ||
| if self.state.count < 5 then | ||
| self:setState({ |
There was a problem hiding this comment.
Same thing as above ⬆️ (or below, I don't know how github will order my comments 😄 )
| if config.tempFixUpdateChildrenReEntrancy then | ||
| if not virtualNode.wasUnmounted then | ||
| unmountVirtualNode(virtualNode) | ||
| end |
There was a problem hiding this comment.
I wonder if this could be related to #235? Would it fix that issue?
There was a problem hiding this comment.
This looks reasonably safe to me. Thanks for doing the legwork and pinning the changes with testing. Please take a look at @oltrep's comments, but I think the solution as implemented looks alright.
See the previous pull request here:
#301
Originally I had thought that this issue was specific to events but it can actually also be caused using callbacks. We recently ran into this issue again and the original fix for this did not fix the issue because it was caused by callbacks and not events.
Basically, the issue is that nodes lower down the Roact tree can change outside of their standard lifecycle and then cause a callback or event to a node higher up the tree. That component can re-render while the node further down the tree is updating its children, causing duplicates of elements to be added.
I added a check to guard against reentrancy in updateChildren and if updateChildren has been rerentered any extra nodes that have been created and are no longer needed will be unmounted.
Checklist before submitting:
CHANGELOG.md