From 412e47d3119f131aa1dbb114a8c1e4beeb76ffce Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 20 Nov 2018 17:20:16 +0100 Subject: [PATCH] fix(ivy): support multiple directives with the same selector (#27298) Previously the concept of multiple directives with the same selector was not supported by ngtsc. This is due to the treatment of directives for a component as a Map from selector to the directive, which is an erroneous representation. Now the directives for a component are stored as an array which supports multiple directives with the same selector. Testing strategy: a new ngtsc_spec test asserts that multiple directives with the same selector are matched on an element. PR Close #27298 --- .../src/ngtsc/annotations/src/component.ts | 10 ++++--- .../ngtsc/annotations/src/selector_scope.ts | 24 ++++++++--------- .../annotations/test/selector_scope_spec.ts | 4 +-- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 27 +++++++++++++++++++ .../compiler/src/compiler_facade_interface.ts | 2 +- packages/compiler/src/render3/view/api.ts | 4 +-- .../compiler/src/render3/view/compiler.ts | 8 +++--- .../render3/jit/compiler_facade_interface.ts | 2 +- packages/core/src/render3/jit/directive.ts | 2 +- 9 files changed, 55 insertions(+), 28 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index a3bbbf896d..e859f06ce9 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -23,6 +23,7 @@ import {ScopeDirective, SelectorScopeRegistry} from './selector_scope'; import {extractDirectiveGuards, isAngularCore, unwrapExpression} from './util'; const EMPTY_MAP = new Map(); +const EMPTY_ARRAY: any[] = []; export interface ComponentHandlerData { meta: R3ComponentMetadata; @@ -208,7 +209,7 @@ export class ComponentDecoratorHandler implements // These will be replaced during the compilation step, after all `NgModule`s have been // analyzed and the full compilation scope for the component can be realized. pipes: EMPTY_MAP, - directives: EMPTY_MAP, + directives: EMPTY_ARRAY, wrapDirectivesAndPipesInClosure: false, // animations, viewProviders @@ -225,7 +226,7 @@ export class ComponentDecoratorHandler implements const matcher = new SelectorMatcher>(); if (scope !== null) { scope.directives.forEach( - (meta, selector) => { matcher.addSelectables(CssSelector.parse(selector), meta); }); + ({selector, meta}) => { matcher.addSelectables(CssSelector.parse(selector), meta); }); ctx.addTemplate(node as ts.ClassDeclaration, meta.parsedTemplate, matcher); } } @@ -241,8 +242,9 @@ export class ComponentDecoratorHandler implements // scope. This is possible now because during compile() the whole compilation unit has been // fully analyzed. const {pipes, containsForwardDecls} = scope; - const directives = new Map(); - scope.directives.forEach((meta, selector) => directives.set(selector, meta.directive)); + const directives: {selector: string, expression: Expression}[] = []; + scope.directives.forEach( + ({selector, meta}) => directives.push({selector, expression: meta.directive})); const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls; metadata = {...metadata, directives, pipes, wrapDirectivesAndPipesInClosure}; } diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts index 4f41fc09ea..834635b021 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/selector_scope.ts @@ -31,7 +31,7 @@ export interface ModuleData { * context of some module. */ export interface CompilationScope { - directives: Map>; + directives: {selector: string, meta: ScopeDirective}[]; pipes: Map; containsForwardDecls?: boolean; } @@ -153,7 +153,7 @@ export class SelectorScopeRegistry { } // This is the first time the scope for this module is being computed. - const directives = new Map>>(); + const directives: {selector: string, meta: ScopeDirective>}[] = []; const pipes = new Map(); // Process the declaration scope of the module, and lookup the selector of every declared type. @@ -166,7 +166,7 @@ export class SelectorScopeRegistry { const metadata = this.lookupDirectiveMetadata(ref); // Only directives/components with selectors get added to the scope. if (metadata != null) { - directives.set(metadata.selector, {...metadata, directive: ref}); + directives.push({selector: metadata.selector, meta: {...metadata, directive: ref}}); return; } @@ -418,18 +418,16 @@ function absoluteModuleName(ref: Reference): string|null { return ref.moduleName; } -function convertDirectiveReferenceMap( - map: Map>, - context: ts.SourceFile): Map> { - const newMap = new Map>(); - map.forEach((meta, selector) => { +function convertDirectiveReferenceList( + input: {selector: string, meta: ScopeDirective}[], + context: ts.SourceFile): {selector: string, meta: ScopeDirective}[] { + return input.map(({selector, meta}) => { const directive = meta.directive.toExpression(context); if (directive === null) { throw new Error(`Could not write expression to reference ${meta.directive.node}`); } - newMap.set(selector, {...meta, directive}); + return {selector, meta: {...meta, directive}}; }); - return newMap; } function convertPipeReferenceMap( @@ -448,13 +446,13 @@ function convertPipeReferenceMap( function convertScopeToExpressions( scope: CompilationScope, context: ts.Declaration): CompilationScope { const sourceContext = ts.getOriginalNode(context).getSourceFile(); - const directives = convertDirectiveReferenceMap(scope.directives, sourceContext); + const directives = convertDirectiveReferenceList(scope.directives, sourceContext); const pipes = convertPipeReferenceMap(scope.pipes, sourceContext); const declPointer = maybeUnwrapNameOfDeclaration(context); let containsForwardDecls = false; - directives.forEach(expr => { + directives.forEach(({selector, meta}) => { containsForwardDecls = containsForwardDecls || - isExpressionForwardReference(expr.directive, declPointer, sourceContext); + isExpressionForwardReference(meta.directive, declPointer, sourceContext); }); !containsForwardDecls && pipes.forEach(expr => { containsForwardDecls = diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/selector_scope_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/selector_scope_spec.ts index 94de1771e6..b926dfba15 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/selector_scope_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/selector_scope_spec.ts @@ -91,7 +91,7 @@ describe('SelectorScopeRegistry', () => { const scope = registry.lookupCompilationScope(ProgramCmp) !; expect(scope).toBeDefined(); expect(scope.directives).toBeDefined(); - expect(scope.directives.size).toBe(2); + expect(scope.directives.length).toBe(2); }); it('exports of third-party libs work', () => { @@ -162,6 +162,6 @@ describe('SelectorScopeRegistry', () => { const scope = registry.lookupCompilationScope(ProgramCmp) !; expect(scope).toBeDefined(); expect(scope.directives).toBeDefined(); - expect(scope.directives.size).toBe(2); + expect(scope.directives.length).toBe(2); }); }); \ No newline at end of file diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index a7d1bd4c73..11daa8dd8a 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -832,4 +832,31 @@ describe('ngtsc behavioral tests', () => { expect(jsContents).toContain('ɵsetClassMetadata(TestNgModule, '); expect(jsContents).toContain('ɵsetClassMetadata(TestPipe, '); }); + + it('should compile a template using multiple directives with the same selector', () => { + env.tsconfig(); + env.write('test.ts', ` + import {Component, Directive, NgModule} from '@angular/core'; + + @Directive({selector: '[test]'}) + class DirA {} + + @Directive({selector: '[test]'}) + class DirB {} + + @Component({ + template: '
', + }) + class Cmp {} + + @NgModule({ + declarations: [Cmp, DirA, DirB], + }) + class Module {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/directives: \[DirA,\s+DirB\]/); + }); }); diff --git a/packages/compiler/src/compiler_facade_interface.ts b/packages/compiler/src/compiler_facade_interface.ts index c23539ddd2..0c7b004d8f 100644 --- a/packages/compiler/src/compiler_facade_interface.ts +++ b/packages/compiler/src/compiler_facade_interface.ts @@ -128,7 +128,7 @@ export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade { animations: any[]|undefined; viewQueries: R3QueryMetadataFacade[]; pipes: Map; - directives: Map; + directives: {selector: string, expression: any}[]; styles: string[]; encapsulation: ViewEncapsulation; viewProviders: Provider[]|null; diff --git a/packages/compiler/src/render3/view/api.ts b/packages/compiler/src/render3/view/api.ts index 89f032c6f0..9530ffabdb 100644 --- a/packages/compiler/src/render3/view/api.ts +++ b/packages/compiler/src/render3/view/api.ts @@ -153,10 +153,10 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata { pipes: Map; /** - * A map of directive selectors to an expression referencing the directive type which are in the + * A list of directive selectors and an expression referencing the directive type which are in the * scope of the compilation. */ - directives: Map; + directives: {selector: string, expression: o.Expression}[]; /** * Whether to wrap the 'directives' and/or `pipes` array, if one is generated, in a closure. diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index ce8005ac94..759e82bf0e 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -231,11 +231,11 @@ export function compileComponentFromMetadata( // Generate the CSS matcher that recognize directive let directiveMatcher: SelectorMatcher|null = null; - if (meta.directives.size) { + if (meta.directives.length > 0) { const matcher = new SelectorMatcher(); - meta.directives.forEach((expression, selector: string) => { + for (const {selector, expression} of meta.directives) { matcher.addSelectables(CssSelector.parse(selector), expression); - }); + } directiveMatcher = matcher; } @@ -375,7 +375,7 @@ export function compileComponentFromRender2( ngContentSelectors: render3Ast.ngContentSelectors, relativeContextFilePath: '', }, - directives: typeMapToExpressionMap(directiveTypeBySel, outputCtx), + directives: [], pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx), viewQueries: queriesFromGlobalMetadata(component.viewQueries, outputCtx), wrapDirectivesAndPipesInClosure: false, diff --git a/packages/core/src/render3/jit/compiler_facade_interface.ts b/packages/core/src/render3/jit/compiler_facade_interface.ts index c23539ddd2..0c7b004d8f 100644 --- a/packages/core/src/render3/jit/compiler_facade_interface.ts +++ b/packages/core/src/render3/jit/compiler_facade_interface.ts @@ -128,7 +128,7 @@ export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade { animations: any[]|undefined; viewQueries: R3QueryMetadataFacade[]; pipes: Map; - directives: Map; + directives: {selector: string, expression: any}[]; styles: string[]; encapsulation: ViewEncapsulation; viewProviders: Provider[]|null; diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index c69ab96bb0..a08686f325 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -58,7 +58,7 @@ export function compileComponent(type: Type, metadata: Component): void { styles: metadata.styles || EMPTY_ARRAY, animations: metadata.animations, viewQueries: extractQueriesMetadata(getReflect().propMetadata(type), isViewQuery), - directives: new Map(), + directives: [], pipes: new Map(), encapsulation: metadata.encapsulation || ViewEncapsulation.Emulated, viewProviders: metadata.viewProviders || null,