fix(animations): cleanup DOM elements when the root view is removed#39982
fix(animations): cleanup DOM elements when the root view is removed#39982arturovt wants to merge 1 commit intoangular:masterfrom arturovt:fix/39955
Conversation
atscott
left a comment
There was a problem hiding this comment.
LGTM, just one nit in the test.
packages/platform-browser/animations/test/animation_renderer_spec.ts
Outdated
Show resolved
Hide resolved
|
Looks like this is causing some failures internally based on the presubmits and will need more investigation. |
Currently, when importing `BrowserAnimationsModule`, Angular uses `AnimationRenderer` as the renderer. When the root view is removed, the `AnimationRenderer` defers the actual work to the `TransitionAnimationEngine` to do this, and the `TransitionAnimationEngine` doesn't actually remove the DOM node, but just calls `markElementAsRemoved()`. The actual DOM node is not removed until `TransitionAnimationEngine` "flushes". Unfortunately, though, that "flush" will never happen, since the root view is being destroyed and there will be no more flushes. This commit adds `flush()` call when the root view is being destroyed. PR Close #39955
|
@atscott I have removed lifecycle hook from test. Ping me if some change is needed regarding the presubmit, since I can’t see why it fails |
|
@arturovt There were a couple instances of tests that were inadvertently depending on previous tests which had not been cleaned up. I've gone through and fixed these by hand, but we may still want to include this as a breaking change in v12 rather than in a minor/patch version. @josephperrott is working on another project that has to do with test teardowns. Joey, I mentioned this PR to you before and you asked why this would cause failures because tests should be using the Edit: Also a link to the global tap run |
|
@atscott I've also investigated that there is currently a memory leak on the server-side because engine doesn't flush elements on destroy:
This |
|
Thanks for the additional info @arturovt. This is on our list to land for v12. Since it's a breaking change, we have to wait for a major release before merging. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently, when importing
BrowserAnimationsModule, Angular usesAnimationRendereras the renderer. When the root view is removed, the
AnimationRendererdefers the actualwork to the
TransitionAnimationEngineto do this, and theTransitionAnimationEnginedoesn't actually remove the DOM node, but just calls
markElementAsRemoved().The actual DOM node is not removed until
TransitionAnimationEngine"flushes".Unfortunately, though, that "flush" will never happen, since the root view is being
destroyed and there will be no more flushes.
This commit adds
flush()call when the root view is being destroyed.PR Checklist
PR Type
What is the current behavior?
Issue Number: #39955
Does this PR introduce a breaking change?