From a42057d0f8ea3f8bd7c0afc2b92a29f1800cf787 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 (#33131) 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 PR Close #33131 --- .../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, 166 insertions(+), 110 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 1704e2aeb4db0..d4cd46cabdffb 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 7a814e7bec225..c40d354581d81 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 6739226478141..bde0ee4c71794 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; @@ -77,7 +77,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; @@ -87,7 +86,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 bb4a12c6344a3..2559fd6a35357 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 d49a9540130fc..a3982fd37c247 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -137,10 +137,6 @@ function extractCoercedInput(member: ClassMember): string|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 3d7cb2a90f7f0..94f9f7ad1cdc5 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 e5fc130ffebfb..a38c2c3e71d93 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.ɵdir = i0.ɵɵdefineDirective'); + expect(jsContents).toContain('TestDir.ɵfac = function'); + expect(jsContents).not.toContain('__decorate'); + + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ɵdir: i0.ɵɵDirectiveDefWithMeta'); + expect(dtsContents).toContain('static ɵfac: 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.ɵdir = i0.ɵɵdefineDirective'); + expect(jsContents).toContain('TestDir.ɵfac = function'); + expect(jsContents).not.toContain('__decorate'); + + const dtsContents = env.getContents('test.d.ts'); + expect(dtsContents) + .toContain( + 'static ɵdir: i0.ɵɵDirectiveDefWithMeta'); + expect(dtsContents).toContain('static ɵfac: 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 85fe42130406b..6b0293592b3ab 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -370,10 +370,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; } @@ -386,7 +391,7 @@ export declare class CommonModule { @Component({ selector: 'test', - template: '
', + template: '
', }) class TestCmp {} @@ -397,13 +402,68 @@ export declare class CommonModule { `); const diags = env.driveDiagnostics(); - expect(diags.length).toBe(2); - expect(diags[0].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); - expect(diags[0].start).toEqual(386); - expect(diags[0].length).toEqual(14); - expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); - expect(diags[1].start).toEqual(401); - expect(diags[1].length).toEqual(15); + expect(diags.length).toBe(3); + expect(diags[0].messageText).toBe(`Type 'true' is not assignable to type 'number'.`); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); + expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); + 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 ɵdir: i0.ɵɵDirectiveDefWithMeta; + } + + export declare class BaseDir extends AbstractDir { + fromBase: string; + + static ɵdir: i0.ɵɵDirectiveDefWithMeta; + } + + export declare class ExternalModule { + static ɵmod: 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'.`); + expect(getSourceCodeForDiagnostic(diags[0])).toEqual('[fromAbstract]="true"'); + expect(diags[1].messageText).toBe(`Type 'number' is not assignable to type 'string'.`); + expect(getSourceCodeForDiagnostic(diags[1])).toEqual('[fromBase]="3"'); + expect(diags[2].messageText).toBe(`Type 'number' is not assignable to type 'boolean'.`); + expect(getSourceCodeForDiagnostic(diags[2])).toEqual('[fromChild]="4"'); }); describe('input coercion', () => { diff --git a/packages/compiler/src/render3/view/compiler.ts b/packages/compiler/src/render3/view/compiler.ts index 145d5734fe173..9769f31a5a1ab 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); @@ -538,11 +534,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 b1b7d75647370..a2c53e8186969 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,