Skip to content

Commit

Permalink
fix(compiler): correct confusion between field and property names
Browse files Browse the repository at this point in the history
The `R3TargetBinder` accepts an interface for directive metadata which
declares types for `input` and `output` objects. These types convey the
mapping between the property names for an input or output and the
corresponding field name on the component class. Due to `R3TargetBinder`'s
requirements, this mapping was specified with property names as keys and
field names as values.

However, because of duck typing, this interface was accidentally satisifed
by the opposite mapping, of field names to property names, that was produced
in other parts of the compiler. This form more naturally represents the data
model for inputs.

Rather than accept the field -> property mapping and invert it, this commit
introduces a new abstraction for field to property mappings which is
bidirectional, eliminating the ambiguous plain object type. This mapping can
be used to satisfy both the needs of the binder (property -> field) as well
as those of the template type-checker (field -> property).

A new test ensures that the input/output metadata produced by the compiler
during analysis is directly compatible with the binder via this unambiguous
new interface.
  • Loading branch information
alxhub committed Sep 2, 2020
1 parent 59c234c commit 0f0bbea
Show file tree
Hide file tree
Showing 19 changed files with 360 additions and 77 deletions.
12 changes: 9 additions & 3 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Expand Up @@ -16,6 +16,7 @@ import {DefaultImportRecorder, ModuleResolver, Reference, ReferenceEmitter} from
import {DependencyTracker} from '../../incremental/api';
import {IndexingContext} from '../../indexer';
import {DirectiveMeta, DirectiveTypeCheckMeta, extractDirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
import {FieldPropertyMapping} from '../../metadata';
import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance';
import {EnumValue, PartialEvaluator} from '../../partial_evaluator';
import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
Expand Down Expand Up @@ -55,6 +56,9 @@ export interface ComponentAnalysisData {
template: ParsedTemplateWithSource;
metadataStmt: Statement|null;

inputs: FieldPropertyMapping;
outputs: FieldPropertyMapping;

/**
* Providers extracted from the `providers` field of the component annotation which will require
* an Angular factory definition at runtime.
Expand Down Expand Up @@ -190,7 +194,7 @@ export class ComponentDecoratorHandler implements
}

// Next, read the `@Component`-specific fields.
const {decorator: component, metadata} = directiveResult;
const {decorator: component, metadata, inputs, outputs} = directiveResult;

// Go through the root directories for this project, and select the one with the smallest
// relative path representation.
Expand Down Expand Up @@ -310,6 +314,8 @@ export class ComponentDecoratorHandler implements
const output: AnalysisOutput<ComponentAnalysisData> = {
analysis: {
baseClass: readBaseClass(node, this.reflector, this.evaluator),
inputs,
outputs,
meta: {
...metadata,
template: {
Expand Down Expand Up @@ -351,8 +357,8 @@ export class ComponentDecoratorHandler implements
name: node.name.text,
selector: analysis.meta.selector,
exportAs: analysis.meta.exportAs,
inputs: analysis.meta.inputs,
outputs: analysis.meta.outputs,
inputs: analysis.inputs,
outputs: analysis.outputs,
queries: analysis.meta.queries.map(query => query.propertyName),
isComponent: true,
baseClass: analysis.baseClass,
Expand Down
39 changes: 29 additions & 10 deletions packages/compiler-cli/src/ngtsc/annotations/src/directive.ts
Expand Up @@ -12,6 +12,7 @@ import * as ts from 'typescript';
import {ErrorCode, FatalDiagnosticError} from '../../diagnostics';
import {DefaultImportRecorder, Reference} from '../../imports';
import {DirectiveTypeCheckMeta, InjectableClassRegistry, MetadataReader, MetadataRegistry} from '../../metadata';
import {FieldPropertyMapping} from '../../metadata';
import {extractDirectiveTypeCheckMeta} from '../../metadata/src/util';
import {DynamicValue, EnumValue, PartialEvaluator} from '../../partial_evaluator';
import {ClassDeclaration, ClassMember, ClassMemberKind, Decorator, filterToMembersWithDecorator, ReflectionHost, reflectObjectLiteral} from '../../reflection';
Expand Down Expand Up @@ -39,6 +40,8 @@ export interface DirectiveHandlerData {
meta: R3DirectiveMetadata;
metadataStmt: Statement|null;
providersRequiringFactory: Set<Reference<ClassDeclaration>>|null;
inputs: FieldPropertyMapping;
outputs: FieldPropertyMapping;
}

export class DirectiveDecoratorHandler implements
Expand Down Expand Up @@ -83,11 +86,10 @@ export class DirectiveDecoratorHandler implements
const directiveResult = extractDirectiveMetadata(
node, decorator, this.reflector, this.evaluator, this.defaultImportRecorder, this.isCore,
flags, this.annotateForClosureCompiler);
const analysis = directiveResult && directiveResult.metadata;

if (analysis === undefined) {
if (directiveResult === undefined) {
return {};
}
const analysis = directiveResult.metadata;

let providersRequiringFactory: Set<Reference<ClassDeclaration>>|null = null;
if (directiveResult !== undefined && directiveResult.decorator.has('providers')) {
Expand All @@ -97,6 +99,8 @@ export class DirectiveDecoratorHandler implements

return {
analysis: {
inputs: directiveResult.inputs,
outputs: directiveResult.outputs,
meta: analysis,
metadataStmt: generateSetClassMetadataCall(
node, this.reflector, this.defaultImportRecorder, this.isCore,
Expand All @@ -117,8 +121,8 @@ export class DirectiveDecoratorHandler implements
name: node.name.text,
selector: analysis.meta.selector,
exportAs: analysis.meta.exportAs,
inputs: analysis.meta.inputs,
outputs: analysis.meta.outputs,
inputs: analysis.inputs,
outputs: analysis.outputs,
queries: analysis.meta.queries.map(query => query.propertyName),
isComponent: false,
baseClass: analysis.baseClass,
Expand Down Expand Up @@ -199,8 +203,13 @@ export class DirectiveDecoratorHandler implements
export function extractDirectiveMetadata(
clazz: ClassDeclaration, decorator: Readonly<Decorator|null>, reflector: ReflectionHost,
evaluator: PartialEvaluator, defaultImportRecorder: DefaultImportRecorder, isCore: boolean,
flags: HandlerFlags, annotateForClosureCompiler: boolean, defaultSelector: string|null = null):
{decorator: Map<string, ts.Expression>, metadata: R3DirectiveMetadata}|undefined {
flags: HandlerFlags, annotateForClosureCompiler: boolean,
defaultSelector: string|null = null): {
decorator: Map<string, ts.Expression>,
metadata: R3DirectiveMetadata,
inputs: FieldPropertyMapping,
outputs: FieldPropertyMapping,
}|undefined {
let directive: Map<string, ts.Expression>;
if (decorator === null || decorator.args === null || decorator.args.length === 0) {
directive = new Map<string, ts.Expression>();
Expand Down Expand Up @@ -331,15 +340,20 @@ export function extractDirectiveMetadata(
const type = wrapTypeReference(reflector, clazz);
const internalType = new WrappedNodeExpr(reflector.getInternalNameOfClass(clazz));

const inputs =
FieldPropertyMapping.fromFieldMappedObject({...inputsFromMeta, ...inputsFromFields});
const outputs =
FieldPropertyMapping.fromFieldMappedObject({...outputsFromMeta, ...outputsFromFields});

const metadata: R3DirectiveMetadata = {
name: clazz.name.text,
deps: ctorDeps,
host,
lifecycle: {
usesOnChanges,
},
inputs: {...inputsFromMeta, ...inputsFromFields},
outputs: {...outputsFromMeta, ...outputsFromFields},
inputs: inputs.toFieldMappedObject(),
outputs: outputs.toFieldPropertyMappedObject(),
queries,
viewQueries,
selector,
Expand All @@ -352,7 +366,12 @@ export function extractDirectiveMetadata(
exportAs,
providers
};
return {decorator: directive, metadata};
return {
decorator: directive,
metadata,
inputs,
outputs,
};
}

export function extractQueryMetadata(
Expand Down
44 changes: 44 additions & 0 deletions packages/compiler-cli/src/ngtsc/annotations/test/directive_spec.ts
Expand Up @@ -5,6 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {CssSelector, DirectiveMeta as T2DirectiveMeta, parseTemplate, R3TargetBinder, SelectorMatcher, TmplAstElement} from '@angular/compiler';
import * as ts from 'typescript';

import {absoluteFrom} from '../../file_system';
Expand Down Expand Up @@ -73,6 +74,49 @@ runInEachFileSystem(() => {
expect(span.start.toString()).toContain('/entry.ts@5:22');
expect(span.end.toString()).toContain('/entry.ts@5:29');
});

it('should produce metadata compatible with template binding', () => {
const src = `
import {Directive, Input} from '@angular/core';
@Directive({selector: '[dir]'})
export class TestDir {
@Input('propName')
fieldName: string;
}
`;
const {program} = makeProgram([
{
name: _('/node_modules/@angular/core/index.d.ts'),
contents: 'export const Directive: any; export const Input: any;',
},
{
name: _('/entry.ts'),
contents: src,
},
]);

const analysis = analyzeDirective(program, 'TestDir');
const matcher = new SelectorMatcher<T2DirectiveMeta>();
const dirMeta: T2DirectiveMeta = {
exportAs: null,
inputs: analysis.inputs,
outputs: analysis.outputs,
isComponent: false,
name: 'Dir',
};
matcher.addSelectables(CssSelector.parse('[dir]'), dirMeta);

const {nodes} = parseTemplate('<div dir [propName]="expr"></div>', 'unimportant.html');
const binder = new R3TargetBinder(matcher).bind({template: nodes});
const propBinding = (nodes[0] as TmplAstElement).inputs[0];
const propBindingConsumer = binder.getConsumerOfBinding(propBinding);

// Assert that the consumer of the binding is the directive, which means that the metadata
// fed into the SelectorMatcher was compatible with the binder, and did not confuse property
// and field names.
expect(propBindingConsumer).toBe(dirMeta);
});
});

// Helpers
Expand Down
5 changes: 3 additions & 2 deletions packages/compiler-cli/src/ngtsc/indexer/test/util.ts
Expand Up @@ -11,6 +11,7 @@ import * as ts from 'typescript';

import {absoluteFrom, AbsoluteFsPath} from '../../file_system';
import {Reference} from '../../imports';
import {FieldPropertyMapping} from '../../metadata';
import {ClassDeclaration} from '../../reflection';
import {getDeclaration, makeProgram} from '../../testing';
import {ComponentMeta} from '../src/context';
Expand Down Expand Up @@ -50,8 +51,8 @@ export function getBoundTemplate(
selector,
name: declaration.name.getText(),
isComponent: true,
inputs: {},
outputs: {},
inputs: FieldPropertyMapping.fromFieldMappedObject({}),
outputs: FieldPropertyMapping.fromFieldMappedObject({}),
exportAs: null,
});
});
Expand Down
1 change: 1 addition & 0 deletions packages/compiler-cli/src/ngtsc/metadata/index.ts
Expand Up @@ -10,3 +10,4 @@ export * from './src/api';
export {DtsMetadataReader} from './src/dts';
export {CompoundMetadataRegistry, LocalMetadataRegistry, InjectableClassRegistry} from './src/registry';
export {extractDirectiveTypeCheckMeta, CompoundMetadataReader} from './src/util';
export {FieldPropertyMapping, InputOrOutput} from './src/field_mapping';
12 changes: 12 additions & 0 deletions packages/compiler-cli/src/ngtsc/metadata/src/api.ts
Expand Up @@ -12,6 +12,8 @@ import * as ts from 'typescript';
import {Reference} from '../../imports';
import {ClassDeclaration} from '../../reflection';

import {FieldPropertyMapping} from './field_mapping';


/**
* Metadata collected for an `NgModule`.
Expand Down Expand Up @@ -89,6 +91,16 @@ export interface DirectiveMeta extends T2DirectiveMeta, DirectiveTypeCheckMeta {
selector: string|null;
queries: string[];

/**
* A mapping of input field names to the property names.
*/
inputs: FieldPropertyMapping;

/**
* A mapping of output field names to the property names.
*/
outputs: FieldPropertyMapping;

/**
* A `Reference` to the base class for the directive, if one was detected.
*
Expand Down
6 changes: 4 additions & 2 deletions packages/compiler-cli/src/ngtsc/metadata/src/dts.ts
Expand Up @@ -12,6 +12,7 @@ import {Reference} from '../../imports';
import {ClassDeclaration, isNamedClassDeclaration, ReflectionHost} from '../../reflection';

import {DirectiveMeta, MetadataReader, NgModuleMeta, PipeMeta} from './api';
import {FieldPropertyMapping} from './field_mapping';
import {extractDirectiveTypeCheckMeta, extractReferencesFromType, readStringArrayType, readStringMapType, readStringType} from './util';

/**
Expand Down Expand Up @@ -77,14 +78,15 @@ export class DtsMetadataReader implements MetadataReader {
}

const inputs = readStringMapType(def.type.typeArguments[3]);
const outputs = readStringMapType(def.type.typeArguments[4]);
return {
ref,
name: clazz.name.text,
isComponent: def.name === 'ɵcmp',
selector: readStringType(def.type.typeArguments[1]),
exportAs: readStringArrayType(def.type.typeArguments[2]),
inputs,
outputs: readStringMapType(def.type.typeArguments[4]),
inputs: FieldPropertyMapping.fromFieldMappedObject(inputs),
outputs: FieldPropertyMapping.fromFieldMappedObject(outputs),
queries: readStringArrayType(def.type.typeArguments[5]),
...extractDirectiveTypeCheckMeta(clazz, inputs, this.reflector),
baseClass: readBaseClass(clazz, this.checker, this.reflector),
Expand Down

0 comments on commit 0f0bbea

Please sign in to comment.