From 79eda30f0f3aeac58d488ebc51690190c19cf7e5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matias=20Niemel=C3=A4?= Date: Sat, 9 Jul 2016 00:43:17 -0700 Subject: [PATCH] refactor(animations): collect parser / lookup errors in the same place --- .../src/animation/animation_compiler.ts | 73 +++++++++++++++++-- .../src/view_compiler/property_binder.ts | 6 +- .../src/view_compiler/view_builder.ts | 2 +- .../src/view_compiler/view_compiler.ts | 2 +- .../test/animation/animation_compiler_spec.ts | 2 +- .../animation/animation_integration_spec.ts | 26 ++++++- 6 files changed, 96 insertions(+), 15 deletions(-) diff --git a/modules/@angular/compiler/src/animation/animation_compiler.ts b/modules/@angular/compiler/src/animation/animation_compiler.ts index 5c63830fb8..a16b256865 100644 --- a/modules/@angular/compiler/src/animation/animation_compiler.ts +++ b/modules/@angular/compiler/src/animation/animation_compiler.ts @@ -15,6 +15,7 @@ import {BaseException} from '../facade/exceptions'; import {isArray, isBlank, isPresent} from '../facade/lang'; import {Identifiers} from '../identifiers'; import * as o from '../output/output_ast'; +import {PropertyBindingType, TemplateAst, TemplateAstVisitor, NgContentAst, EmbeddedTemplateAst, ElementAst, ReferenceAst, VariableAst, BoundEventAst, BoundElementPropertyAst, AttrAst, BoundTextAst, TextAst, DirectiveAst, BoundDirectivePropertyAst, templateVisitAll,} from '../template_ast'; import {AnimationAst, AnimationAstVisitor, AnimationEntryAst, AnimationGroupAst, AnimationKeyframeAst, AnimationSequenceAst, AnimationStateAst, AnimationStateDeclarationAst, AnimationStateTransitionAst, AnimationStepAst, AnimationStylesAst} from './animation_ast'; import {AnimationParseError, ParsedAnimationResult, parseAnimationEntry} from './animation_parser'; @@ -27,19 +28,21 @@ export class CompiledAnimation { } export class AnimationCompiler { - compileComponent(component: CompileDirectiveMetadata): CompiledAnimation[] { + compileComponent(component: CompileDirectiveMetadata, template: TemplateAst[]): + CompiledAnimation[] { var compiledAnimations: CompiledAnimation[] = []; var index = 0; + var groupedErrors: string[] = []; + component.template.animations.forEach(entry => { var result = parseAnimationEntry(entry); if (result.errors.length > 0) { - var errorMessage = ''; + var errorMessage = + `Unable to parse the animation sequence for "${entry.name}" due to the following errors:`; result.errors.forEach( - (error: AnimationParseError) => { errorMessage += '\n- ' + error.msg; }); + (error: AnimationParseError) => { errorMessage += '\n-- ' + error.msg; }); // todo (matsko): include the component name when throwing - throw new BaseException( - `Unable to parse the animation sequence for "${entry.name}" due to the following errors: ` + - errorMessage); + groupedErrors.push(errorMessage); } var factoryName = `${component.type.name}_${entry.name}_${index}`; @@ -48,6 +51,18 @@ export class AnimationCompiler { var visitor = new _AnimationBuilder(entry.name, factoryName); compiledAnimations.push(visitor.build(result.ast)); }); + + _validateAnimationProperties(compiledAnimations, template).forEach(entry => { + groupedErrors.push(entry.msg); + }); + + if (groupedErrors.length > 0) { + var errorMessageStr = + `Animation parsing for ${component.type.name} has failed due to the following errors:`; + groupedErrors.forEach(error => errorMessageStr += `\n- ${error}`); + throw new BaseException(errorMessageStr); + } + return compiledAnimations; } } @@ -360,3 +375,49 @@ function _isEndStateAnimateStep(step: AnimationAst): boolean { function _getStylesArray(obj: any): {[key: string]: any}[] { return obj.styles.styles; } + +function _validateAnimationProperties( + compiledAnimations: CompiledAnimation[], template: TemplateAst[]): AnimationParseError[] { + var visitor = new _AnimationTemplatePropertyVisitor(compiledAnimations); + templateVisitAll(visitor, template); + return visitor.errors; +} + +class _AnimationTemplatePropertyVisitor implements TemplateAstVisitor { + private _nodeIndex: number = 0; + private _animationRegistry: {[key: string]: boolean} = {}; + + public errors: AnimationParseError[] = []; + + constructor(animations: CompiledAnimation[]) { + animations.forEach(entry => { this._animationRegistry[entry.name] = true; }); + } + + visitElement(ast: ElementAst, ctx: any): any { + ast.inputs.forEach(input => { + if (input.type == PropertyBindingType.Animation) { + var animationName = input.name; + if (!isPresent(this._animationRegistry[animationName])) { + this.errors.push( + new AnimationParseError(`couldn't find an animation entry for ${animationName}`)); + } + } + }); + templateVisitAll(this, ast.children); + } + + visitBoundText(ast: BoundTextAst, ctx: any): any { this._nodeIndex++; } + + visitText(ast: TextAst, ctx: any): any { this._nodeIndex++; } + + visitEmbeddedTemplate(ast: EmbeddedTemplateAst, ctx: any): any { this._nodeIndex++; } + + visitNgContent(ast: NgContentAst, ctx: any): any {} + visitAttr(ast: AttrAst, ctx: any): any {} + visitDirective(ast: DirectiveAst, ctx: any): any {} + visitEvent(ast: BoundEventAst, ctx: any): any {} + visitReference(ast: ReferenceAst, ctx: any): any {} + visitVariable(ast: VariableAst, ctx: any): any {} + visitDirectiveProperty(ast: BoundDirectivePropertyAst, ctx: any): any {} + visitElementProperty(ast: BoundElementPropertyAst, ctx: any): any {} +} diff --git a/modules/@angular/compiler/src/view_compiler/property_binder.ts b/modules/@angular/compiler/src/view_compiler/property_binder.ts index 7287a47031..895cf031e8 100644 --- a/modules/@angular/compiler/src/view_compiler/property_binder.ts +++ b/modules/@angular/compiler/src/view_compiler/property_binder.ts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.io/license */ +import {BaseException, SecurityContext} from '@angular/core'; + import {EMPTY_STATE as EMPTY_ANIMATION_STATE, LifecycleHooks, isDefaultChangeDetectionStrategy} from '../../core_private'; import * as cdAst from '../expression_parser/ast'; import {isBlank, isPresent} from '../facade/lang'; @@ -136,10 +138,6 @@ function bindAndWriteToRenderer( case PropertyBindingType.Animation: var animationName = boundProp.name; var animation = view.componentView.animations.get(animationName); - if (!isPresent(animation)) { - throw new BaseException( - `Internal Error: couldn't find an animation entry for ${boundProp.name}`); - } // it's important to normalize the void value as `void` explicitly // so that the styles data can be obtained from the stringmap diff --git a/modules/@angular/compiler/src/view_compiler/view_builder.ts b/modules/@angular/compiler/src/view_compiler/view_builder.ts index b07a20bd42..0990c225a7 100644 --- a/modules/@angular/compiler/src/view_compiler/view_builder.ts +++ b/modules/@angular/compiler/src/view_compiler/view_builder.ts @@ -275,7 +275,7 @@ class ViewBuilderVisitor implements TemplateAstVisitor { ast.hasViewContainer, true, ast.references); this.view.nodes.push(compileElement); - var compiledAnimations = this._animationCompiler.compileComponent(this.view.component); + var compiledAnimations = this._animationCompiler.compileComponent(this.view.component, [ast]); this.nestedViewCount++; var embeddedView = new CompileView( diff --git a/modules/@angular/compiler/src/view_compiler/view_compiler.ts b/modules/@angular/compiler/src/view_compiler/view_compiler.ts index d43c22a494..2ba1c11942 100644 --- a/modules/@angular/compiler/src/view_compiler/view_compiler.ts +++ b/modules/@angular/compiler/src/view_compiler/view_compiler.ts @@ -36,7 +36,7 @@ export class ViewCompiler { component: CompileDirectiveMetadata, template: TemplateAst[], styles: o.Expression, pipes: CompilePipeMetadata[]): ViewCompileResult { var dependencies: Array = []; - var compiledAnimations = this._animationCompiler.compileComponent(component); + var compiledAnimations = this._animationCompiler.compileComponent(component, template); var statements: o.Statement[] = []; compiledAnimations.map(entry => { statements.push(entry.statesMapStatement); diff --git a/modules/@angular/compiler/test/animation/animation_compiler_spec.ts b/modules/@angular/compiler/test/animation/animation_compiler_spec.ts index 42448c7749..26b32a10f2 100644 --- a/modules/@angular/compiler/test/animation/animation_compiler_spec.ts +++ b/modules/@angular/compiler/test/animation/animation_compiler_spec.ts @@ -22,7 +22,7 @@ export function main() { var compiler = new AnimationCompiler(); var compileAnimations = (component: CompileDirectiveMetadata): CompiledAnimation => { - return compiler.compileComponent(component)[0]; + return compiler.compileComponent(component, [])[0]; }; var compile = (seq: AnimationMetadata) => { diff --git a/modules/@angular/core/test/animation/animation_integration_spec.ts b/modules/@angular/core/test/animation/animation_integration_spec.ts index 232dd8d1d9..67917d75d9 100644 --- a/modules/@angular/core/test/animation/animation_integration_spec.ts +++ b/modules/@angular/core/test/animation/animation_integration_spec.ts @@ -13,6 +13,7 @@ import {AnimationDriver} from '@angular/platform-browser/src/dom/animation_drive import {getDOM} from '@angular/platform-browser/src/dom/dom_adapter'; import {MockAnimationDriver} from '@angular/platform-browser/testing/mock_animation_driver'; +import {BaseException} from '../../../compiler/src/facade/exceptions'; import {Component} from '../../index'; import {DEFAULT_STATE} from '../../src/animation/animation_constants'; import {AnimationKeyframe} from '../../src/animation/animation_keyframe'; @@ -44,12 +45,15 @@ function declareTests({useJit}: {useJit: boolean}) { var makeAnimationCmp = (tcb: TestComponentBuilder, tpl: string, animationEntry: AnimationEntryMetadata | AnimationEntryMetadata[], - callback: any /** TODO #9100 */ = null) => { + callback: Function = null, failure: Function = null) => { var entries = isArray(animationEntry) ? animationEntry : [animationEntry]; tcb = tcb.overrideTemplate(DummyIfCmp, tpl); tcb = tcb.overrideAnimations(DummyIfCmp, entries); - tcb.createAsync(DummyIfCmp).then((root) => { callback(root); }); + var promise = tcb.createAsync(DummyIfCmp).then((root) => { callback(root); }); + if (isPresent(failure)) { + promise.catch(failure); + } tick(); }; @@ -878,6 +882,24 @@ function declareTests({useJit}: {useJit: boolean}) { }); describe('animation states', () => { + it('should throw an error when an animation is referenced that isn\'t defined within the component annotation', + inject( + [TestComponentBuilder, AnimationDriver], + fakeAsync((tcb: TestComponentBuilder, driver: MockAnimationDriver) => { + makeAnimationCmp( + tcb, '
', [], + () => { + throw new BaseException( + 'Error: expected animations for DummyIfCmp to throw an error within this spec'); + }, + (e: any) => { + var message = e.message; + expect(message).toMatch( + /Animation parsing for DummyIfCmp has failed due to the following errors:/); + expect(message).toMatch(/- couldn't find an animation entry for status/); + }); + }))); + it('should retain the destination animation state styles once the animation is complete', inject( [TestComponentBuilder, AnimationDriver],