Skip to content

Commit

Permalink
fix(ivy): Support selector-less directive as base classes
Browse files Browse the repository at this point in the history
Following #31379, this adds support for directives without a selector to
Ivy.
  • Loading branch information
atscott committed Aug 16, 2019
1 parent 5a562d8 commit fa23bb1
Show file tree
Hide file tree
Showing 19 changed files with 169 additions and 79 deletions.
Expand Up @@ -86,9 +86,9 @@ export class DecorationAnalyzer {
this.reflectionHost, NOOP_DEFAULT_IMPORT_RECORDER, this.isCore,
/* strictCtorDeps */ false),
new NgModuleDecoratorHandler(
this.reflectionHost, this.evaluator, this.fullRegistry, this.scopeRegistry,
this.referencesRegistry, this.isCore, /* routeAnalyzer */ null, this.refEmitter,
NOOP_DEFAULT_IMPORT_RECORDER),
this.reflectionHost, this.evaluator, this.fullMetaReader, this.fullRegistry,
this.scopeRegistry, this.referencesRegistry, this.isCore, /* routeAnalyzer */ null,
this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER),
new PipeDecoratorHandler(
this.reflectionHost, this.evaluator, this.metaRegistry, NOOP_DEFAULT_IMPORT_RECORDER,
this.isCore),
Expand Down
36 changes: 25 additions & 11 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
Expand Up @@ -72,6 +72,10 @@ export class DirectiveDecoratorHandler implements
});
}

if (analysis && !analysis.selector) {
this.metaRegistry.registerAbstractDirective(node);
}

if (analysis === undefined) {
return {};
}
Expand Down Expand Up @@ -102,7 +106,10 @@ export class DirectiveDecoratorHandler implements
}

/**
* Helper function to extract metadata from a `Directive` or `Component`.
* Helper function to extract metadata from a `Directive` or `Component`. `Directive`s without a
* selector are allowed to be used for abstract base classes. These abstract directives should not
* appear in the declarations of an `NgModule` and additional verification is done when processing
* the module.
*/
export function extractDirectiveMetadata(
clazz: ClassDeclaration, decorator: Decorator, reflector: ReflectionHost,
Expand All @@ -112,17 +119,22 @@ export function extractDirectiveMetadata(
metadata: R3DirectiveMetadata,
decoratedElements: ClassMember[],
}|undefined {
if (decorator.args === null || decorator.args.length !== 1) {
let directive: Map<string, ts.Expression>;
if (decorator.args === null || decorator.args.length === 0) {
directive = new Map<string, ts.Expression>();
} else if (decorator.args.length !== 1) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARITY_WRONG, decorator.node,
`Incorrect number of arguments to @${decorator.name} decorator`);
} else {
const meta = unwrapExpression(decorator.args[0]);
if (!ts.isObjectLiteralExpression(meta)) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, meta,
`@${decorator.name} argument must be literal.`);
}
directive = reflectObjectLiteral(meta);
}
const meta = unwrapExpression(decorator.args[0]);
if (!ts.isObjectLiteralExpression(meta)) {
throw new FatalDiagnosticError(
ErrorCode.DECORATOR_ARG_NOT_LITERAL, meta, `@${decorator.name} argument must be literal.`);
}
const directive = reflectObjectLiteral(meta);

if (directive.has('jit')) {
// The only allowed value is true, so there's no need to expand further.
Expand Down Expand Up @@ -188,9 +200,11 @@ export function extractDirectiveMetadata(
}
// use default selector in case selector is an empty string
selector = resolved === '' ? defaultSelector : resolved;
}
if (!selector) {
throw new Error(`Directive ${clazz.name.text} has no selector, please add it!`);
if (!selector) {
throw new FatalDiagnosticError(
ErrorCode.DIRECTIVE_MISSING_SELECTOR, expr,
`Directive ${clazz.name.text} has no selector, please add it!`);
}
}

const host = extractHostBindings(decoratedElements, evaluator, coreModule, directive);
Expand Down
17 changes: 13 additions & 4 deletions packages/compiler-cli/src/ngtsc/annotations/src/ng_module.ts
Expand Up @@ -11,7 +11,7 @@ import * as ts from 'typescript';

