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
This commit is contained in:
Alex Rickabaugh 2018-12-04 09:42:44 -08:00 committed by Igor Minar
parent b82f62a11d
commit dfdaaf6a0d
4 changed files with 74 additions and 23 deletions

View File

@ -266,8 +266,9 @@ export class ComponentDecoratorHandler implements
const scope = this.scopeRegistry.lookupCompilationScopeAsRefs(node);
const matcher = new SelectorMatcher<ScopeDirective<any>>();
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};
}

View File

@ -31,7 +31,7 @@ export interface ModuleData {
* context of some module.
*/
export interface CompilationScope<T> {
directives: {selector: string, meta: ScopeDirective<T>}[];
directives: ScopeDirective<T>[];
pipes: Map<string, T>;
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<Reference<ts.Declaration>>}[] = [];
const pipes = new Map<string, Reference>();
const directives: ScopeDirective<Reference<ts.Declaration>>[] = [];
const pipes = new Map<string, Reference<ts.Declaration>>();
// Tracks which declarations already appear in the `CompilationScope`.
const seenSet = new Set<ts.Declaration>();
// 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<Reference> = {directives, pipes};
@ -419,14 +429,13 @@ function absoluteModuleName(ref: Reference): string|null {
}
function convertDirectiveReferenceList(
input: {selector: string, meta: ScopeDirective<Reference>}[],
context: ts.SourceFile): {selector: string, meta: ScopeDirective<Expression>}[] {
return input.map(({selector, meta}) => {
input: ScopeDirective<Reference>[], context: ts.SourceFile): ScopeDirective<Expression>[] {
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);
});

View File

@ -60,4 +60,7 @@ export function forwardRef<T>(fn: () => T): T {
return fn();
}
export interface SimpleChanges { [propName: string]: any; }
export interface SimpleChanges { [propName: string]: any; }
export type ɵNgModuleDefWithMeta<ModuleT, DeclarationsT, ImportsT, ExportsT> = any;
export type ɵDirectiveDefWithMeta<DirT, SelectorT, ExportAsT, InputsT, OutputsT, QueriesT> = any;

View File

@ -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<ExternalDir, '[test]', never, never, never, never>;
}
export declare class ExternalModule {
static ngModuleDef: ɵNgModuleDefWithMeta<ExternalModule, [typeof ExternalDir], never, [typeof ExternalDir]>;
}
`);
env.write('test.ts', `
import {Component, Directive, NgModule} from '@angular/core';
import {ExternalModule} from 'external';
@Component({
template: '<div test></div>',
})
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\]/);
});
});