-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[iOS][Layout Animations] Remove exiting
views from UIManager's view registry
#3824
Conversation
84912ac
to
0c950fa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This loosk good. Left two minor comments, please address them before merging
c95c5d1
to
023295f
Compare
023295f
to
f733def
Compare
Hey guys, just checking if this has been released or if it will be in the next |
… registry (#3824) <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary Currently, when views with an `exiting` animation are unmounted, any deleted in that transaction aren't removed from the UIManager's view registry (the mapping of react tags to native views). This can cause memory leaks of removed views. This PR restores the original behaviour of UIManager when removing views, allowing it to clean them up properly, and reattaches the views with `exiting` animations after the transaction removing the views has completed. <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. --> - In example app, go to "Entering and Exiting with Layout" - Set a breakpoint somewhere in `RCTUIManager`, for example `_manageChildren`. - Press "Toggle" to create the animated views, and note the number of entries in `_viewRegistry`. - Disable the breakpoint, resume the app and start spamming the toggle button. - Enable the breakpoint again, and press toggle one more time. - The number of entries in `_viewRegistry` should be the same as before spamming toggle.
@nandorojo This hasn't been included in a release yet, we perhaps want to include one more fix (#3849) before publishing |
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> #3824 introduces a bug with how we register ancestors of views with `exiting` animations. When a tree with `exiting` views is reattached to the native view hierarchy, we don't register the ancestors of that tree as ancestors of exiting subviews. This may cause issues with deleting these views early, while there're still `exiting` animations running inside them. ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. -->
… registry (software-mansion#3824) <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary Currently, when views with an `exiting` animation are unmounted, any deleted in that transaction aren't removed from the UIManager's view registry (the mapping of react tags to native views). This can cause memory leaks of removed views. This PR restores the original behaviour of UIManager when removing views, allowing it to clean them up properly, and reattaches the views with `exiting` animations after the transaction removing the views has completed. <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. --> - In example app, go to "Entering and Exiting with Layout" - Set a breakpoint somewhere in `RCTUIManager`, for example `_manageChildren`. - Press "Toggle" to create the animated views, and note the number of entries in `_viewRegistry`. - Disable the breakpoint, resume the app and start spamming the toggle button. - Enable the breakpoint again, and press toggle one more time. - The number of entries in `_viewRegistry` should be the same as before spamming toggle.
…ware-mansion#3849) <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary <!-- Explain the motivation for this PR. Include "Fixes #<number>" if applicable. --> software-mansion#3824 introduces a bug with how we register ancestors of views with `exiting` animations. When a tree with `exiting` views is reattached to the native view hierarchy, we don't register the ancestors of that tree as ancestors of exiting subviews. This may cause issues with deleting these views early, while there're still `exiting` animations running inside them. ## Test plan <!-- Provide a minimal but complete code snippet that can be used to test out this change along with instructions how to run it and a description of the expected behavior. -->
Summary
Currently, when views with an
exiting
animation are unmounted, any deleted in that transaction aren't removed from the UIManager's view registry (the mapping of react tags to native views).This can cause memory leaks of removed views.
This PR restores the original behaviour of UIManager when removing views, allowing it to clean them up properly, and reattaches the views with
exiting
animations after the transaction removing the views has completed.Test plan
RCTUIManager
, for example_manageChildren
._viewRegistry
._viewRegistry
should be the same as before spamming toggle.