import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
import {DefaultImportRecorder, Reference, ReferenceEmitter} from '../../imports';
import {MetadataRegistry} from '../../metadata';
import {MetadataReader, MetadataRegistry} from '../../metadata';
import {PartialEvaluator, ResolvedValue} from '../../partial_evaluator';
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral, typeNodeToValueExpr} from '../../reflection';
import {NgModuleRouteAnalyzer} from '../../routing';
Expand Down Expand Up @@ -40,7 +40,8 @@ export interface NgModuleAnalysis {
export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalysis, Decorator> {
constructor(
private reflector: ReflectionHost, private evaluator: PartialEvaluator,
private metaRegistry: MetadataRegistry, private scopeRegistry: LocalModuleScopeRegistry,
private metaReader: MetadataReader, private metaRegistry: MetadataRegistry,
private scopeRegistry: LocalModuleScopeRegistry,
private referencesRegistry: ReferencesRegistry, private isCore: boolean,
private routeAnalyzer: NgModuleRouteAnalyzer|null, private refEmitter: ReferenceEmitter,
private defaultImportRecorder: DefaultImportRecorder, private localeId?: string) {}
Expand Down Expand Up @@ -210,15 +211,23 @@ export class NgModuleDecoratorHandler implements DecoratorHandler<NgModuleAnalys
const scope = this.scopeRegistry.getScopeOfModule(node);
const diagnostics = this.scopeRegistry.getDiagnosticsOfModule(node) || undefined;

// Using the scope information, extend the injector's imports using the modules that are
// specified as module exports.
if (scope !== null) {
// Using the scope information, extend the injector's imports using the modules that are
// specified as module exports.
const context = getSourceFile(node);
for (const exportRef of analysis.exports) {
if (isNgModule(exportRef.node, scope.compilation)) {
analysis.ngInjectorDef.imports.push(this.refEmitter.emit(exportRef, context));
}
}

for (const decl of analysis.declarations) {
if (this.metaReader.isAbstractDirective(decl)) {
throw new FatalDiagnosticError(
ErrorCode.DIRECTIVE_MISSING_SELECTOR, decl.node,
`Directive ${decl.node.name.text} has no selector, please add it!`);
}
}
}

if (scope === null || scope.reexports === null) {
Expand Down
Expand Up @@ -8,10 +8,11 @@
import {WrappedNodeExpr} from '@angular/compiler';
import {R3Reference} from '@angular/compiler/src/compiler';
import * as ts from 'typescript';

import {absoluteFrom} from '../../file_system';
import {runInEachFileSystem} from '../../file_system/testing';
import {LocalIdentifierStrategy, NOOP_DEFAULT_IMPORT_RECORDER, ReferenceEmitter} from '../../imports';
import {DtsMetadataReader, LocalMetadataRegistry} from '../../metadata';
import {CompoundMetadataReader, DtsMetadataReader, LocalMetadataRegistry} from '../../metadata';
import {PartialEvaluator} from '../../partial_evaluator';
import {TypeScriptReflectionHost, isNamedClassDeclaration} from '../../reflection';
import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from '../../scope';
Expand Down Expand Up @@ -59,15 +60,16 @@ runInEachFileSystem(() => {
const evaluator = new PartialEvaluator(reflectionHost, checker);
const referencesRegistry = new NoopReferencesRegistry();
const metaRegistry = new LocalMetadataRegistry();
const metaReader = new CompoundMetadataReader([metaRegistry]);
const dtsReader = new DtsMetadataReader(checker, reflectionHost);
const scopeRegistry = new LocalModuleScopeRegistry(
metaRegistry, new MetadataDtsModuleScopeResolver(dtsReader, null),
new ReferenceEmitter([]), null);
const refEmitter = new ReferenceEmitter([new LocalIdentifierStrategy()]);

const handler = new NgModuleDecoratorHandler(
reflectionHost, evaluator, metaRegistry, scopeRegistry, referencesRegistry, false, null,
refEmitter, NOOP_DEFAULT_IMPORT_RECORDER);
reflectionHost, evaluator, metaReader, metaRegistry, scopeRegistry, referencesRegistry,
false, null, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER);
const TestModule =
getDeclaration(program, _('/entry.ts'), 'TestModule', isNamedClassDeclaration);
const detected =
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/diagnostics/src/code.ts
Expand Up @@ -24,6 +24,7 @@ export enum ErrorCode {
COMPONENT_MISSING_TEMPLATE = 2001,
PIPE_MISSING_NAME = 2002,
PARAM_MISSING_TOKEN = 2003,
DIRECTIVE_MISSING_SELECTOR = 2004,

SYMBOL_NOT_EXPORTED = 3001,
SYMBOL_EXPORTED_UNDER_DIFFERENT_NAME = 3002,
Expand Down
14 changes: 14 additions & 0 deletions packages/compiler-cli/src/ngtsc/incremental/src/state.ts
Expand Up @@ -93,6 +93,19 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
metadata.ngModuleMeta.set(meta.ref.node, meta);
}

isAbstractDirective(ref: Reference<ClassDeclaration>): 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<ClassDeclaration>): DirectiveMeta|null {
if (!this.metadata.has(ref.node.getSourceFile())) {
return null;
Expand Down Expand Up @@ -187,6 +200,7 @@ class FileMetadata {
/** A set of source files that this file depends upon. */
fileDependencies = new Set<ts.SourceFile>();
resourcePaths = new Set<string>();
abstractDirectives = new Set<ClassDeclaration>();
directiveMeta = new Map<ClassDeclaration, DirectiveMeta>();
ngModuleMeta = new Map<ClassDeclaration, NgModuleMeta>();
pipeMeta = new Map<ClassDeclaration, PipeMeta>();
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/api.ts
Expand Up @@ -75,6 +75,7 @@ export interface PipeMeta {
* or a registry.
*/
export interface MetadataReader {
isAbstractDirective(node: Reference<ClassDeclaration>): boolean;
getDirectiveMetadata(node: Reference<ClassDeclaration>): DirectiveMeta|null;
getNgModuleMetadata(node: Reference<ClassDeclaration>): NgModuleMeta|null;
getPipeMetadata(node: Reference<ClassDeclaration>): PipeMeta|null;
Expand All @@ -84,6 +85,7 @@ 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;
Expand Down
5 changes: 5 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/dts.ts
Expand Up @@ -21,6 +21,11 @@ import {extractDirectiveGuards, extractReferencesFromType, readStringArrayType,
export class DtsMetadataReader implements MetadataReader {
constructor(private checker: ts.TypeChecker, private reflector: ReflectionHost) {}

isAbstractDirective(ref: Reference<ClassDeclaration>): 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).
Expand Down
11 changes: 11 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/registry.ts
Expand Up @@ -16,10 +16,14 @@ import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta}
* unit, which supports both reading and registering.
*/
export class LocalMetadataRegistry implements MetadataRegistry, MetadataReader {
private abstractDirectives = new Set<ClassDeclaration>();
private directives = new Map<ClassDeclaration, DirectiveMeta>();
private ngModules = new Map<ClassDeclaration, NgModuleMeta>();
private pipes = new Map<ClassDeclaration, PipeMeta>();

isAbstractDirective(ref: Reference<ClassDeclaration>): boolean {
return this.abstractDirectives.has(ref.node);
}
getDirectiveMetadata(ref: Reference<ClassDeclaration>): DirectiveMeta|null {
return this.directives.has(ref.node) ? this.directives.get(ref.node) ! : null;
}
Expand All @@ -30,6 +34,7 @@ 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); }
Expand All @@ -41,6 +46,12 @@ 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);
Expand Down
4 changes: 4 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/util.ts
Expand Up @@ -125,6 +125,10 @@ function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null {
export class CompoundMetadataReader implements MetadataReader {
constructor(private readers: MetadataReader[]) {}

isAbstractDirective(node: Reference<ClassDeclaration>): boolean {
return this.readers.some(r => r.isAbstractDirective(node));
}

getDirectiveMetadata(node: Reference<ClassDeclaration<ts.Declaration>>): DirectiveMeta|null {
for (const reader of this.readers) {
const meta = reader.getDirectiveMetadata(node);
Expand Down
6 changes: 3 additions & 3 deletions packages/compiler-cli/src/ngtsc/program.ts
Expand Up @@ -514,9 +514,9 @@ export class NgtscProgram implements api.Program {
this.reflector, this.defaultImportTracker, this.isCore,
this.options.strictInjectionParameters || false),
new NgModuleDecoratorHandler(
this.reflector, evaluator, metaRegistry, scopeRegistry, referencesRegistry, this.isCore,
this.routeAnalyzer, this.refEmitter, this.defaultImportTracker,
this.options.i18nInLocale),
this.reflector, evaluator, this.metaReader, metaRegistry, scopeRegistry,
referencesRegistry, this.isCore, this.routeAnalyzer, this.refEmitter,
this.defaultImportTracker, this.options.i18nInLocale),
new PipeDecoratorHandler(
this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore),
];
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/scope/src/local.ts
Expand Up @@ -117,6 +117,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScop
}
}

registerAbstractDirective(clazz: ClassDeclaration): void {}

registerDirectiveMetadata(directive: DirectiveMeta): void {}

registerPipeMetadata(pipe: PipeMeta): void {}
Expand Down
Expand Up @@ -669,44 +669,6 @@ describe('compiler compliance', () => {
source, EmptyOutletComponentDefinition, 'Incorrect EmptyOutletComponent.ngComponentDef');
});

it('should not support directives without selector', () => {
const files = {
app: {
'spec.ts': `
import {Component, Directive, NgModule} from '@angular/core';
@Directive({})
export class EmptyOutletDirective {}
@NgModule({declarations: [EmptyOutletDirective]})
export class MyModule{}
`
}
};

expect(() => compile(files, angularFiles))
.toThrowError('Directive EmptyOutletDirective has no selector, please add it!');
});

it('should not support directives with empty selector', () => {
const files = {
app: {
'spec.ts': `
import {Component, Directive, NgModule} from '@angular/core';
@Directive({selector: ''})
export class EmptyOutletDirective {}
@NgModule({declarations: [EmptyOutletDirective]})
export class MyModule{}
`
}
};

expect(() => compile(files, angularFiles))
.toThrowError('Directive EmptyOutletDirective has no selector, please add it!');
});

it('should not treat ElementRef, ViewContainerRef, or ChangeDetectorRef specially when injecting',
() => {
const files = {
Expand Down
42 changes: 33 additions & 9 deletions packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
Expand Up @@ -1008,19 +1008,43 @@ runInEachFileSystem(os => {
expect(jsContents).toContain('selectors: [["ng-component"]]');
});

it('should throw if selector is missing in Directive decorator params', () => {
env.write('test.ts', `
import {Directive} from '@angular/core';
it('should allow directives with no selector that are not in NgModules', () => {
env.write('main.ts', `
import {Directive} from '@angular/core';
@Directive({
inputs: ['a', 'b']
})
export class TestDir {}
`);
@Directive({})
export class BaseDir {}
@Directive({})
export abstract class AbstractBaseDir {}
@Directive()
export abstract class EmptyDir {}
@Directive({
inputs: ['a', 'b']
})
export class TestDirWithInputs {}
`);
const errors = env.driveDiagnostics();
expect(errors.length).toBe(0);
});

it('should not allow directives with no selector that are in NgModules', () => {
env.write('main.ts', `
import {Directive, NgModule} from '@angular/core';
@Directive({})
export class BaseDir {}
@NgModule({
declarations: [BaseDir],
})
export class MyModule {}
`);
const errors = env.driveDiagnostics();
expect(trim(errors[0].messageText as string))
.toContain('Directive TestDir has no selector, please add it!');
.toContain('Directive BaseDir has no selector, please add it!');
});

it('should throw if Directive selector is an empty string', () => {
Expand Down
4 changes: 0 additions & 4 deletions packages/compiler/src/render3/view/compiler.ts
Expand Up @@ -132,10 +132,6 @@ export function compileDirectiveFromMetadata(
addFeatures(definitionMap, meta);
const expression = o.importExpr(R3.defineDirective).callFn([definitionMap.toLiteralMap()]);

if (!meta.selector) {
throw new Error(`Directive ${meta.name} has no selector, please add it!`);
}

const type = createTypeForDef(meta, R3.DirectiveDefWithMeta);
return {expression, type, statements};
}
Expand Down

0 comments on commit fa23bb1

Please sign in to comment.