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
This commit is contained in:
Alex Rickabaugh 2018-11-20 17:20:16 +01:00 committed by Igor Minar
parent c331fc6f0c
commit 412e47d311
9 changed files with 55 additions and 28 deletions

View File

@ -23,6 +23,7 @@ import {ScopeDirective, SelectorScopeRegistry} from './selector_scope';
import {extractDirectiveGuards, isAngularCore, unwrapExpression} from './util'; import {extractDirectiveGuards, isAngularCore, unwrapExpression} from './util';
const EMPTY_MAP = new Map<string, Expression>(); const EMPTY_MAP = new Map<string, Expression>();
const EMPTY_ARRAY: any[] = [];
export interface ComponentHandlerData { export interface ComponentHandlerData {
meta: R3ComponentMetadata; meta: R3ComponentMetadata;
@ -208,7 +209,7 @@ export class ComponentDecoratorHandler implements
// These will be replaced during the compilation step, after all `NgModule`s have been // 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. // analyzed and the full compilation scope for the component can be realized.
pipes: EMPTY_MAP, pipes: EMPTY_MAP,
directives: EMPTY_MAP, directives: EMPTY_ARRAY,
wrapDirectivesAndPipesInClosure: false, // wrapDirectivesAndPipesInClosure: false, //
animations, animations,
viewProviders viewProviders
@ -225,7 +226,7 @@ export class ComponentDecoratorHandler implements
const matcher = new SelectorMatcher<ScopeDirective<any>>(); const matcher = new SelectorMatcher<ScopeDirective<any>>();
if (scope !== null) { if (scope !== null) {
scope.directives.forEach( 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); 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 // scope. This is possible now because during compile() the whole compilation unit has been
// fully analyzed. // fully analyzed.
const {pipes, containsForwardDecls} = scope; const {pipes, containsForwardDecls} = scope;
const directives = new Map<string, Expression>(); const directives: {selector: string, expression: Expression}[] = [];
scope.directives.forEach((meta, selector) => directives.set(selector, meta.directive)); scope.directives.forEach(
({selector, meta}) => directives.push({selector, expression: meta.directive}));
const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls; const wrapDirectivesAndPipesInClosure: boolean = !!containsForwardDecls;
metadata = {...metadata, directives, pipes, wrapDirectivesAndPipesInClosure}; metadata = {...metadata, directives, pipes, wrapDirectivesAndPipesInClosure};
} }

View File

@ -31,7 +31,7 @@ export interface ModuleData {
* context of some module. * context of some module.
*/ */
export interface CompilationScope<T> { export interface CompilationScope<T> {
directives: Map<string, ScopeDirective<T>>; directives: {selector: string, meta: ScopeDirective<T>}[];
pipes: Map<string, T>; pipes: Map<string, T>;
containsForwardDecls?: boolean; containsForwardDecls?: boolean;
} }
@ -153,7 +153,7 @@ export class SelectorScopeRegistry {
} }
// This is the first time the scope for this module is being computed. // This is the first time the scope for this module is being computed.
const directives = new Map<string, ScopeDirective<Reference<ts.Declaration>>>(); const directives: {selector: string, meta: ScopeDirective<Reference<ts.Declaration>>}[] = [];
const pipes = new Map<string, Reference>(); const pipes = new Map<string, Reference>();
// Process the declaration scope of the module, and lookup the selector of every declared type. // 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); const metadata = this.lookupDirectiveMetadata(ref);
// Only directives/components with selectors get added to the scope. // Only directives/components with selectors get added to the scope.
if (metadata != null) { if (metadata != null) {
directives.set(metadata.selector, {...metadata, directive: ref}); directives.push({selector: metadata.selector, meta: {...metadata, directive: ref}});
return; return;
} }
@ -418,18 +418,16 @@ function absoluteModuleName(ref: Reference): string|null {
return ref.moduleName; return ref.moduleName;
} }
function convertDirectiveReferenceMap( function convertDirectiveReferenceList(
map: Map<string, ScopeDirective<Reference>>, input: {selector: string, meta: ScopeDirective<Reference>}[],
context: ts.SourceFile): Map<string, ScopeDirective<Expression>> { context: ts.SourceFile): {selector: string, meta: ScopeDirective<Expression>}[] {
const newMap = new Map<string, ScopeDirective<Expression>>(); return input.map(({selector, meta}) => {
map.forEach((meta, selector) => {
const directive = meta.directive.toExpression(context); const directive = meta.directive.toExpression(context);
if (directive === null) { if (directive === null) {
throw new Error(`Could not write expression to reference ${meta.directive.node}`); 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( function convertPipeReferenceMap(
@ -448,13 +446,13 @@ function convertPipeReferenceMap(
function convertScopeToExpressions( function convertScopeToExpressions(
scope: CompilationScope<Reference>, context: ts.Declaration): CompilationScope<Expression> { scope: CompilationScope<Reference>, context: ts.Declaration): CompilationScope<Expression> {
const sourceContext = ts.getOriginalNode(context).getSourceFile(); 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 pipes = convertPipeReferenceMap(scope.pipes, sourceContext);
const declPointer = maybeUnwrapNameOfDeclaration(context); const declPointer = maybeUnwrapNameOfDeclaration(context);
let containsForwardDecls = false; let containsForwardDecls = false;
directives.forEach(expr => { directives.forEach(({selector, meta}) => {
containsForwardDecls = containsForwardDecls || containsForwardDecls = containsForwardDecls ||
isExpressionForwardReference(expr.directive, declPointer, sourceContext); isExpressionForwardReference(meta.directive, declPointer, sourceContext);
}); });
!containsForwardDecls && pipes.forEach(expr => { !containsForwardDecls && pipes.forEach(expr => {
containsForwardDecls = containsForwardDecls =

View File

@ -91,7 +91,7 @@ describe('SelectorScopeRegistry', () => {
const scope = registry.lookupCompilationScope(ProgramCmp) !; const scope = registry.lookupCompilationScope(ProgramCmp) !;
expect(scope).toBeDefined(); expect(scope).toBeDefined();
expect(scope.directives).toBeDefined(); expect(scope.directives).toBeDefined();
expect(scope.directives.size).toBe(2); expect(scope.directives.length).toBe(2);
}); });
it('exports of third-party libs work', () => { it('exports of third-party libs work', () => {
@ -162,6 +162,6 @@ describe('SelectorScopeRegistry', () => {
const scope = registry.lookupCompilationScope(ProgramCmp) !; const scope = registry.lookupCompilationScope(ProgramCmp) !;
expect(scope).toBeDefined(); expect(scope).toBeDefined();
expect(scope.directives).toBeDefined(); expect(scope.directives).toBeDefined();
expect(scope.directives.size).toBe(2); expect(scope.directives.length).toBe(2);
}); });
}); });

View File

@ -832,4 +832,31 @@ describe('ngtsc behavioral tests', () => {
expect(jsContents).toContain('ɵsetClassMetadata(TestNgModule, '); expect(jsContents).toContain('ɵsetClassMetadata(TestNgModule, ');
expect(jsContents).toContain('ɵsetClassMetadata(TestPipe, '); 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: '<div test></div>',
})
class Cmp {}
@NgModule({
declarations: [Cmp, DirA, DirB],
})
class Module {}
`);
env.driveMain();
const jsContents = env.getContents('test.js');
expect(jsContents).toMatch(/directives: \[DirA,\s+DirB\]/);
});
}); });

View File

@ -128,7 +128,7 @@ export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade {
animations: any[]|undefined; animations: any[]|undefined;
viewQueries: R3QueryMetadataFacade[]; viewQueries: R3QueryMetadataFacade[];
pipes: Map<string, any>; pipes: Map<string, any>;
directives: Map<string, any>; directives: {selector: string, expression: any}[];
styles: string[]; styles: string[];
encapsulation: ViewEncapsulation; encapsulation: ViewEncapsulation;
viewProviders: Provider[]|null; viewProviders: Provider[]|null;

View File

@ -153,10 +153,10 @@ export interface R3ComponentMetadata extends R3DirectiveMetadata {
pipes: Map<string, o.Expression>; pipes: Map<string, o.Expression>;
/** /**
* 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. * scope of the compilation.
*/ */
directives: Map<string, o.Expression>; directives: {selector: string, expression: o.Expression}[];
/** /**
* Whether to wrap the 'directives' and/or `pipes` array, if one is generated, in a closure. * Whether to wrap the 'directives' and/or `pipes` array, if one is generated, in a closure.

View File

@ -231,11 +231,11 @@ export function compileComponentFromMetadata(
// Generate the CSS matcher that recognize directive // Generate the CSS matcher that recognize directive
let directiveMatcher: SelectorMatcher|null = null; let directiveMatcher: SelectorMatcher|null = null;
if (meta.directives.size) { if (meta.directives.length > 0) {
const matcher = new SelectorMatcher(); const matcher = new SelectorMatcher();
meta.directives.forEach((expression, selector: string) => { for (const {selector, expression} of meta.directives) {
matcher.addSelectables(CssSelector.parse(selector), expression); matcher.addSelectables(CssSelector.parse(selector), expression);
}); }
directiveMatcher = matcher; directiveMatcher = matcher;
} }
@ -375,7 +375,7 @@ export function compileComponentFromRender2(
ngContentSelectors: render3Ast.ngContentSelectors, ngContentSelectors: render3Ast.ngContentSelectors,
relativeContextFilePath: '', relativeContextFilePath: '',
}, },
directives: typeMapToExpressionMap(directiveTypeBySel, outputCtx), directives: [],
pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx), pipes: typeMapToExpressionMap(pipeTypeByName, outputCtx),
viewQueries: queriesFromGlobalMetadata(component.viewQueries, outputCtx), viewQueries: queriesFromGlobalMetadata(component.viewQueries, outputCtx),
wrapDirectivesAndPipesInClosure: false, wrapDirectivesAndPipesInClosure: false,

View File

@ -128,7 +128,7 @@ export interface R3ComponentMetadataFacade extends R3DirectiveMetadataFacade {
animations: any[]|undefined; animations: any[]|undefined;
viewQueries: R3QueryMetadataFacade[]; viewQueries: R3QueryMetadataFacade[];
pipes: Map<string, any>; pipes: Map<string, any>;
directives: Map<string, any>; directives: {selector: string, expression: any}[];
styles: string[]; styles: string[];
encapsulation: ViewEncapsulation; encapsulation: ViewEncapsulation;
viewProviders: Provider[]|null; viewProviders: Provider[]|null;

View File

@ -58,7 +58,7 @@ export function compileComponent(type: Type<any>, metadata: Component): void {
styles: metadata.styles || EMPTY_ARRAY, styles: metadata.styles || EMPTY_ARRAY,
animations: metadata.animations, animations: metadata.animations,
viewQueries: extractQueriesMetadata(getReflect().propMetadata(type), isViewQuery), viewQueries: extractQueriesMetadata(getReflect().propMetadata(type), isViewQuery),
directives: new Map(), directives: [],
pipes: new Map(), pipes: new Map(),
encapsulation: metadata.encapsulation || ViewEncapsulation.Emulated, encapsulation: metadata.encapsulation || ViewEncapsulation.Emulated,
viewProviders: metadata.viewProviders || null, viewProviders: metadata.viewProviders || null,