fix(animations): Ensure elements are removed from the cache after leave animation.#50929
fix(animations): Ensure elements are removed from the cache after leave animation.#50929JeanMeche wants to merge 1 commit intoangular:mainfrom
Conversation
1e4ea00 to
10a8d95
Compare
JoostK
left a comment
There was a problem hiding this comment.
Thanks for investigating, this must have been hard to track down!
There was a problem hiding this comment.
This test definitely looked broken before, with both these elementContainsData calls being checked to return true even after removal and flush. I'd suggest also removing the first elementContainsData assertion as it seems rather meaningless with the new assertions, and it allows dropping test-only elementContainsData implementations from production code (both from TransitionAnimationsEngine and AnimationTransitionNamespace).
There was a problem hiding this comment.
Great suggestion ! I'll remove it !
There was a problem hiding this comment.
It's a bit unfortunate to me that cleanup logic is (still) spread across two phases, especially now that we've seen that first-phase cleanup can remove needed state too early. Apparently the destroyed namespace is still interacting with this engine, so arguably we should not be cleaning it up until those interactions can no longer occur entirely?
tldr; can all of the afterFlush work be pushed into this second phase, or would that result in issues?
There was a problem hiding this comment.
Looks like, we're good with moving everything into afterFlushAnimationsDone !
…ve animation. This commit fixes a memory leak. `_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup. Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element. Fixes angular#24197 & angular#50533
10a8d95 to
f0e1011
Compare
jessicajaniuk
left a comment
There was a problem hiding this comment.
Nice work fixing this!
|
This PR was merged into the repository by commit a14bdfe. |
…ve animation. (angular#50929) This commit fixes a memory leak. `_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup. Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element. Fixes angular#24197 & angular#50533 PR Close angular#50929
|
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. |
…ve animation. (angular#50929) This commit fixes a memory leak. `_namespaceLookup` was cleared before the call to `processLeaveNode()` which was using the lookup. Without that lookup `clearElementCache()` wasn't called thus keeping a reference to the element. Fixes angular#24197 & angular#50533 PR Close angular#50929
This commit fixes a memory leak where references were retained in multiple
Mapstructures._namespaceLookupwas cleared before the call toprocessLeaveNode()which was using the lookup.Without that lookup
clearElementCache()wasn't called thus keeping a reference to the element.Fixes #24197 & #50533
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?