From dfdaaf6a0d52140285c4869bec80b0bd0f130598 Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Tue, 4 Dec 2018 09:42:44 -0800 Subject: [PATCH] fix(ivy): deduplicate directives in component scopes (#27462) A previous fix to ngtsc opened the door for duplicate directives in the 'directives' array of a component. This would happen if the directive was declared in a module which was imported more than once within the component's module. This commit adds deduplication when the component's scope is materialized, so declarations which arrive via more than one module import are coalesced. PR Close #27462 --- .../src/ngtsc/annotations/src/component.ts | 11 +++-- .../ngtsc/annotations/src/selector_scope.ts | 45 +++++++++++-------- .../test/ngtsc/fake_core/index.ts | 5 ++- .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 36 +++++++++++++++ 4 files changed, 74 insertions(+), 23 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index ffa0da9954..8631e89645 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -266,8 +266,9 @@ export class ComponentDecoratorHandler implements const scope = this.scopeRegistry.lookupCompilationScopeAsRefs(node); const matcher = new SelectorMatcher>(); if (scope !== null) { - scope.directives.forEach( - ({selector, meta}) => { matcher.addSelectables(CssSelector.parse(selector), meta); }); + for (const meta of scope.directives) { + matcher.addSelectables(CssSelector.parse(meta.selector), meta); + } ctx.addTemplate(node as ts.ClassDeclaration, meta.parsedTemplate, matcher); } } @@ -284,8 +285,10 @@ export class ComponentDecoratorHandler implements // fully analyzed. const {pipes, containsForwardDecls} = scope; const directives: {selector: string, expression: Expression}[] = []; - scope.directives.forEach( - ({selector, meta}) => directives.push({selector, expression: meta.directive})); + + for (const meta of scope.directives) { + directives.push({selector: meta.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 834635b021..2688d1a82a 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: {selector: string, meta: ScopeDirective}[]; + directives: ScopeDirective[]; pipes: Map; containsForwardDecls?: boolean; } @@ -153,28 +153,38 @@ export class SelectorScopeRegistry { } // This is the first time the scope for this module is being computed. - const directives: {selector: string, meta: ScopeDirective>}[] = []; - const pipes = new Map(); + const directives: ScopeDirective>[] = []; + const pipes = new Map>(); + + // Tracks which declarations already appear in the `CompilationScope`. + const seenSet = new Set(); // Process the declaration scope of the module, and lookup the selector of every declared type. // The initial value of ngModuleImportedFrom is 'null' which signifies that the NgModule // was not imported from a .d.ts source. - this.lookupScopesOrDie(module !, /* ngModuleImportedFrom */ null).compilation.forEach(ref => { + for (const ref of this.lookupScopesOrDie(module !, /* ngModuleImportedFrom */ null) + .compilation) { const node = ts.getOriginalNode(ref.node) as ts.Declaration; + // Track whether this `ts.Declaration` has been seen before. + if (seenSet.has(node)) { + continue; + } else { + seenSet.add(node); + } + // Either the node represents a directive or a pipe. Look for both. const metadata = this.lookupDirectiveMetadata(ref); // Only directives/components with selectors get added to the scope. - if (metadata != null) { - directives.push({selector: metadata.selector, meta: {...metadata, directive: ref}}); - return; + if (metadata !== null) { + directives.push({...metadata, directive: ref}); + } else { + const name = this.lookupPipeName(node); + if (name !== null) { + pipes.set(name, ref); + } } - - const name = this.lookupPipeName(node); - if (name != null) { - pipes.set(name, ref); - } - }); + } const scope: CompilationScope = {directives, pipes}; @@ -419,14 +429,13 @@ function absoluteModuleName(ref: Reference): string|null { } function convertDirectiveReferenceList( - input: {selector: string, meta: ScopeDirective}[], - context: ts.SourceFile): {selector: string, meta: ScopeDirective}[] { - return input.map(({selector, meta}) => { + input: ScopeDirective[], context: ts.SourceFile): ScopeDirective[] { + return input.map(meta => { const directive = meta.directive.toExpression(context); if (directive === null) { throw new Error(`Could not write expression to reference ${meta.directive.node}`); } - return {selector, meta: {...meta, directive}}; + return {...meta, directive}; }); } @@ -450,7 +459,7 @@ function convertScopeToExpressions( const pipes = convertPipeReferenceMap(scope.pipes, sourceContext); const declPointer = maybeUnwrapNameOfDeclaration(context); let containsForwardDecls = false; - directives.forEach(({selector, meta}) => { + directives.forEach(meta => { containsForwardDecls = containsForwardDecls || isExpressionForwardReference(meta.directive, declPointer, sourceContext); }); diff --git a/packages/compiler-cli/test/ngtsc/fake_core/index.ts b/packages/compiler-cli/test/ngtsc/fake_core/index.ts index ea784dec32..831831738d 100644 --- a/packages/compiler-cli/test/ngtsc/fake_core/index.ts +++ b/packages/compiler-cli/test/ngtsc/fake_core/index.ts @@ -60,4 +60,7 @@ export function forwardRef(fn: () => T): T { return fn(); } -export interface SimpleChanges { [propName: string]: any; } \ No newline at end of file +export interface SimpleChanges { [propName: string]: any; } + +export type ɵNgModuleDefWithMeta = any; +export type ɵDirectiveDefWithMeta = any; diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 79192f1d01..c3d79bb7fc 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -1078,4 +1078,40 @@ describe('ngtsc behavioral tests', () => { // Success is enough to indicate that this passes. }); + + it('should not emit multiple references to the same directive', () => { + env.tsconfig(); + env.write('node_modules/external/index.d.ts', ` + import {ɵDirectiveDefWithMeta, ɵNgModuleDefWithMeta} from '@angular/core'; + + export declare class ExternalDir { + static ngDirectiveDef: ɵDirectiveDefWithMeta; + } + + export declare class ExternalModule { + static ngModuleDef: ɵNgModuleDefWithMeta; + } + `); + env.write('test.ts', ` + import {Component, Directive, NgModule} from '@angular/core'; + import {ExternalModule} from 'external'; + + @Component({ + template: '
', + }) + class Cmp {} + + @NgModule({ + declarations: [Cmp], + // Multiple imports of the same module used to result in duplicate directive references + // in the output. + imports: [ExternalModule, ExternalModule], + }) + class Module {} + `); + + env.driveMain(); + const jsContents = env.getContents('test.js'); + expect(jsContents).toMatch(/directives: \[i1\.ExternalDir\]/); + }); });