Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(language-service): correctly handle host directive inputs/outputs #48147

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/ngtsc/typecheck/api/api.ts
Expand Up @@ -10,9 +10,8 @@ import {AbsoluteSourceSpan, BoundTarget, DirectiveMeta, ParseSourceSpan, SchemaM
import ts from 'typescript';

import {ErrorCode} from '../../diagnostics';
import {AbsoluteFsPath} from '../../file_system';
import {Reference} from '../../imports';
import {ClassPropertyMapping, DirectiveTypeCheckMeta} from '../../metadata';
import {ClassPropertyMapping, DirectiveTypeCheckMeta, HostDirectiveMeta} from '../../metadata';
import {ClassDeclaration} from '../../reflection';


Expand All @@ -26,6 +25,7 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta, DirectiveType
inputs: ClassPropertyMapping;
outputs: ClassPropertyMapping;
isStandalone: boolean;
hostDirectives: HostDirectiveMeta[]|null;
}

export type TemplateId = string&{__brand: 'TemplateId'};
Expand Down
17 changes: 12 additions & 5 deletions packages/compiler-cli/src/ngtsc/typecheck/api/symbols.ts
Expand Up @@ -258,11 +258,8 @@ export interface TemplateSymbol {
templateNode: TmplAstTemplate;
}

