Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(animations): Ensure elements are removed from the cache after leave animation. #50929

Closed
wants to merge 1 commit into from
Closed

fix(animations): Ensure elements are removed from the cache after leave animation. #50929

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
29 changes: 5 additions & 24 deletions packages/animations/browser/src/render/transition_animation_engine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -508,14 +508,6 @@ export class AnimationTransitionNamespace {
this.players.forEach(p => p.destroy());
this._signalRemovalForInnerTriggers(this.hostElement, context);
}

elementContainsData(element: any): boolean {
let containsData = false;
if (this._elementListeners.has(element)) containsData = true;
containsData =
(this._queue.find(entry => entry.element === element) ? true : false) || containsData;
return containsData;
}
}

export interface QueuedTransition {
Expand Down Expand Up @@ -640,19 +632,18 @@ export class TransitionAnimationEngine {

destroy(namespaceId: string, context: any) {
if (!namespaceId) return;
this.afterFlush(() => {});

const ns = this._fetchNamespace(namespaceId);

this.afterFlush(() => {
this.afterFlushAnimationsDone(() => {
const ns = this._fetchNamespace(namespaceId);
this.namespacesByHostElement.delete(ns.hostElement);
delete this._namespaceLookup[namespaceId];
const index = this._namespaceList.indexOf(ns);
if (index >= 0) {
this._namespaceList.splice(index, 1);
}
ns.destroy(context);
delete this._namespaceLookup[namespaceId];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like, we're good with moving everything into afterFlushAnimationsDone !

});

this.afterFlushAnimationsDone(() => ns.destroy(context));
}

private _fetchNamespace(id: string) {
Expand Down Expand Up @@ -1314,16 +1305,6 @@ export class TransitionAnimationEngine {
return rootPlayers;
}

elementContainsData(namespaceId: string, element: any) {
let containsData = false;
const details = element[REMOVAL_FLAG] as ElementAnimationState;
if (details && details.setForRemoval) containsData = true;
if (this.playersByElement.has(element)) containsData = true;
if (this.playersByQueriedElement.has(element)) containsData = true;
if (this.statesByElement.has(element)) containsData = true;
return this._fetchNamespace(namespaceId).elementContainsData(element) || containsData;
}

afterFlush(callback: () => any) {
this._flushFns.push(callback);
}
Expand Down
13 changes: 10 additions & 3 deletions packages/animations/browser/test/render/transition_animation_engine_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,20 @@ describe('TransitionAnimationEngine', () => {
registerTrigger(element, engine, trig);
setProperty(element, engine, 'myTrigger', 'value');
engine.flush();
expect(engine.statesByElement.has(element))
.toBe(true, 'Expected element data to be defined.');
expect(engine.playersByElement.has(element))
.toBe(true, 'Expected element data to be defined.');

expect(engine.elementContainsData(DEFAULT_NAMESPACE_ID, element)).toBeTruthy();

engine.destroy(DEFAULT_NAMESPACE_ID, null);
engine.removeNode(DEFAULT_NAMESPACE_ID, element, true);
engine.flush();
engine.players[0].finish();

expect(engine.elementContainsData(DEFAULT_NAMESPACE_ID, element)).toBeTruthy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great suggestion ! I'll remove it !

expect(engine.statesByElement.has(element))
.toBe(false, 'Expected element data to be undefined.');
expect(engine.playersByElement.has(element))
.toBe(false, 'Expected element data to be undefined.');
});

it('should create and recreate a namespace for a host element with the same component source',
Expand Down