Skip to content
Permalink
Browse files

fix(core): static-query migration should handle queries on accessors (#…

…30327)

Currently the static-query migration ignores queries declared on getters
or setters as these are not part of a `PropertyDeclaration`. We need to
handle these queries in order to cover all queries within a given project.

The usage strategy is not able to detect timing for queries on accessors,
so we add a TODO and print a message. The template strategy is able
to detect the proper timing for such queries because it's not dependent
on detecting the usage of the query.

Resolves FW-1215

PR Close #30327
  • Loading branch information...
devversion authored and alxhub committed May 8, 2019
1 parent cb6ad97 commit 0ffdb48f62458b6a9853cbdd97fa1050ba89a674
@@ -11,6 +11,7 @@ import * as ts from 'typescript';
import {ResolvedTemplate} from '../../../utils/ng_component_template';
import {getAngularDecorators} from '../../../utils/ng_decorators';
import {findParentClassDeclaration, getBaseTypeIdentifiers} from '../../../utils/typescript/class_declaration';
import {getPropertyNameText} from '../../../utils/typescript/property_name';

import {getInputNamesOfClass} from './directive_inputs';
import {NgQueryDefinition, QueryType} from './query-definition';
@@ -53,12 +54,30 @@ export class NgQueryResolveVisitor {
case ts.SyntaxKind.ClassDeclaration:
this.visitClassDeclaration(node as ts.ClassDeclaration);
break;
case ts.SyntaxKind.GetAccessor:
case ts.SyntaxKind.SetAccessor:
this.visitAccessorDeclaration(node as ts.AccessorDeclaration);
break;
}

ts.forEachChild(node, n => this.visitNode(n));
}

private visitPropertyDeclaration(node: ts.PropertyDeclaration) {
this._recordQueryDeclaration(node, node, getPropertyNameText(node.name));
}

private visitAccessorDeclaration(node: ts.AccessorDeclaration) {
this._recordQueryDeclaration(node, null, getPropertyNameText(node.name));
}

private visitClassDeclaration(node: ts.ClassDeclaration) {
this._recordClassInputSetters(node);
this._recordClassInheritances(node);
}