/**
* A representation of a directive/component whose selector matches a node in a component
* template.
*/
export interface DirectiveSymbol extends PotentialDirective {
/** Interface shared between host and non-host directives. */
interface DirectiveSymbolBase extends PotentialDirective {
kind: SymbolKind.Directive;

/** The `ts.Type` for the class declaration. */
Expand All @@ -272,6 +269,16 @@ export interface DirectiveSymbol extends PotentialDirective {
tcbLocation: TcbLocation;
}

/**
* A representation of a directive/component whose selector matches a node in a component
* template.
*/
export type DirectiveSymbol = (DirectiveSymbolBase&{isHostDirective: false})|(DirectiveSymbolBase&{
isHostDirective: true;
exposedInputs: Record<string, string>|null;
exposedOutputs: Record<string, string>|null;
});

/**
* A representation of an attribute on an element or template. These bindings aren't currently
* type-checked (see `checkTypeOfDomBindings`) so they won't have a `ts.Type`, `ts.Symbol`, or shim
Expand Down
Expand Up @@ -11,6 +11,7 @@ import ts from 'typescript';

import {AbsoluteFsPath} from '../../file_system';
import {Reference} from '../../imports';
import {HostDirectiveMeta} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {ComponentScopeKind, ComponentScopeReader} from '../../scope';
import {isAssignment, isSymbolWithValueDeclaration} from '../../util/src/typescript';
Expand Down Expand Up @@ -118,38 +119,80 @@ export class SymbolBuilder {

const nodes = findAllMatchingNodes(
this.typeCheckBlock, {withSpan: elementSourceSpan, filter: isDirectiveDeclaration});
return nodes
.map(node => {
const symbol = this.getSymbolOfTsNode(node.parent);
if (symbol === null || !isSymbolWithValueDeclaration(symbol.tsSymbol) ||
!ts.isClassDeclaration(symbol.tsSymbol.valueDeclaration)) {
return null;
}
const meta = this.getDirectiveMeta(element, symbol.tsSymbol.valueDeclaration);
if (meta === null) {
return null;
}

const ngModule = this.getDirectiveModule(symbol.tsSymbol.valueDeclaration);
if (meta.selector === null) {
return null;
}
const isComponent = meta.isComponent ?? null;
const ref = new Reference<ClassDeclaration>(symbol.tsSymbol.valueDeclaration as any);
const directiveSymbol: DirectiveSymbol = {
...symbol,
ref,
tsSymbol: symbol.tsSymbol,
selector: meta.selector,
isComponent,
ngModule,
kind: SymbolKind.Directive,
isStructural: meta.isStructural,
isInScope: true,
};
return directiveSymbol;
})
.filter((d): d is DirectiveSymbol => d !== null);
const symbols: DirectiveSymbol[] = [];

for (const node of nodes) {
const symbol = this.getSymbolOfTsNode(node.parent);
if (symbol === null || !isSymbolWithValueDeclaration(symbol.tsSymbol) ||
!ts.isClassDeclaration(symbol.tsSymbol.valueDeclaration)) {
continue;
}

const meta = this.getDirectiveMeta(element, symbol.tsSymbol.valueDeclaration);

if (meta !== null && meta.selector !== null) {
const ref = new Reference<ClassDeclaration>(symbol.tsSymbol.valueDeclaration as any);

if (meta.hostDirectives !== null) {
this.addHostDirectiveSymbols(element, meta.hostDirectives, symbols);
}

const directiveSymbol: DirectiveSymbol = {
...symbol,
ref,
tsSymbol: symbol.tsSymbol,
selector: meta.selector,
isComponent: meta.isComponent,
ngModule: this.getDirectiveModule(symbol.tsSymbol.valueDeclaration),
kind: SymbolKind.Directive,
isStructural: meta.isStructural,
isInScope: true,
isHostDirective: false,
};

symbols.push(directiveSymbol);
}
}

return symbols;
}

private addHostDirectiveSymbols(
host: TmplAstTemplate|TmplAstElement, hostDirectives: HostDirectiveMeta[],
symbols: DirectiveSymbol[]): void {
for (const current of hostDirectives) {
if (!ts.isClassDeclaration(current.directive.node)) {
continue;
}

const symbol = this.getSymbolOfTsNode(current.directive.node);
const meta = this.getDirectiveMeta(host, current.directive.node);

if (meta !== null && symbol !== null && isSymbolWithValueDeclaration(symbol.tsSymbol)) {
if (meta.hostDirectives !== null) {
this.addHostDirectiveSymbols(host, meta.hostDirectives, symbols);
}

const directiveSymbol: DirectiveSymbol = {
...symbol,
isHostDirective: true,
ref: current.directive,
tsSymbol: symbol.tsSymbol,
exposedInputs: current.inputs,
exposedOutputs: current.outputs,
// TODO(crisbeto): rework `DirectiveSymbol` to make
// `selector` nullable and remove the `|| ''` here.
selector: meta.selector || '',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The || '' here is weird, but I don't know the context behind why the language service requires a selector for all directives, even though directives can be declared without selectors.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before the directive composition, a directive couldn't match anything unless it had a selector. I think the correct thing would be to make this optional now. Can you add a TODO or make the change if it's easy enough?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a TODO for myself. I'll send a follow-up PR once this one goes in.

isComponent: meta.isComponent,
ngModule: this.getDirectiveModule(current.directive.node),
kind: SymbolKind.Directive,
isStructural: meta.isStructural,
isInScope: true,
};

symbols.push(directiveSymbol);
}
}
}

private getDirectiveMeta(
Expand Down Expand Up @@ -376,6 +419,7 @@ export class SymbolBuilder {
isStructural,
selector,
ngModule,
isHostDirective: false,
isInScope: true, // TODO: this should always be in scope in this context, right?
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts
Expand Up @@ -228,7 +228,7 @@ export interface TestDirective extends Partial<Pick<
Exclude<
keyof TypeCheckableDirectiveMeta,
'ref'|'coercedInputFields'|'restrictedInputFields'|'stringLiteralInputFields'|
'undeclaredInputFields'|'inputs'|'outputs'>>> {
'undeclaredInputFields'|'inputs'|'outputs'|'hostDirectives'>>> {
selector: string;
name: string;
file?: AbsoluteFsPath;
Expand Down
26 changes: 24 additions & 2 deletions packages/language-service/src/attribute_completions.ts
Expand Up @@ -220,7 +220,18 @@ export function buildAttributeCompletionTable(
continue;
}

for (const [classPropertyName, propertyName] of meta.inputs) {
for (const [classPropertyName, rawProperyName] of meta.inputs) {
let propertyName: string;

if (dirSymbol.isHostDirective) {
if (!dirSymbol.exposedInputs?.hasOwnProperty(rawProperyName)) {
continue;
}
propertyName = dirSymbol.exposedInputs[rawProperyName];
} else {
propertyName = rawProperyName;
}

if (table.has(propertyName)) {
continue;
}
Expand All @@ -234,7 +245,18 @@ export function buildAttributeCompletionTable(
});
}

for (const [classPropertyName, propertyName] of meta.outputs) {
for (const [classPropertyName, rawProperyName] of meta.outputs) {
let propertyName: string;

if (dirSymbol.isHostDirective) {
if (!dirSymbol.exposedOutputs?.hasOwnProperty(rawProperyName)) {
continue;
}
propertyName = dirSymbol.exposedOutputs[rawProperyName];
} else {
propertyName = rawProperyName;
}

if (table.has(propertyName)) {
continue;
}
Expand Down