Skip to content

Commit 0ffdb48

Browse files
devversionalxhub
devversion
authored and
alxhub
committed
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
1 parent cb6ad97 commit 0ffdb48

6 files changed

Lines changed: 138 additions & 13 deletions

File tree

‎packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import * as ts from 'typescript';
1111
import {ResolvedTemplate} from '../../../utils/ng_component_template';
1212
import {getAngularDecorators} from '../../../utils/ng_decorators';
1313
import {findParentClassDeclaration, getBaseTypeIdentifiers} from '../../../utils/typescript/class_declaration';
14+
import {getPropertyNameText} from '../../../utils/typescript/property_name';
1415

1516
import {getInputNamesOfClass} from './directive_inputs';
1617
import {NgQueryDefinition, QueryType} from './query-definition';
@@ -53,12 +54,30 @@ export class NgQueryResolveVisitor {
5354
case ts.SyntaxKind.ClassDeclaration:
5455
this.visitClassDeclaration(node as ts.ClassDeclaration);
5556
break;
57+
case ts.SyntaxKind.GetAccessor:
58+
case ts.SyntaxKind.SetAccessor:
59+
this.visitAccessorDeclaration(node as ts.AccessorDeclaration);
60+
break;
5661
}
5762

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

6166
private visitPropertyDeclaration(node: ts.PropertyDeclaration) {
67+
this._recordQueryDeclaration(node, node, getPropertyNameText(node.name));
68+
}
69+
70+
private visitAccessorDeclaration(node: ts.AccessorDeclaration) {
71+
this._recordQueryDeclaration(node, null, getPropertyNameText(node.name));
72+
}
73+
74+
private visitClassDeclaration(node: ts.ClassDeclaration) {
75+
this._recordClassInputSetters(node);
76+
this._recordClassInheritances(node);
77+
}
78+
79+
private _recordQueryDeclaration(
80+
node: ts.Node, property: ts.PropertyDeclaration|null, queryName: string|null) {
6281
if (!node.decorators || !node.decorators.length) {
6382
return;
6483
}
@@ -83,18 +102,15 @@ export class NgQueryResolveVisitor {
83102
const newQueries = this.resolvedQueries.get(sourceFile) || [];
84103

85104
this.resolvedQueries.set(sourceFile, newQueries.concat({
105+
name: queryName,
86106
type: queryDecorator.name === 'ViewChild' ? QueryType.ViewChild : QueryType.ContentChild,
87-
property: node,
107+
node,
108+
property,
88109
decorator: queryDecorator,
89110
container: queryContainer,
90111
}));
91112
}
92113

93-
private visitClassDeclaration(node: ts.ClassDeclaration) {
94-
this._recordClassInputSetters(node);
95-
this._recordClassInheritances(node);
96-
}
97-
98114
private _recordClassInputSetters(node: ts.ClassDeclaration) {
99115
const resolvedInputNames = getInputNamesOfClass(node, this.typeChecker);
100116

‎packages/core/schematics/migrations/static-queries/angular/query-definition.ts‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,17 @@ export enum QueryType {
2222
}
2323

2424
export interface NgQueryDefinition {
25+
/** Name of the query. Set to "null" in case the query name is not statically analyzable. */
26+
name: string|null;
2527
/** Type of the query definition. */
2628
type: QueryType;
27-
/** Property that declares the query. */
28-
property: ts.PropertyDeclaration;
29+
/** Node that declares this query. */
30+
node: ts.Node;
31+
/**
32+
* Property declaration that refers to the query value. For accessors there
33+
* is no property that is guaranteed to access the query value.
34+
*/
35+
property: ts.PropertyDeclaration|null;
2936
/** Decorator that declares this as a query. */
3037
decorator: NgDecorator;
3138
/** Class declaration that holds this query. */

‎packages/core/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,13 @@ export class QueryTemplateStrategy implements TimingStrategy {
9999
detectTiming(query: NgQueryDefinition): TimingResult {
100100
if (query.type === QueryType.ContentChild) {
101101
return {timing: null, message: 'Content queries cannot be migrated automatically.'};
102-
} else if (!hasPropertyNameText(query.property.name)) {
102+
} else if (!query.name) {
103103
// In case the query property name is not statically analyzable, we mark this
104104
// query as unresolved. NGC currently skips these view queries as well.
105105
return {timing: null, message: 'Query is not statically analyzable.'};
106106
}
107107

108-
const propertyName = query.property.name.text;
108+
const propertyName = query.name;
109109
const classMetadata = this.classMetadata.get(query.container);
110110

111111
// In case there is no class metadata or there are no derived classes that

‎packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,10 @@ export class QueryUsageStrategy implements TimingStrategy {
4444
* on the current usage of the query.
4545
*/
4646
detectTiming(query: NgQueryDefinition): TimingResult {
47+
if (query.property === null) {
48+
return {timing: null, message: 'Queries defined on accessors cannot be analyzed.'};
49+
}
50+
4751
return {
4852
timing:
4953
isQueryUsedStatically(query.container, query, this.classMetadata, this.typeChecker, []) ?
@@ -62,7 +66,7 @@ function isQueryUsedStatically(
6266
classDecl: ts.ClassDeclaration, query: NgQueryDefinition, classMetadataMap: ClassMetadataMap,
6367
typeChecker: ts.TypeChecker, knownInputNames: string[],
6468
functionCtx: FunctionContext = new Map(), visitInheritedClasses = true): boolean {
65-
const usageVisitor = new DeclarationUsageVisitor(query.property, typeChecker, functionCtx);
69+
const usageVisitor = new DeclarationUsageVisitor(query.property !, typeChecker, functionCtx);
6670
const classMetadata = classMetadataMap.get(classDecl);
6771

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

98102
if (parsedHtml && htmlVisitor.isQueryUsedStatically(parsedHtml)) {
99103
return true;

‎packages/core/schematics/test/static_queries_migration_template_spec.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,55 @@ describe('static-queries migration with template strategy', () => {
401401
.toContain(`@ViewChild('myRef', { static: true }) query: any;`);
402402
});
403403

404+
it('should detect queries declared on setter', async() => {
405+
writeFile('/index.ts', `
406+
import {Component, NgModule, ViewChild} from '@angular/core';
407+
408+
@Component({templateUrl: 'my-tmpl.html'})
409+
export class MyComp {
410+
@ViewChild('myRef')
411+
set query(result: any) { /* noop */}
412+
}
413+
414+
@NgModule({declarations: [MyComp]})
415+
export class MyModule {}
416+
`);
417+
418+
writeFile(`/my-tmpl.html`, `
419+
<span #myRef>My Ref</span>
420+
`);
421+
422+
await runMigration();
423+
424+
expect(tree.readContent('/index.ts'))
425+
.toMatch(/@ViewChild\('myRef', { static: true }\)\s+set query/);
426+
});
427+
428+
it('should detect queries declared on getter', async() => {
429+
writeFile('/index.ts', `
430+
import {Component, NgModule, ViewChild} from '@angular/core';
431+
432+
@Component({templateUrl: 'my-tmpl.html'})
433+
export class MyComp {
434+
@ViewChild('myRef')
435+
get query() { return null; }
436+
set query(result: any) { /* noop */}
437+
}
438+
439+
@NgModule({declarations: [MyComp]})
440+
export class MyModule {}
441+
`);
442+
443+
writeFile(`/my-tmpl.html`, `
444+
<span #myRef>My Ref</span>
445+
`);
446+
447+
await runMigration();
448+
449+
expect(tree.readContent('/index.ts'))
450+
.toMatch(/@ViewChild\('myRef', { static: true }\)\s+get query/);
451+
});
452+
404453
it('should add a todo if a query is not declared in any component', async() => {
405454
writeFile('/index.ts', `
406455
import {Component, NgModule, ViewChild, SomeToken} from '@angular/core';

‎packages/core/schematics/test/static_queries_migration_usage_spec.ts‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ describe('static-queries migration with usage strategy', () => {
1818
let tree: UnitTestTree;
1919
let tmpDirPath: string;
2020
let previousWorkingDir: string;
21+
let warnOutput: string[] = [];
2122

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

43+
warnOutput = [];
44+
runner.logger.subscribe(logEntry => {
45+
if (logEntry.level === 'warn') {
46+
warnOutput.push(logEntry.message);
47+
}
48+
});
49+
4250
previousWorkingDir = shx.pwd();
4351
tmpDirPath = getSystemPath(host.root);
4452

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

263+
it('should add a todo for queries declared on setter', async() => {
264+
writeFile('/index.ts', `
265+
import {Component, ${queryType}} from '@angular/core';
266+
267+
@Component({template: '<span #test></span>'})
268+
export class MyComp {
269+
@${queryType}('test')
270+
set query(result: any) {};
271+
}
272+
`);
273+
274+
await runMigration();
275+
276+
expect(tree.readContent('/index.ts'))
277+
.toContain(`@${queryType}('test', /* TODO: add static flag */ {})`);
278+
expect(warnOutput.length).toBe(1);
279+
expect(warnOutput[0])
280+
.toMatch(/index.ts@6:11: Queries defined on accessors cannot be analyzed.$/);
281+
});
282+
283+
it('should add a todo for queries declared on getter', async() => {
284+
writeFile('/index.ts', `
285+
import {Component, ${queryType}} from '@angular/core';
286+
287+
@Component({template: '<span #test></span>'})
288+
export class MyComp {
289+
@${queryType}('test')
290+
get query() { return null; }
291+
set query(result: any) {}
292+
}
293+
`);
294+
295+
await runMigration();
296+
297+
expect(tree.readContent('/index.ts'))
298+
.toContain(`@${queryType}('test', /* TODO: add static flag */ {})`);
299+
expect(warnOutput.length).toBe(1);
300+
expect(warnOutput[0])
301+
.toMatch(/index.ts@6:11: Queries defined on accessors cannot be analyzed.$/);
302+
});
303+
255304
it('should not overwrite existing explicit query timing', async() => {
256305
writeFile('/index.ts', `
257306
import {Component, ${queryType}} from '@angular/core';

0 commit comments

Comments
 (0)