From 436a17955289430f8bd8724c75d63ff68cb370b2 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 24 Feb 2017 12:10:19 -0800 Subject: [PATCH] fix(animations): properly cache renderer and namespace triggers (#14703) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Don’t use the animation renderer if a component used style encapsulation but no animations. - The `AnimationRenderer` should be cached in the same lifecycle as its delegate. - Trigger names need to be namespaced per component type. --- modules/@angular/core/src/render/api.ts | 8 +++- modules/@angular/core/src/view/services.ts | 2 + .../animation_integration_next_spec.ts | 16 +++++++ .../animations/src/animation_engine.ts | 2 +- .../src/render/animation_renderer.ts | 44 +++++++++++++------ .../src/render/dom_animation_engine.ts | 4 +- .../src/render/noop_animation_engine.ts | 5 ++- .../platform-browser/src/dom/dom_renderer.ts | 2 + .../test/animation/animation_renderer_spec.ts | 4 +- .../platform-server/src/server_renderer.ts | 2 + .../src/web_workers/worker/renderer.ts | 2 + tools/public_api_guard/core/typings/core.d.ts | 5 ++- 12 files changed, 73 insertions(+), 23 deletions(-) diff --git a/modules/@angular/core/src/render/api.ts b/modules/@angular/core/src/render/api.ts index 562faa713e..2ea657a0b0 100644 --- a/modules/@angular/core/src/render/api.ts +++ b/modules/@angular/core/src/render/api.ts @@ -123,7 +123,7 @@ export interface RendererTypeV2 { id: string; encapsulation: ViewEncapsulation; styles: (string|any[])[]; - data: {[kind: string]: any[]}; + data: {[kind: string]: any}; } /** @@ -137,6 +137,12 @@ export abstract class RendererFactoryV2 { * @experimental */ export abstract class RendererV2 { + /** + * This field can be used to store arbitrary data on this renderer instance. + * This is useful for renderers that delegate to other renderers. + */ + abstract get data(): {[key: string]: any}; + abstract destroy(): void; abstract createElement(name: string, namespace?: string): any; abstract createComment(value: string): any; diff --git a/modules/@angular/core/src/view/services.ts b/modules/@angular/core/src/view/services.ts index 2db987597c..38d84ebc02 100644 --- a/modules/@angular/core/src/view/services.ts +++ b/modules/@angular/core/src/view/services.ts @@ -427,6 +427,8 @@ class DebugRendererFactoryV2 implements RendererFactoryV2 { class DebugRendererV2 implements RendererV2 { constructor(private delegate: RendererV2) {} + get data() { return this.delegate.data; } + destroyNode(node: any) { removeDebugNodeFromIndex(getDebugNode(node)); if (this.delegate.destroyNode) { diff --git a/modules/@angular/core/test/animation/animation_integration_next_spec.ts b/modules/@angular/core/test/animation/animation_integration_next_spec.ts index db89cf2d8b..41b38fbb8e 100644 --- a/modules/@angular/core/test/animation/animation_integration_next_spec.ts +++ b/modules/@angular/core/test/animation/animation_integration_next_spec.ts @@ -79,6 +79,22 @@ function declareTests({useJit}: {useJit: boolean}) { ]); }); + it('should not throw an error if a trigger with the same name exists in separate components', + () => { + @Component({selector: 'cmp1', template: '...', animations: [trigger('trig', [])]}) + class Cmp1 { + } + + @Component({selector: 'cmp2', template: '...', animations: [trigger('trig', [])]}) + class Cmp2 { + } + + TestBed.configureTestingModule({declarations: [Cmp1, Cmp2]}); + const cmp1 = TestBed.createComponent(Cmp1); + const cmp2 = TestBed.createComponent(Cmp2); + }); + + it('should trigger a state change animation from void => state on the component host element', () => { @Component({ diff --git a/modules/@angular/platform-browser/animations/src/animation_engine.ts b/modules/@angular/platform-browser/animations/src/animation_engine.ts index 2315b6e00d..d8088f145d 100644 --- a/modules/@angular/platform-browser/animations/src/animation_engine.ts +++ b/modules/@angular/platform-browser/animations/src/animation_engine.ts @@ -8,7 +8,7 @@ import {AnimationPlayer, AnimationTriggerMetadata} from '@angular/animations'; export abstract class AnimationEngine { - abstract registerTrigger(trigger: AnimationTriggerMetadata): void; + abstract registerTrigger(trigger: AnimationTriggerMetadata, name?: string): void; abstract onInsert(element: any, domFn: () => any): void; abstract onRemove(element: any, domFn: () => any): void; abstract setProperty(element: any, property: string, value: any): void; diff --git a/modules/@angular/platform-browser/animations/src/render/animation_renderer.ts b/modules/@angular/platform-browser/animations/src/render/animation_renderer.ts index d05e29a4e1..ac2f63bd59 100644 --- a/modules/@angular/platform-browser/animations/src/render/animation_renderer.ts +++ b/modules/@angular/platform-browser/animations/src/render/animation_renderer.ts @@ -5,7 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ -import {AnimationTriggerMetadata} from '@angular/animations'; +import {AnimationEvent, AnimationTriggerMetadata} from '@angular/animations'; import {Injectable, NgZone, RendererFactoryV2, RendererTypeV2, RendererV2} from '@angular/core'; import {AnimationEngine} from '../animation_engine'; @@ -18,15 +18,18 @@ export class AnimationRendererFactory implements RendererFactoryV2 { createRenderer(hostElement: any, type: RendererTypeV2): RendererV2 { let delegate = this.delegate.createRenderer(hostElement, type); - if (!hostElement || !type) return delegate; + if (!hostElement || !type || !type.data || !type.data['animation']) return delegate; - let animationRenderer = type.data['__animationRenderer__'] as any as AnimationRenderer; - if (animationRenderer && delegate == animationRenderer.delegate) { - return animationRenderer; + let animationRenderer = delegate.data['animationRenderer']; + if (!animationRenderer) { + const namespaceId = type.id; + const animationTriggers = type.data['animation'] as AnimationTriggerMetadata[]; + animationTriggers.forEach( + trigger => + this._engine.registerTrigger(trigger, namespaceify(namespaceId, trigger.name))); + animationRenderer = new AnimationRenderer(delegate, this._engine, this._zone, namespaceId); + delegate.data['animationRenderer'] = animationRenderer; } - const animationTriggers = type.data['animation'] as AnimationTriggerMetadata[]; - animationRenderer = (type.data as any)['__animationRenderer__'] = - new AnimationRenderer(delegate, this._engine, this._zone, animationTriggers); return animationRenderer; } } @@ -37,13 +40,12 @@ export class AnimationRenderer implements RendererV2 { constructor( public delegate: RendererV2, private _engine: AnimationEngine, private _zone: NgZone, - _triggers: AnimationTriggerMetadata[] = null) { + private _namespaceId: string) { this.destroyNode = this.delegate.destroyNode ? (n) => delegate.destroyNode(n) : null; - if (_triggers) { - _triggers.forEach(trigger => _engine.registerTrigger(trigger)); - } } + get data() { return this.delegate.data; } + destroy(): void { this.delegate.destroy(); } createElement(name: string, namespace?: string): any { @@ -102,7 +104,7 @@ export class AnimationRenderer implements RendererV2 { setProperty(el: any, name: string, value: any): void { if (name.charAt(0) == '@') { - this._engine.setProperty(el, name.substr(1), value); + this._engine.setProperty(el, namespaceify(this._namespaceId, name.substr(1)), value); this._queueFlush(); } else { this.delegate.setProperty(el, name, value); @@ -115,7 +117,13 @@ export class AnimationRenderer implements RendererV2 { const element = resolveElementFromTarget(target); const [name, phase] = parseTriggerCallbackName(eventName.substr(1)); return this._engine.listen( - element, name, phase, (event: any) => this._zone.run(() => callback(event))); + element, namespaceify(this._namespaceId, name), phase, (event: any) => { + const e = event as any; + if (e.triggerName) { + e.triggerName = deNamespaceify(this._namespaceId, e.triggerName); + } + this._zone.run(() => callback(event)); + }); } return this.delegate.listen(target, eventName, callback); } @@ -151,3 +159,11 @@ function parseTriggerCallbackName(triggerName: string) { const phase = triggerName.substr(dotIndex + 1); return [trigger, phase]; } + +function namespaceify(namespaceId: string, value: string): string { + return `${namespaceId}#${value}`; +} + +function deNamespaceify(namespaceId: string, value: string): string { + return value.replace(namespaceId + '#', ''); +} diff --git a/modules/@angular/platform-browser/animations/src/render/dom_animation_engine.ts b/modules/@angular/platform-browser/animations/src/render/dom_animation_engine.ts index 522dd2262a..c13e834124 100644 --- a/modules/@angular/platform-browser/animations/src/render/dom_animation_engine.ts +++ b/modules/@angular/platform-browser/animations/src/render/dom_animation_engine.ts @@ -55,8 +55,8 @@ export class DomAnimationEngine { return players; } - registerTrigger(trigger: AnimationTriggerMetadata): void { - const name = trigger.name; + registerTrigger(trigger: AnimationTriggerMetadata, name: string = null): void { + name = name || trigger.name; if (this._triggers[name]) { throw new Error(`The provided animation trigger "${name}" has already been registered!`); } diff --git a/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts b/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts index 7ec0b1ead4..3818c25655 100644 --- a/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts +++ b/modules/@angular/platform-browser/animations/src/render/noop_animation_engine.ts @@ -34,7 +34,7 @@ export class NoopAnimationEngine extends AnimationEngine { private _onDoneFns: (() => any)[] = []; private _triggerStyles: {[triggerName: string]: {[stateName: string]: ɵStyleData}} = {}; - registerTrigger(trigger: AnimationTriggerMetadata): void { + registerTrigger(trigger: AnimationTriggerMetadata, name: string = null): void { const stateMap: {[stateName: string]: ɵStyleData} = {}; trigger.definitions.forEach(def => { if (def.type === AnimationMetadataType.State) { @@ -42,7 +42,8 @@ export class NoopAnimationEngine extends AnimationEngine { stateMap[stateDef.name] = normalizeStyles(stateDef.styles.styles); } }); - this._triggerStyles[trigger.name] = stateMap; + name = name || trigger.name; + this._triggerStyles[name] = stateMap; } onInsert(element: any, domFn: () => any): void { domFn(); } diff --git a/modules/@angular/platform-browser/src/dom/dom_renderer.ts b/modules/@angular/platform-browser/src/dom/dom_renderer.ts index 58b946b136..f367c2da36 100644 --- a/modules/@angular/platform-browser/src/dom/dom_renderer.ts +++ b/modules/@angular/platform-browser/src/dom/dom_renderer.ts @@ -385,6 +385,8 @@ export class DomRendererFactoryV2 implements RendererFactoryV2 { } class DefaultDomRendererV2 implements RendererV2 { + data: {[key: string]: any} = Object.create(null); + constructor(private eventManager: EventManager) {} destroy(): void {} diff --git a/modules/@angular/platform-browser/test/animation/animation_renderer_spec.ts b/modules/@angular/platform-browser/test/animation/animation_renderer_spec.ts index 96a2a5cc6d..361938be49 100644 --- a/modules/@angular/platform-browser/test/animation/animation_renderer_spec.ts +++ b/modules/@angular/platform-browser/test/animation/animation_renderer_spec.ts @@ -82,7 +82,7 @@ export function main() { expect(engine.captures['setProperty']).toBeFalsy(); renderer.setProperty(element, '@prop', 'value'); - expect(engine.captures['setProperty'].pop()).toEqual([element, 'prop', 'value']); + expect(engine.captures['setProperty'].pop()).toEqual([element, 'id#prop', 'value']); }); describe('listen', () => { @@ -96,7 +96,7 @@ export function main() { expect(engine.captures['listen']).toBeFalsy(); renderer.listen(element, '@event.phase', cb); - expect(engine.captures['listen'].pop()).toEqual([element, 'event', 'phase']); + expect(engine.captures['listen'].pop()).toEqual([element, 'id#event', 'phase']); }); it('should resolve the body|document|window nodes given their values as strings as input', diff --git a/modules/@angular/platform-server/src/server_renderer.ts b/modules/@angular/platform-server/src/server_renderer.ts index 7a456f5540..c597b4de94 100644 --- a/modules/@angular/platform-server/src/server_renderer.ts +++ b/modules/@angular/platform-server/src/server_renderer.ts @@ -306,6 +306,8 @@ export class ServerRendererFactoryV2 implements RendererFactoryV2 { } class DefaultServerRendererV2 implements RendererV2 { + data: {[key: string]: any} = Object.create(null); + constructor( private document: any, private ngZone: NgZone, private schema: DomElementSchemaRegistry) {} diff --git a/modules/@angular/platform-webworker/src/web_workers/worker/renderer.ts b/modules/@angular/platform-webworker/src/web_workers/worker/renderer.ts index 2e7c6a7051..b4ed900bd4 100644 --- a/modules/@angular/platform-webworker/src/web_workers/worker/renderer.ts +++ b/modules/@angular/platform-webworker/src/web_workers/worker/renderer.ts @@ -386,6 +386,8 @@ export class WebWorkerRendererFactoryV2 implements RendererFactoryV2 { export class WebWorkerRendererV2 implements RendererV2 { + data: {[key: string]: any} = Object.create(null); + constructor(private _rendererFactory: WebWorkerRendererFactoryV2) {} private asFnArg = new FnArg(this, SerializerTypes.RENDER_STORE_OBJECT); diff --git a/tools/public_api_guard/core/typings/core.d.ts b/tools/public_api_guard/core/typings/core.d.ts index 7a1d0d9695..c3ddf4b5f1 100644 --- a/tools/public_api_guard/core/typings/core.d.ts +++ b/tools/public_api_guard/core/typings/core.d.ts @@ -834,7 +834,7 @@ export declare abstract class RendererFactoryV2 { /** @experimental */ export interface RendererTypeV2 { data: { - [kind: string]: any[]; + [kind: string]: any; }; encapsulation: ViewEncapsulation; id: string; @@ -843,6 +843,9 @@ export interface RendererTypeV2 { /** @experimental */ export declare abstract class RendererV2 { + readonly abstract data: { + [key: string]: any; + }; destroyNode: (node: any) => void | null; abstract addClass(el: any, name: string): void; abstract appendChild(parent: any, newChild: any): void;