From 52552f9ce80696dfadd25a1fd345e65971d9ae5e Mon Sep 17 00:00:00 2001 From: JoostK Date: Sun, 13 Oct 2019 17:00:13 +0200 Subject: [PATCH] fix(ivy): support abstract directives in template type checking Recently it was made possible to have a directive without selector, which are referred to as abstract directives. Such directives should not be registered in an NgModule, but can still contain decorators for inputs, outputs, queries, etc. The information from these decorators and the `@Directive()` decorator itself needs to be registered with the central `MetadataRegistry` so that other areas of the compiler can request information about a given directive, an example of which is the template type checker that needs to know about the inputs and outputs of directives. Prior to this change, however, abstract directives would only register themselves with the `MetadataRegistry` as being an abstract directive, without all of its other metadata like inputs and outputs. This meant that the template type checker was unable to resolve the inputs and outputs of these abstract directives, therefore failing to check them correctly. The typical error would be that some property does not exist on a DOM element, whereas said property should have been bound to the abstract directive's input. This commit fixes the problem by always registering the metadata of a directive or component with the `MetadataRegistry`. Tests have been added to ensure abstract directives are handled correctly in the template type checker, together with tests to verify the form of abstract directives in declaration files. Fixes #30080 --- .../src/ngtsc/annotations/src/component.ts | 50 ++++++------ .../src/ngtsc/annotations/src/directive.ts | 37 ++++----- .../src/ngtsc/annotations/src/ng_module.ts | 4 +- .../src/ngtsc/incremental/src/state.ts | 14 ---- .../src/ngtsc/indexer/src/context.ts | 4 +- .../src/ngtsc/metadata/src/api.ts | 6 +- .../src/ngtsc/metadata/src/dts.ts | 12 +-- .../src/ngtsc/metadata/src/registry.ts | 11 --- .../src/ngtsc/metadata/src/util.ts | 4 - .../compiler-cli/src/ngtsc/scope/src/local.ts | 2 - .../compiler-cli/test/ngtsc/ngtsc_spec.ts | 44 +++++++++++ .../test/ngtsc/template_typecheck_spec.ts | 78 +++++++++++++++++-- .../compiler/src/render3/view/compiler.ts | 8 +- packages/core/src/render3/jit/directive.ts | 2 +- 14 files changed, 168 insertions(+), 108 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 62b3571031f1f..4578da8b48e02 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -216,24 +216,20 @@ export class ComponentDecoratorHandler implements `Errors parsing template: ${template.errors.map(e => e.toString()).join(', ')}`); } - // If the component has a selector, it should be registered with the - // `LocalModuleScopeRegistry` - // so that when this component appears in an `@NgModule` scope, its selector can be - // determined. - if (metadata.selector !== null) { - const ref = new Reference(node); - this.metaRegistry.registerDirectiveMetadata({ - ref, - name: node.name.text, - selector: metadata.selector, - exportAs: metadata.exportAs, - inputs: metadata.inputs, - outputs: metadata.outputs, - queries: metadata.queries.map(query => query.propertyName), - isComponent: true, ...extractDirectiveGuards(node, this.reflector), - baseClass: readBaseClass(node, this.reflector, this.evaluator), - }); - } + // Register this component's information with the `MetadataRegistry`. This ensures that + // the information about the component is available during the compile() phase. + const ref = new Reference(node); + this.metaRegistry.registerDirectiveMetadata({ + ref, + name: node.name.text, + selector: metadata.selector, + exportAs: metadata.exportAs, + inputs: metadata.inputs, + outputs: metadata.outputs, + queries: metadata.queries.map(query => query.propertyName), + isComponent: true, ...extractDirectiveGuards(node, this.reflector), + baseClass: readBaseClass(node, this.reflector, this.evaluator), + }); // Figure out the set of styles. The ordering here is important: external resources (styleUrls) // precede inline styles, and styles defined in the template override styles defined in the @@ -325,7 +321,9 @@ export class ComponentDecoratorHandler implements const matcher = new SelectorMatcher(); if (scope !== null) { for (const directive of scope.compilation.directives) { - matcher.addSelectables(CssSelector.parse(directive.selector), directive); + if (directive.selector !== null) { + matcher.addSelectables(CssSelector.parse(directive.selector), directive); + } } } const binder = new R3TargetBinder(matcher); @@ -368,8 +366,10 @@ export class ComponentDecoratorHandler implements const scope = this.scopeReader.getScopeForComponent(node); if (scope !== null) { for (const meta of scope.compilation.directives) { - const extMeta = flattenInheritedDirectiveMetadata(this.metaReader, meta.ref); - matcher.addSelectables(CssSelector.parse(meta.selector), extMeta); + if (meta.selector !== null) { + const extMeta = flattenInheritedDirectiveMetadata(this.metaReader, meta.ref); + matcher.addSelectables(CssSelector.parse(meta.selector), extMeta); + } } for (const {name, ref} of scope.compilation.pipes) { if (!ts.isClassDeclaration(ref.node)) { @@ -422,9 +422,11 @@ export class ComponentDecoratorHandler implements for (const dir of scope.compilation.directives) { const {ref, selector} = dir; - const expression = this.refEmitter.emit(ref, context); - directives.push({selector, expression}); - matcher.addSelectables(CssSelector.parse(selector), {...dir, expression}); + if (selector !== null) { + const expression = this.refEmitter.emit(ref, context); + directives.push({selector, expression}); + matcher.addSelectables(CssSelector.parse(selector), {...dir, expression}); + } } const pipes = new Map(); for (const pipe of scope.compilation.pipes) { diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts index 32965b0e3f222..79c3037c9b371 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/directive.ts @@ -55,32 +55,25 @@ export class DirectiveDecoratorHandler implements const directiveResult = extractDirectiveMetadata( node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore); const analysis = directiveResult && directiveResult.metadata; - - // If the directive has a selector, it should be registered with the `SelectorScopeRegistry` so - // when this directive appears in an `@NgModule` scope, its selector can be determined. - if (analysis && analysis.selector !== null) { - const ref = new Reference(node); - this.metaRegistry.registerDirectiveMetadata({ - ref, - name: node.name.text, - selector: analysis.selector, - exportAs: analysis.exportAs, - inputs: analysis.inputs, - outputs: analysis.outputs, - queries: analysis.queries.map(query => query.propertyName), - isComponent: false, ...extractDirectiveGuards(node, this.reflector), - baseClass: readBaseClass(node, this.reflector, this.evaluator), - }); - } - - if (analysis && !analysis.selector) { - this.metaRegistry.registerAbstractDirective(node); - } - if (analysis === undefined) { return {}; } + // Register this directive's information with the `MetadataRegistry`. This ensures that + // the information about the directive is available during the compile() phase. + const ref = new Reference(node); + this.metaRegistry.registerDirectiveMetadata({ + ref, + name: node.name.text, + selector: analysis.selector, + exportAs: analysis.exportAs, + inputs: analysis.inputs, + outputs: analysis.outputs, + queries: analysis.queries.map(query => query.propertyName), + isComponent: false, ...extractDirectiveGuards(node, this.reflector), + baseClass: readBaseClass(node, this.reflector, this.evaluator), + }); + return { analysis: { meta: analysis, diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts index 95b485a3370f9..adc7b265bbe58 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts @@ -262,7 +262,9 @@ export class NgModuleDecoratorHandler implements DecoratorHandler): boolean { - if (!this.metadata.has(ref.node.getSourceFile())) { - return false; - } - const metadata = this.metadata.get(ref.node.getSourceFile()) !; - return metadata.abstractDirectives.has(ref.node); - } - - registerAbstractDirective(clazz: ClassDeclaration): void { - const metadata = this.ensureMetadata(clazz.getSourceFile()); - metadata.abstractDirectives.add(clazz); - } - getDirectiveMetadata(ref: Reference): DirectiveMeta|null { if (!this.metadata.has(ref.node.getSourceFile())) { return null; @@ -200,7 +187,6 @@ class FileMetadata { /** A set of source files that this file depends upon. */ fileDependencies = new Set(); resourcePaths = new Set(); - abstractDirectives = new Set(); directiveMeta = new Map(); ngModuleMeta = new Map(); pipeMeta = new Map(); diff --git a/packages/compiler-cli/src/ngtsc/indexer/src/context.ts b/packages/compiler-cli/src/ngtsc/indexer/src/context.ts index 32991eab073b8..1881ec01f0ce4 100644 --- a/packages/compiler-cli/src/ngtsc/indexer/src/context.ts +++ b/packages/compiler-cli/src/ngtsc/indexer/src/context.ts @@ -13,9 +13,9 @@ import {ClassDeclaration} from '../../reflection'; export interface ComponentMeta extends DirectiveMeta { ref: Reference; /** - * Unparsed selector of the directive. + * Unparsed selector of the directive, or null if the directive does not have a selector. */ - selector: string; + selector: string|null; } /** diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 34b4202c8144c..b3bea87eb1076 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -28,9 +28,9 @@ export interface NgModuleMeta { export interface DirectiveMeta extends T2DirectiveMeta { ref: Reference; /** - * Unparsed selector of the directive. + * Unparsed selector of the directive, or null if the directive does not have a selector. */ - selector: string; + selector: string|null; queries: string[]; ngTemplateGuards: TemplateGuardMeta[]; hasNgTemplateContextGuard: boolean; @@ -76,7 +76,6 @@ export interface PipeMeta { * or a registry. */ export interface MetadataReader { - isAbstractDirective(node: Reference): boolean; getDirectiveMetadata(node: Reference): DirectiveMeta|null; getNgModuleMetadata(node: Reference): NgModuleMeta|null; getPipeMetadata(node: Reference): PipeMeta|null; @@ -86,7 +85,6 @@ export interface MetadataReader { * Registers new metadata for directives, pipes, and modules. */ export interface MetadataRegistry { - registerAbstractDirective(clazz: ClassDeclaration): void; registerDirectiveMetadata(meta: DirectiveMeta): void; registerNgModuleMetadata(meta: NgModuleMeta): void; registerPipeMetadata(meta: PipeMeta): void; diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts index 17d6a6e2a16fd..417d983ce92ef 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/dts.ts @@ -21,11 +21,6 @@ import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType, export class DtsMetadataReader implements MetadataReader { constructor(private checker: ts.TypeChecker, private reflector: ReflectionHost) {} - isAbstractDirective(ref: Reference): boolean { - const meta = this.getDirectiveMetadata(ref); - return meta !== null && meta.selector === null; - } - /** * Read the metadata from a class that has already been compiled somehow (either it's in a .d.ts * file, or in a .ts file with a handwritten definition). @@ -79,15 +74,12 @@ export class DtsMetadataReader implements MetadataReader { // The type metadata was the wrong shape. return null; } - const selector = readStringType(def.type.typeArguments[1]); - if (selector === null) { - return null; - } return { ref, name: clazz.name.text, - isComponent: def.name === 'ɵcmp', selector, + isComponent: def.name === 'ɵcmp', + selector: readStringType(def.type.typeArguments[1]), exportAs: readStringArrayType(def.type.typeArguments[2]), inputs: readStringMapType(def.type.typeArguments[3]), outputs: readStringMapType(def.type.typeArguments[4]), diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts index c4d7ec05e6712..f834218efd2e2 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/registry.ts @@ -16,14 +16,10 @@ import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} * unit, which supports both reading and registering. */ export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader { - private abstractDirectives = new Set(); private directives = new Map(); private ngModules = new Map(); private pipes = new Map(); - isAbstractDirective(ref: Reference): boolean { - return this.abstractDirectives.has(ref.node); - } getDirectiveMetadata(ref: Reference): DirectiveMeta|null { return this.directives.has(ref.node) ? this.directives.get(ref.node) ! : null; } @@ -34,7 +30,6 @@ export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader { return this.pipes.has(ref.node) ? this.pipes.get(ref.node) ! : null; } - registerAbstractDirective(clazz: ClassDeclaration): void { this.abstractDirectives.add(clazz); } registerDirectiveMetadata(meta: DirectiveMeta): void { this.directives.set(meta.ref.node, meta); } registerNgModuleMetadata(meta: NgModuleMeta): void { this.ngModules.set(meta.ref.node, meta); } registerPipeMetadata(meta: PipeMeta): void { this.pipes.set(meta.ref.node, meta); } @@ -46,12 +41,6 @@ export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader { export class CompoundMetadataRegistry implements MetadataRegistry { constructor(private registries: MetadataRegistry[]) {} - registerAbstractDirective(clazz: ClassDeclaration) { - for (const registry of this.registries) { - registry.registerAbstractDirective(clazz); - } - } - registerDirectiveMetadata(meta: DirectiveMeta): void { for (const registry of this.registries) { registry.registerDirectiveMetadata(meta); diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index 7f8a9d2678fdb..31a39e9362933 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -125,10 +125,6 @@ function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { export class CompoundMetadataReader implements MetadataReader { constructor(private readers: MetadataReader[]) {} - isAbstractDirective(node: Reference): boolean { - return this.readers.some(r => r.isAbstractDirective(node)); - } - getDirectiveMetadata(node: Reference>): DirectiveMeta|null { for (const reader of this.readers) { const meta = reader.getDirectiveMetadata(node); diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index dd9f679bc32c8..a05fa6b02edab 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -118,8 +118,6 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop } } - registerAbstractDirective(clazz: ClassDeclaration): void {} - registerDirectiveMetadata(directive: DirectiveMeta): void {} registerPipeMetadata(pipe: PipeMeta): void {} diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts index 8ddfc0ad9c434..bd40aafe42f81 100644 --- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts +++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts @@ -191,6 +191,50 @@ runInEachFileSystem(os => { expect(jsContents).toContain('inject(Dep, 8)'); }); + it('should compile Directives without errors', () => { + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[dir]'}) + export class TestDir {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('TestDir.ngDirectiveDef = i0.ɵɵdefineDirective'); + expect(jsContents).toContain('TestDir.ngFactoryDef = function'); + expect(jsContents).not.toContain('__decorate'); + + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ngDirectiveDef: i0.ɵɵDirectiveDefWithMeta'); + expect(dtsContents).toContain('static ngFactoryDef: i0.ɵɵFactoryDef'); + }); + + it('should compile abstract Directives without errors', () => { + env.write('test.ts', ` + import {Directive} from '@angular/core'; + + @Directive() + export class TestDir {} + `); + + env.driveMain(); + + const jsContents = env.getContents('test.js'); + expect(jsContents).toContain('TestDir.ngDirectiveDef = i0.ɵɵdefineDirective'); + expect(jsContents).toContain('TestDir.ngFactoryDef = function'); + expect(jsContents).not.toContain('__decorate'); + + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ngDirectiveDef: i0.ɵɵDirectiveDefWithMeta'); + expect(dtsContents).toContain('static ngFactoryDef: i0.ɵɵFactoryDef'); + }); + it('should compile Components (inline template) without errors', () => { env.write('test.ts', ` import {Component} from '@angular/core'; diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index a3be05aa2707e..31ba74ac2541c 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -308,10 +308,15 @@ export declare class CommonModule { env.write('test.ts', ` import {Component, Directive, Input, NgModule} from '@angular/core'; + @Directive() + class AbstractDir { + @Input() fromAbstract!: number; + } + @Directive({ selector: '[base]', }) - class BaseDir { + class BaseDir extends AbstractDir { @Input() fromBase!: string; } @@ -324,7 +329,7 @@ export declare class CommonModule { @Component({ selector: 'test', - template: '
', + template: '
', }) class TestCmp {} @@ -335,15 +340,74 @@ export declare class CommonModule { `); const diags = env.driveDiagnostics(); - expect(diags.length).toBe(2); + expect(diags.length).toBe(3); expect(diags[0].messageText) + .toBe(`Type 'true' is not assignable to type 'number | undefined'.`); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); + expect(diags[1].messageText) .toBe(`Type 'number' is not assignable to type 'string | undefined'.`); - expect(diags[0].start).toEqual(386); - expect(diags[0].length).toEqual(14); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(diags[2].messageText) + .toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`); + expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); + }); + + it('should properly type-check inherited directives from external libraries', () => { + env.write('node_modules/external/index.d.ts', ` + import * as i0 from '@angular/core'; + + export declare class AbstractDir { + fromAbstract: number; + + static ngDirectiveDef: i0.ɵɵDirectiveDefWithMeta; + } + + export declare class BaseDir extends AbstractDir { + fromBase: string; + + static ngDirectiveDef: i0.ɵɵDirectiveDefWithMeta; + } + + export declare class ExternalModule { + static ngModuleDef: i0.ɵɵNgModuleDefWithMeta; + } + `); + + env.write('test.ts', ` + import {Component, Directive, Input, NgModule} from '@angular/core'; + import {BaseDir, ExternalModule} from 'external'; + + @Directive({ + selector: '[child]', + }) + class ChildDir extends BaseDir { + @Input() fromChild!: boolean; + } + + @Component({ + selector: 'test', + template: '
', + }) + class TestCmp {} + + @NgModule({ + declarations: [TestCmp, ChildDir], + imports: [ExternalModule], + }) + class Module {} + `); + + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(3); + expect(diags[0].messageText) + .toBe(`Type 'true' is not assignable to type 'number | undefined'.`); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); expect(diags[1].messageText) + .toBe(`Type 'number' is not assignable to type 'string | undefined'.`); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(diags[2].messageText) .toBe(`Type 'number' is not assignable to type 'boolean | undefined'.`); - expect(diags[1].start).toEqual(401); - expect(diags[1].length).toEqual(15); + expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); }); describe('legacy schema checking with the DOM schema', () => { diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index a05323ad25bdf..aa7185cb4b130 100644 --- a/packages/compiler/src/render3/view/compiler.ts +++ b/packages/compiler/src/render3/view/compiler.ts @@ -304,10 +304,6 @@ export function compileComponentFromMetadata( definitionMap.set('changeDetection', o.literal(changeDetection)); } - // On the type side, remove newlines from the selector as it will need to fit into a TypeScript - // string literal, which must be on one line. - const selectorForType = (meta.selector || '').replace(/\n/g, ''); - const expression = o.importExpr(R3.defineComponent).callFn([definitionMap.toLiteralMap()]); const type = createTypeForDef(meta, R3.ComponentDefWithMeta); @@ -540,11 +536,11 @@ function stringArrayAsType(arr: string[]): o.Type { function createTypeForDef(meta: R3DirectiveMetadata, typeBase: o.ExternalReference): o.Type { // On the type side, remove newlines from the selector as it will need to fit into a TypeScript // string literal, which must be on one line. - const selectorForType = (meta.selector || '').replace(/\n/g, ''); + const selectorForType = meta.selector !== null ? meta.selector.replace(/\n/g, '') : null; return o.expressionType(o.importExpr(typeBase, [ typeWithParameters(meta.type, meta.typeArgumentCount), - stringAsType(selectorForType), + selectorForType !== null ? stringAsType(selectorForType) : o.NONE_TYPE, meta.exportAs !== null ? stringArrayAsType(meta.exportAs) : o.NONE_TYPE, stringMapAsType(meta.inputs), stringMapAsType(meta.outputs), diff --git a/packages/core/src/render3/jit/directive.ts b/packages/core/src/render3/jit/directive.ts index 26af0adb67739..9298e4755436f 100644 --- a/packages/core/src/render3/jit/directive.ts +++ b/packages/core/src/render3/jit/directive.ts @@ -192,7 +192,7 @@ export function directiveMetadata(type: Type, metadata: Directive): R3Direc name: type.name, type: type, typeArgumentCount: 0, - selector: metadata.selector !, + selector: metadata.selector !== undefined ? metadata.selector : null, deps: reflectDependencies(type), host: metadata.host || EMPTY_OBJ, propMetadata: propMetadata,