From 4c12d742dc122068976b4e1c66cb44ced4117a85 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 3 May 2019 15:02:08 +0200 Subject: [PATCH] fix(core): static-query migration should not prompt if no queries are used (#30254) Currently we always prompt when the static-query migration runs. This is not always needed because some applications do not even use `ViewChild` or `ContentChild` queries and it just causes confusion if developers need to decide on a migration strategy while there is nothing to migrate. In order to avoid this confusion, we no longer prompt for a strategy if there are no queries declared within the project. PR Close #30254 --- .../angular/ng_query_visitor.ts | 2 + .../google3/explicitQueryTimingRule.ts | 7 +- .../migrations/static-queries/index.ts | 144 +++++++++--------- .../static-queries/strategy_prompt.ts | 50 ++++++ .../noTemplateVariableAssignmentRule.ts | 3 +- .../template-var-assignment/index.ts | 3 +- .../static_queries_migration_usage_spec.ts | 17 +++ .../schematics/utils/ng_component_template.ts | 2 + .../utils/typescript/visit_nodes.ts | 16 -- 9 files changed, 147 insertions(+), 97 deletions(-) create mode 100644 packages/core/schematics/migrations/static-queries/strategy_prompt.ts delete mode 100644 packages/core/schematics/utils/typescript/visit_nodes.ts 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 index 9b77c61e43..a413099724 100644 --- a/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts +++ b/packages/core/schematics/migrations/static-queries/angular/ng_query_visitor.ts @@ -54,6 +54,8 @@ export class NgQueryResolveVisitor { this.visitClassDeclaration(node as ts.ClassDeclaration); break; } + + ts.forEachChild(node, n => this.visitNode(n)); } private visitPropertyDeclaration(node: ts.PropertyDeclaration) { diff --git a/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts b/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts index 27af87fb73..5bed32571a 100644 --- a/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts +++ b/packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts @@ -10,7 +10,6 @@ import {Replacement, RuleFailure, Rules} from 'tslint'; import * as ts from 'typescript'; import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template'; -import {visitAllNodes} from '../../../utils/typescript/visit_nodes'; import {NgQueryResolveVisitor} from '../angular/ng_query_visitor'; import {QueryTiming} from '../angular/query-definition'; import {QueryUsageStrategy} from '../strategies/usage_strategy/usage_strategy'; @@ -35,10 +34,8 @@ export class Rule extends Rules.TypedRule { // Analyze source files by detecting queries, class relations and component templates. rootSourceFiles.forEach(sourceFile => { - // The visit utility function only traverses the source file once. We don't want to - // traverse through all source files multiple times for each visitor as this could be - // slow. - visitAllNodes(sourceFile, [queryVisitor, templateVisitor]); + queryVisitor.visitNode(sourceFile); + templateVisitor.visitNode(sourceFile); }); const {resolvedQueries, classMetadata} = queryVisitor; diff --git a/packages/core/schematics/migrations/static-queries/index.ts b/packages/core/schematics/migrations/static-queries/index.ts index 6faf0aedef..f592e56e7c 100644 --- a/packages/core/schematics/migrations/static-queries/index.ts +++ b/packages/core/schematics/migrations/static-queries/index.ts @@ -13,21 +13,24 @@ import * as ts from 'typescript'; import {NgComponentTemplateVisitor} from '../../utils/ng_component_template'; import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; -import {getInquirer, supportsPrompt} from '../../utils/schematics_prompt'; import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig'; -import {TypeScriptVisitor, visitAllNodes} from '../../utils/typescript/visit_nodes'; import {NgQueryResolveVisitor} from './angular/ng_query_visitor'; import {QueryTemplateStrategy} from './strategies/template_strategy/template_strategy'; import {QueryTestStrategy} from './strategies/test_strategy/test_strategy'; import {TimingStrategy} from './strategies/timing-strategy'; import {QueryUsageStrategy} from './strategies/usage_strategy/usage_strategy'; +import {SELECTED_STRATEGY, promptForMigrationStrategy} from './strategy_prompt'; import {getTransformedQueryCallExpr} from './transform'; -export enum SELECTED_STRATEGY { - TEMPLATE, - USAGE, - TESTS, +interface AnalyzedProject { + program: ts.Program; + host: ts.CompilerHost; + queryVisitor: NgQueryResolveVisitor; + sourceFiles: ts.SourceFile[]; + basePath: string; + typeChecker: ts.TypeChecker; + tsconfigPath: string; } /** Entry point for the V8 static-query migration. */ @@ -57,51 +60,37 @@ async function runMigration(tree: Tree, context: SchematicContext) { 'to explicit timing.'); } - // In case prompts are supported, determine the desired migration strategy - // by creating a choice prompt. By default the template strategy is used. - let selectedStrategy: SELECTED_STRATEGY = SELECTED_STRATEGY.TEMPLATE; - if (supportsPrompt()) { - logger.info('There are two available migration strategies that can be selected:'); - logger.info(' • Template strategy - migration tool (short-term gains, rare corrections)'); - logger.info(' • Usage strategy - best practices (long-term gains, manual corrections)'); - logger.info('For an easy migration, the template strategy is recommended. The usage'); - logger.info('strategy can be used for best practices and a code base that will be more'); - logger.info('flexible to changes going forward.'); - const {strategyName} = await getInquirer().prompt<{strategyName: string}>({ - type: 'list', - name: 'strategyName', - message: 'What migration strategy do you want to use?', - choices: [ - {name: 'Template strategy', value: 'template'}, {name: 'Usage strategy', value: 'usage'} - ], - default: 'template', - }); - logger.info(''); - selectedStrategy = - strategyName === 'usage' ? SELECTED_STRATEGY.USAGE : SELECTED_STRATEGY.TEMPLATE; - } else { - // In case prompts are not supported, we still want to allow developers to opt - // into the usage strategy by specifying an environment variable. The tests also - // use the environment variable as there is no headless way to select via prompt. - selectedStrategy = !!process.env['NG_STATIC_QUERY_USAGE_STRATEGY'] ? SELECTED_STRATEGY.USAGE : - SELECTED_STRATEGY.TEMPLATE; - } - + const buildProjects = new Set(); const failures = []; for (const tsconfigPath of buildPaths) { - failures.push(...await runStaticQueryMigration(tree, tsconfigPath, basePath, selectedStrategy)); + const project = analyzeProject(tree, tsconfigPath, basePath); + if (project) { + buildProjects.add(project); + } } + + // In case there are projects which contain queries that need to be migrated, + // we want to prompt for the migration strategy and run the migration. + if (buildProjects.size) { + const strategy = await promptForMigrationStrategy(logger); + for (let project of Array.from(buildProjects.values())) { + failures.push(...await runStaticQueryMigration(tree, project, strategy)); + } + } + // For the "test" tsconfig projects we always want to use the test strategy as // we can't detect the proper timing within spec files. for (const tsconfigPath of testPaths) { - failures.push( - ...await runStaticQueryMigration(tree, tsconfigPath, basePath, SELECTED_STRATEGY.TESTS)); + const project = await analyzeProject(tree, tsconfigPath, basePath); + if (project) { + failures.push(...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS)); + } } if (failures.length) { - logger.info('Some queries cannot be migrated automatically. Please go through'); - logger.info('those manually and apply the appropriate timing:'); + logger.info('Some queries could not be migrated automatically. Please go'); + logger.info('through those manually and apply the appropriate timing:'); failures.forEach(failure => logger.warn(`⮑ ${failure}`)); } @@ -109,47 +98,58 @@ async function runMigration(tree: Tree, context: SchematicContext) { } /** - * Runs the static query migration for the given TypeScript project. The schematic - * analyzes all queries within the project and sets up the query timing based on - * the current usage of the query property. e.g. a view query that is not used in any - * lifecycle hook does not need to be static and can be set up with "static: false". + * Analyzes the given TypeScript project by looking for queries that need to be + * migrated. In case there are no queries that can be migrated, null is returned. + */ +function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string): + AnalyzedProject|null { + const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath)); + const host = ts.createCompilerHost(parsed.options, true); + + // We need to overwrite the host "readFile" method, as we want the TypeScript + // program to be based on the file contents in the virtual file tree. Otherwise + // if we run the migration for multiple tsconfig files which have intersecting + // source files, it can end up updating query definitions multiple times. + host.readFile = fileName => { + const buffer = tree.read(relative(basePath, fileName)); + return buffer ? buffer.toString() : undefined; + }; + + const program = ts.createProgram(parsed.fileNames, parsed.options, host); + const typeChecker = program.getTypeChecker(); + const sourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !); + const queryVisitor = new NgQueryResolveVisitor(typeChecker); + + // Analyze all project source-files and collect all queries that + // need to be migrated. + sourceFiles.forEach(sourceFile => queryVisitor.visitNode(sourceFile)); + + if (queryVisitor.resolvedQueries.size === 0) { + return null; + } + + return {program, host, tsconfigPath, typeChecker, basePath, queryVisitor, sourceFiles}; + } + +/** + * Runs the static query migration for the given project. The schematic analyzes all + * queries within the project and sets up the query timing based on the current usage + * of the query property. e.g. a view query that is not used in any lifecycle hook does + * not need to be static and can be set up with "static: false". */ async function runStaticQueryMigration( - tree: Tree, tsconfigPath: string, basePath: string, selectedStrategy: SELECTED_STRATEGY) { - const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath)); - const host = ts.createCompilerHost(parsed.options, true); - - // We need to overwrite the host "readFile" method, as we want the TypeScript - // program to be based on the file contents in the virtual file tree. Otherwise - // if we run the migration for multiple tsconfig files which have intersecting - // source files, it can end up updating query definitions multiple times. - host.readFile = fileName => { - const buffer = tree.read(relative(basePath, fileName)); - return buffer ? buffer.toString() : undefined; - }; - - const program = ts.createProgram(parsed.fileNames, parsed.options, host); - const typeChecker = program.getTypeChecker(); - const queryVisitor = new NgQueryResolveVisitor(typeChecker); - const templateVisitor = new NgComponentTemplateVisitor(typeChecker); - const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !); + tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY) { + const {sourceFiles, typeChecker, host, queryVisitor, tsconfigPath, basePath} = project; const printer = ts.createPrinter(); - const analysisVisitors: TypeScriptVisitor[] = [queryVisitor]; const failureMessages: string[] = []; + const templateVisitor = new NgComponentTemplateVisitor(typeChecker); // If the "usage" strategy is selected, we also need to add the query visitor // to the analysis visitors so that query usage in templates can be also checked. if (selectedStrategy === SELECTED_STRATEGY.USAGE) { - analysisVisitors.push(templateVisitor); + sourceFiles.forEach(s => templateVisitor.visitNode(s)); } - rootSourceFiles.forEach(sourceFile => { - // The visit utility function only traverses a source file once. We don't want to - // traverse through all source files multiple times for each visitor as this could be - // slow. - visitAllNodes(sourceFile, analysisVisitors); - }); - const {resolvedQueries, classMetadata} = queryVisitor; const {resolvedTemplates} = templateVisitor; diff --git a/packages/core/schematics/migrations/static-queries/strategy_prompt.ts b/packages/core/schematics/migrations/static-queries/strategy_prompt.ts new file mode 100644 index 0000000000..e0cbf514a9 --- /dev/null +++ b/packages/core/schematics/migrations/static-queries/strategy_prompt.ts @@ -0,0 +1,50 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * 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 {logging} from '@angular-devkit/core'; + +import {getInquirer, supportsPrompt} from '../../utils/schematics_prompt'; + +export enum SELECTED_STRATEGY { + TEMPLATE, + USAGE, + TESTS, +} + +/** + * Prompts the user for the migration strategy that should be used. Defaults to the + * template strategy as it provides a migration with rare manual corrections. + * */ +export async function promptForMigrationStrategy(logger: logging.LoggerApi) { + if (supportsPrompt()) { + logger.info('There are two available migration strategies that can be selected:'); + logger.info(' • Template strategy - migration tool (short-term gains, rare corrections)'); + logger.info(' • Usage strategy - best practices (long-term gains, manual corrections)'); + logger.info('For an easy migration, the template strategy is recommended. The usage'); + logger.info('strategy can be used for best practices and a code base that will be more'); + logger.info('flexible to changes going forward.'); + const {strategyName} = await getInquirer().prompt<{strategyName: string}>({ + type: 'list', + name: 'strategyName', + message: 'What migration strategy do you want to use?', + choices: [ + {name: 'Template strategy', value: 'template'}, {name: 'Usage strategy', value: 'usage'} + ], + default: 'template', + }); + logger.info(''); + return strategyName === 'usage' ? SELECTED_STRATEGY.USAGE : SELECTED_STRATEGY.TEMPLATE; + } else { + // In case prompts are not supported, we still want to allow developers to opt + // into the usage strategy by specifying an environment variable. The tests also + // use the environment variable as there is no headless way to select via prompt. + return !!process.env['NG_STATIC_QUERY_USAGE_STRATEGY'] ? SELECTED_STRATEGY.USAGE : + SELECTED_STRATEGY.TEMPLATE; + } +} diff --git a/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts b/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts index 0bd7077ab5..5b8f89d2f4 100644 --- a/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts +++ b/packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts @@ -11,7 +11,6 @@ import * as ts from 'typescript'; import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template'; import {createHtmlSourceFile} from '../../../utils/tslint/tslint_html_source_file'; -import {visitAllNodes} from '../../../utils/typescript/visit_nodes'; import {analyzeResolvedTemplate} from '../analyze_template'; const FAILURE_MESSAGE = 'Found assignment to template variable. This does not work with Ivy and ' + @@ -27,7 +26,7 @@ export class Rule extends Rules.TypedRule { const failures: RuleFailure[] = []; // Analyze the current source files by detecting all referenced HTML templates. - visitAllNodes(sourceFile, [templateVisitor]); + templateVisitor.visitNode(sourceFile); const {resolvedTemplates} = templateVisitor; diff --git a/packages/core/schematics/migrations/template-var-assignment/index.ts b/packages/core/schematics/migrations/template-var-assignment/index.ts index 29fc6203ad..ed27c1f93d 100644 --- a/packages/core/schematics/migrations/template-var-assignment/index.ts +++ b/packages/core/schematics/migrations/template-var-assignment/index.ts @@ -14,7 +14,6 @@ import * as ts from 'typescript'; import {NgComponentTemplateVisitor} from '../../utils/ng_component_template'; import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths'; import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig'; -import {visitAllNodes} from '../../utils/typescript/visit_nodes'; import {analyzeResolvedTemplate} from './analyze_template'; @@ -64,7 +63,7 @@ function runTemplateVariableAssignmentCheck( const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !); // Analyze source files by detecting HTML templates. - rootSourceFiles.forEach(sourceFile => visitAllNodes(sourceFile, [templateVisitor])); + rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile)); const {resolvedTemplates} = templateVisitor; const collectedFailures: string[] = []; diff --git a/packages/core/schematics/test/static_queries_migration_usage_spec.ts b/packages/core/schematics/test/static_queries_migration_usage_spec.ts index afa3f2f801..258d7ff9e7 100644 --- a/packages/core/schematics/test/static_queries_migration_usage_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_usage_spec.ts @@ -1367,5 +1367,22 @@ describe('static-queries migration with usage strategy', () => { expect(tree.readContent('/src/index.ts')) .toContain(`@${queryType}('test', { static: false }) query: any;`); }); + + it(`should not prompt for migration strategy if no @${queryType} query is used`, async() => { + writeFile('/index.ts', ` + import {Component, ${queryType}} from '@angular/core'; + + @Component({template: ''}) + export class NoQueriesDeclared { + } + `); + + const testModule = require('../migrations/static-queries/strategy_prompt'); + spyOn(testModule, 'promptForMigrationStrategy').and.callThrough(); + + await runMigration(); + + expect(testModule.promptForMigrationStrategy).toHaveBeenCalledTimes(0); + }); } }); diff --git a/packages/core/schematics/utils/ng_component_template.ts b/packages/core/schematics/utils/ng_component_template.ts index d905d0e8d5..5b20b964cb 100644 --- a/packages/core/schematics/utils/ng_component_template.ts +++ b/packages/core/schematics/utils/ng_component_template.ts @@ -47,6 +47,8 @@ export class NgComponentTemplateVisitor { if (node.kind === ts.SyntaxKind.ClassDeclaration) { this.visitClassDeclaration(node as ts.ClassDeclaration); } + + ts.forEachChild(node, n => this.visitNode(n)); } private visitClassDeclaration(node: ts.ClassDeclaration) { diff --git a/packages/core/schematics/utils/typescript/visit_nodes.ts b/packages/core/schematics/utils/typescript/visit_nodes.ts deleted file mode 100644 index f0553e8f5a..0000000000 --- a/packages/core/schematics/utils/typescript/visit_nodes.ts +++ /dev/null @@ -1,16 +0,0 @@ -/** - * @license - * Copyright Google Inc. All Rights Reserved. - * - * 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 * as ts from 'typescript'; - -export interface TypeScriptVisitor { visitNode(node: ts.Node): void; } - -export function visitAllNodes(node: ts.Node, visitors: TypeScriptVisitor[]) { - visitors.forEach(v => v.visitNode(node)); - ts.forEachChild(node, node => visitAllNodes(node, visitors)); -}