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
angular · May 9, 2019 · dd299f9 · dd299f9
1 parent 49ec3f3
commit dd299f9
Show file tree

Hide file tree

Showing 6 changed files with 138 additions and 13 deletions.
diff --git a/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts b/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts
@@ -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);
 

diff --git a/packages/core/schematics/migrations/static-queries/angular/query-definition.ts b/packages/core/schematics/migrations/static-queries/angular/query-definition.ts
@@ -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. */

diff --git a/...re/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts b/...re/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts
@@ -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

diff --git a/...ges/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts b/...ges/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts
@@ -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;

diff --git a/packages/core/schematics/test/static_queries_migration_template_spec.ts b/packages/core/schematics/test/static_queries_migration_template_spec.ts
@@ -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';

diff --git a/packages/core/schematics/test/static_queries_migration_usage_spec.ts b/packages/core/schematics/test/static_queries_migration_usage_spec.ts
@@ -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';