private _recordQueryDeclaration(
node: ts.Node, property: ts.PropertyDeclaration|null, queryName: string|null) {
if (!node.decorators || !node.decorators.length) {
return;
}
@@ -83,18 +102,15 @@ export class NgQueryResolveVisitor {
const newQueries = this.resolvedQueries.get(sourceFile) || [];

this.resolvedQueries.set(sourceFile, newQueries.concat({
name: queryName,
type: queryDecorator.name === 'ViewChild' ? QueryType.ViewChild : QueryType.ContentChild,
property: node,
node,
property,
decorator: queryDecorator,
container: queryContainer,
}));
}

private visitClassDeclaration(node: ts.ClassDeclaration) {
this._recordClassInputSetters(node);
this._recordClassInheritances(node);
}

private _recordClassInputSetters(node: ts.ClassDeclaration) {
const resolvedInputNames = getInputNamesOfClass(node, this.typeChecker);

@@ -22,10 +22,17 @@ export enum QueryType {
}

export interface NgQueryDefinition {
/** Name of the query. Set to "null" in case the query name is not statically analyzable. */
name: string|null;
/** Type of the query definition. */
type: QueryType;
/** Property that declares the query. */
property: ts.PropertyDeclaration;
/** Node that declares this query. */
node: ts.Node;
/**
* Property declaration that refers to the query value. For accessors there
* is no property that is guaranteed to access the query value.
*/
property: ts.PropertyDeclaration|null;
/** Decorator that declares this as a query. */
decorator: NgDecorator;
/** Class declaration that holds this query. */
@@ -99,13 +99,13 @@ export class QueryTemplateStrategy implements TimingStrategy {
detectTiming(query: NgQueryDefinition): TimingResult {
if (query.type === QueryType.ContentChild) {
return {timing: null, message: 'Content queries cannot be migrated automatically.'};
} else if (!hasPropertyNameText(query.property.name)) {
} else if (!query.name) {
// In case the query property name is not statically analyzable, we mark this
// query as unresolved. NGC currently skips these view queries as well.
return {timing: null, message: 'Query is not statically analyzable.'};
}

const propertyName = query.property.name.text;
const propertyName = query.name;
const classMetadata = this.classMetadata.get(query.container);

// In case there is no class metadata or there are no derived classes that
@@ -44,6 +44,10 @@ export class QueryUsageStrategy implements TimingStrategy {
* on the current usage of the query.
*/
detectTiming(query: NgQueryDefinition): TimingResult {
if (query.property === null) {
return {timing: null, message: 'Queries defined on accessors cannot be analyzed.'};
}

return {
timing:
isQueryUsedStatically(query.container, query, this.classMetadata, this.typeChecker, []) ?
@@ -62,7 +66,7 @@ function isQueryUsedStatically(
classDecl: ts.ClassDeclaration, query: NgQueryDefinition, classMetadataMap: ClassMetadataMap,
typeChecker: ts.TypeChecker, knownInputNames: string[],
functionCtx: FunctionContext = new Map(), visitInheritedClasses = true): boolean {
const usageVisitor = new DeclarationUsageVisitor(query.property, typeChecker, functionCtx);
const usageVisitor = new DeclarationUsageVisitor(query.property !, typeChecker, functionCtx);
const classMetadata = classMetadataMap.get(classDecl);

// In case there is metadata for the current class, we collect all resolved Angular input
@@ -90,10 +94,10 @@ function isQueryUsedStatically(
// In case there is a component template for the current class, we check if the
// template statically accesses the current query. In case that's true, the query
// can be marked as static.
if (classMetadata.template && hasPropertyNameText(query.property.name)) {
if (classMetadata.template && hasPropertyNameText(query.property !.name)) {
const template = classMetadata.template;
const parsedHtml = parseHtmlGracefully(template.content, template.filePath);
const htmlVisitor = new TemplateUsageVisitor(query.property.name.text);
const htmlVisitor = new TemplateUsageVisitor(query.property !.name.text);

if (parsedHtml && htmlVisitor.isQueryUsedStatically(parsedHtml)) {
return true;
@@ -401,6 +401,55 @@ describe('static-queries migration with template strategy', () => {
.toContain(`@ViewChild('myRef', { static: true }) query: any;`);
});

it('should detect queries declared on setter', async() => {
writeFile('/index.ts', `
import {Component, NgModule, ViewChild} from '@angular/core';
@Component({templateUrl: 'my-tmpl.html'})
export class MyComp {
@ViewChild('myRef')
set query(result: any) { /* noop */}
}
@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

writeFile(`/my-tmpl.html`, `
<span #myRef>My Ref</span>
`);

await runMigration();

expect(tree.readContent('/index.ts'))
.toMatch(/@ViewChild\('myRef', { static: true }\)\s+set query/);
});

it('should detect queries declared on getter', async() => {
writeFile('/index.ts', `
import {Component, NgModule, ViewChild} from '@angular/core';
@Component({templateUrl: 'my-tmpl.html'})
export class MyComp {
@ViewChild('myRef')
get query() { return null; }
set query(result: any) { /* noop */}
}
@NgModule({declarations: [MyComp]})
export class MyModule {}
`);

writeFile(`/my-tmpl.html`, `
<span #myRef>My Ref</span>
`);

await runMigration();

expect(tree.readContent('/index.ts'))
.toMatch(/@ViewChild\('myRef', { static: true }\)\s+get query/);
});

it('should add a todo if a query is not declared in any component', async() => {
writeFile('/index.ts', `
import {Component, NgModule, ViewChild, SomeToken} from '@angular/core';
@@ -18,6 +18,7 @@ describe('static-queries migration with usage strategy', () => {
let tree: UnitTestTree;
let tmpDirPath: string;
let previousWorkingDir: string;
let warnOutput: string[] = [];

// Enables the query usage strategy when running the `static-query` migration. By
// default the schematic runs the template strategy and there is currently no easy
@@ -39,6 +40,13 @@ describe('static-queries migration with usage strategy', () => {
projects: {t: {architect: {build: {options: {tsConfig: './tsconfig.json'}}}}}
}));

warnOutput = [];
runner.logger.subscribe(logEntry => {
if (logEntry.level === 'warn') {
warnOutput.push(logEntry.message);
}
});

previousWorkingDir = shx.pwd();
tmpDirPath = getSystemPath(host.root);

@@ -252,6 +260,47 @@ describe('static-queries migration with usage strategy', () => {
.toContain(`@${queryType}('test', { /* test */ read: null, static: true }) query: any;`);
});

it('should add a todo for queries declared on setter', async() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
@${queryType}('test')
set query(result: any) {};
}
`);

await runMigration();

expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', /* TODO: add static flag */ {})`);
expect(warnOutput.length).toBe(1);
expect(warnOutput[0])
.toMatch(/index.ts@6:11: Queries defined on accessors cannot be analyzed.$/);
});

it('should add a todo for queries declared on getter', async() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';
@Component({template: '<span #test></span>'})
export class MyComp {
@${queryType}('test')
get query() { return null; }
set query(result: any) {}
}
`);

await runMigration();

expect(tree.readContent('/index.ts'))
.toContain(`@${queryType}('test', /* TODO: add static flag */ {})`);
expect(warnOutput.length).toBe(1);
expect(warnOutput[0])
.toMatch(/index.ts@6:11: Queries defined on accessors cannot be analyzed.$/);
});

it('should not overwrite existing explicit query timing', async() => {
writeFile('/index.ts', `
import {Component, ${queryType}} from '@angular/core';

0 comments on commit 0ffdb48

Please sign in to comment.
You can’t perform that action at this time.