Skip to content
Permalink
Browse files

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
  • Loading branch information...
devversion authored and alxhub committed May 3, 2019
1 parent 644925f commit 12fb639b7dafbb41466586853cd840cfa8812203
@@ -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) {
@@ -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;
@@ -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,99 +60,96 @@ 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<AnalyzedProject>();
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}`));
}

logger.info('------------------------------------------------');
}

/**
* 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.
*/
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;
};
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;
}

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) !);
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, 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;

@@ -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;
}
}
@@ -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;

@@ -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[] = [];
@@ -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: '<span #test></span>'})
export class NoQueriesDeclared {
}
`);

const testModule = require('../migrations/static-queries/strategy_prompt');
spyOn(testModule, 'promptForMigrationStrategy').and.callThrough();

await runMigration();

expect(testModule.promptForMigrationStrategy).toHaveBeenCalledTimes(0);
});
}
});
@@ -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) {

This file was deleted.

0 comments on commit 12fb639

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