Skip to content

Commit 4c12d74

Browse files
devversionalxhub
devversion
authored and
alxhub
committed
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
1 parent 04a9f28 commit 4c12d74

9 files changed

Lines changed: 147 additions & 97 deletions

File tree

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,8 @@ export class NgQueryResolveVisitor {
5454
this.visitClassDeclaration(node as ts.ClassDeclaration);
5555
break;
5656
}
57+
58+
ts.forEachChild(node, n => this.visitNode(n));
5759
}
5860

5961
private visitPropertyDeclaration(node: ts.PropertyDeclaration) {

‎packages/core/schematics/migrations/static-queries/google3/explicitQueryTimingRule.ts‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import {Replacement, RuleFailure, Rules} from 'tslint';
1010
import * as ts from 'typescript';
1111

1212
import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template';
13-
import {visitAllNodes} from '../../../utils/typescript/visit_nodes';
1413
import {NgQueryResolveVisitor} from '../angular/ng_query_visitor';
1514
import {QueryTiming} from '../angular/query-definition';
1615
import {QueryUsageStrategy} from '../strategies/usage_strategy/usage_strategy';
@@ -35,10 +34,8 @@ export class Rule extends Rules.TypedRule {
3534

3635
// Analyze source files by detecting queries, class relations and component templates.
3736
rootSourceFiles.forEach(sourceFile => {
38-
// The visit utility function only traverses the source file once. We don't want to
39-
// traverse through all source files multiple times for each visitor as this could be
40-
// slow.
41-
visitAllNodes(sourceFile, [queryVisitor, templateVisitor]);
37+
queryVisitor.visitNode(sourceFile);
38+
templateVisitor.visitNode(sourceFile);
4239
});
4340

4441
const {resolvedQueries, classMetadata} = queryVisitor;

‎packages/core/schematics/migrations/static-queries/index.ts‎

Lines changed: 72 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -13,21 +13,24 @@ import * as ts from 'typescript';
1313

1414
import {NgComponentTemplateVisitor} from '../../utils/ng_component_template';
1515
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
16-
import {getInquirer, supportsPrompt} from '../../utils/schematics_prompt';
1716
import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig';
18-
import {TypeScriptVisitor, visitAllNodes} from '../../utils/typescript/visit_nodes';
1917

2018
import {NgQueryResolveVisitor} from './angular/ng_query_visitor';
2119
import {QueryTemplateStrategy} from './strategies/template_strategy/template_strategy';
2220
import {QueryTestStrategy} from './strategies/test_strategy/test_strategy';
2321
import {TimingStrategy} from './strategies/timing-strategy';
2422
import {QueryUsageStrategy} from './strategies/usage_strategy/usage_strategy';
23+
import {SELECTED_STRATEGY, promptForMigrationStrategy} from './strategy_prompt';
2524
import {getTransformedQueryCallExpr} from './transform';
2625

27-
export enum SELECTED_STRATEGY {
28-
TEMPLATE,
29-
USAGE,
30-
TESTS,
26+
interface AnalyzedProject {
27+
program: ts.Program;
28+
host: ts.CompilerHost;
29+
queryVisitor: NgQueryResolveVisitor;
30+
sourceFiles: ts.SourceFile[];
31+
basePath: string;
32+
typeChecker: ts.TypeChecker;
33+
tsconfigPath: string;
3134
}
3235

3336
/** Entry point for the V8 static-query migration. */
@@ -57,99 +60,96 @@ async function runMigration(tree: Tree, context: SchematicContext) {
5760
'to explicit timing.');
5861
}
5962

60-
// In case prompts are supported, determine the desired migration strategy
61-
// by creating a choice prompt. By default the template strategy is used.
62-
let selectedStrategy: SELECTED_STRATEGY = SELECTED_STRATEGY.TEMPLATE;
63-
if (supportsPrompt()) {
64-
logger.info('There are two available migration strategies that can be selected:');
65-
logger.info(' • Template strategy - migration tool (short-term gains, rare corrections)');
66-
logger.info(' • Usage strategy - best practices (long-term gains, manual corrections)');
67-
logger.info('For an easy migration, the template strategy is recommended. The usage');
68-
logger.info('strategy can be used for best practices and a code base that will be more');
69-
logger.info('flexible to changes going forward.');
70-
const {strategyName} = await getInquirer().prompt<{strategyName: string}>({
71-
type: 'list',
72-
name: 'strategyName',
73-
message: 'What migration strategy do you want to use?',
74-
choices: [
75-
{name: 'Template strategy', value: 'template'}, {name: 'Usage strategy', value: 'usage'}
76-
],
77-
default: 'template',
78-
});
79-
logger.info('');
80-
selectedStrategy =
81-
strategyName === 'usage' ? SELECTED_STRATEGY.USAGE : SELECTED_STRATEGY.TEMPLATE;
82-
} else {
83-
// In case prompts are not supported, we still want to allow developers to opt
84-
// into the usage strategy by specifying an environment variable. The tests also
85-
// use the environment variable as there is no headless way to select via prompt.
86-
selectedStrategy = !!process.env['NG_STATIC_QUERY_USAGE_STRATEGY'] ? SELECTED_STRATEGY.USAGE :
87-
SELECTED_STRATEGY.TEMPLATE;
88-
}
89-
63+
const buildProjects = new Set<AnalyzedProject>();
9064
const failures = [];
9165

9266
for (const tsconfigPath of buildPaths) {
93-
failures.push(...await runStaticQueryMigration(tree, tsconfigPath, basePath, selectedStrategy));
67+
const project = analyzeProject(tree, tsconfigPath, basePath);
68+
if (project) {
69+
buildProjects.add(project);
70+
}
9471
}
72+
73+
// In case there are projects which contain queries that need to be migrated,
74+
// we want to prompt for the migration strategy and run the migration.
75+
if (buildProjects.size) {
76+
const strategy = await promptForMigrationStrategy(logger);
77+
for (let project of Array.from(buildProjects.values())) {
78+
failures.push(...await runStaticQueryMigration(tree, project, strategy));
79+
}
80+
}
81+
9582
// For the "test" tsconfig projects we always want to use the test strategy as
9683
// we can't detect the proper timing within spec files.
9784
for (const tsconfigPath of testPaths) {
98-
failures.push(
99-
...await runStaticQueryMigration(tree, tsconfigPath, basePath, SELECTED_STRATEGY.TESTS));
85+
const project = await analyzeProject(tree, tsconfigPath, basePath);
86+
if (project) {
87+
failures.push(...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS));
88+
}
10089
}
10190

10291
if (failures.length) {
103-
logger.info('Some queries cannot be migrated automatically. Please go through');
104-
logger.info('those manually and apply the appropriate timing:');
92+
logger.info('Some queries could not be migrated automatically. Please go');
93+
logger.info('through those manually and apply the appropriate timing:');
10594
failures.forEach(failure => logger.warn(`⮑ ${failure}`));
10695
}
10796

10897
logger.info('------------------------------------------------');
10998
}
11099

111100
/**
112-
* Runs the static query migration for the given TypeScript project. The schematic
113-
* analyzes all queries within the project and sets up the query timing based on
114-
* the current usage of the query property. e.g. a view query that is not used in any
115-
* lifecycle hook does not need to be static and can be set up with "static: false".
101+
* Analyzes the given TypeScript project by looking for queries that need to be
102+
* migrated. In case there are no queries that can be migrated, null is returned.
116103
*/
117-
async function runStaticQueryMigration(
118-
tree: Tree, tsconfigPath: string, basePath: string, selectedStrategy: SELECTED_STRATEGY) {
119-
const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath));
120-
const host = ts.createCompilerHost(parsed.options, true);
121-
122-
// We need to overwrite the host "readFile" method, as we want the TypeScript
123-
// program to be based on the file contents in the virtual file tree. Otherwise
124-
// if we run the migration for multiple tsconfig files which have intersecting
125-
// source files, it can end up updating query definitions multiple times.
126-
host.readFile = fileName => {
127-
const buffer = tree.read(relative(basePath, fileName));
128-
return buffer ? buffer.toString() : undefined;
129-
};
104+
function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string):
105+
AnalyzedProject|null {
106+
const parsed = parseTsconfigFile(tsconfigPath, dirname(tsconfigPath));
107+
const host = ts.createCompilerHost(parsed.options, true);
108+
109+
// We need to overwrite the host "readFile" method, as we want the TypeScript
110+
// program to be based on the file contents in the virtual file tree. Otherwise
111+
// if we run the migration for multiple tsconfig files which have intersecting
112+
// source files, it can end up updating query definitions multiple times.
113+
host.readFile = fileName => {
114+
const buffer = tree.read(relative(basePath, fileName));
115+
return buffer ? buffer.toString() : undefined;
116+
};
117+
118+
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
119+
const typeChecker = program.getTypeChecker();
120+
const sourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
121+
const queryVisitor = new NgQueryResolveVisitor(typeChecker);
122+
123+
// Analyze all project source-files and collect all queries that
124+
// need to be migrated.
125+
sourceFiles.forEach(sourceFile => queryVisitor.visitNode(sourceFile));
126+
127+
if (queryVisitor.resolvedQueries.size === 0) {
128+
return null;
129+
}
130130

131-
const program = ts.createProgram(parsed.fileNames, parsed.options, host);
132-
const typeChecker = program.getTypeChecker();
133-
const queryVisitor = new NgQueryResolveVisitor(typeChecker);
134-
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
135-
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
131+
return {program, host, tsconfigPath, typeChecker, basePath, queryVisitor, sourceFiles};
132+
}
133+
134+
/**
135+
* Runs the static query migration for the given project. The schematic analyzes all
136+
* queries within the project and sets up the query timing based on the current usage
137+
* of the query property. e.g. a view query that is not used in any lifecycle hook does
138+
* not need to be static and can be set up with "static: false".
139+
*/
140+
async function runStaticQueryMigration(
141+
tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY) {
142+
const {sourceFiles, typeChecker, host, queryVisitor, tsconfigPath, basePath} = project;
136143
const printer = ts.createPrinter();
137-
const analysisVisitors: TypeScriptVisitor[] = [queryVisitor];
138144
const failureMessages: string[] = [];
145+
const templateVisitor = new NgComponentTemplateVisitor(typeChecker);
139146

140147
// If the "usage" strategy is selected, we also need to add the query visitor
141148
// to the analysis visitors so that query usage in templates can be also checked.
142149
if (selectedStrategy === SELECTED_STRATEGY.USAGE) {
143-
analysisVisitors.push(templateVisitor);
150+
sourceFiles.forEach(s => templateVisitor.visitNode(s));
144151
}
145152

146-
rootSourceFiles.forEach(sourceFile => {
147-
// The visit utility function only traverses a source file once. We don't want to
148-
// traverse through all source files multiple times for each visitor as this could be
149-
// slow.
150-
visitAllNodes(sourceFile, analysisVisitors);
151-
});
152-
153153
const {resolvedQueries, classMetadata} = queryVisitor;
154154
const {resolvedTemplates} = templateVisitor;
155155

‎packages/core/schematics/migrations/static-queries/strategy_prompt.ts‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* @license
3+
* Copyright Google Inc. All Rights Reserved.
4+
*
5+
* Use of this source code is governed by an MIT-style license that can be
6+
* found in the LICENSE file at https://angular.io/license
7+
*/
8+
9+
10+
import {logging} from '@angular-devkit/core';
11+
12+
import {getInquirer, supportsPrompt} from '../../utils/schematics_prompt';
13+
14+
export enum SELECTED_STRATEGY {
15+
TEMPLATE,
16+
USAGE,
17+
TESTS,
18+
}
19+
20+
/**
21+
* Prompts the user for the migration strategy that should be used. Defaults to the
22+
* template strategy as it provides a migration with rare manual corrections.
23+
* */
24+
export async function promptForMigrationStrategy(logger: logging.LoggerApi) {
25+
if (supportsPrompt()) {
26+
logger.info('There are two available migration strategies that can be selected:');
27+
logger.info(' • Template strategy - migration tool (short-term gains, rare corrections)');
28+
logger.info(' • Usage strategy - best practices (long-term gains, manual corrections)');
29+
logger.info('For an easy migration, the template strategy is recommended. The usage');
30+
logger.info('strategy can be used for best practices and a code base that will be more');
31+
logger.info('flexible to changes going forward.');
32+
const {strategyName} = await getInquirer().prompt<{strategyName: string}>({
33+
type: 'list',
34+
name: 'strategyName',
35+
message: 'What migration strategy do you want to use?',
36+
choices: [
37+
{name: 'Template strategy', value: 'template'}, {name: 'Usage strategy', value: 'usage'}
38+
],
39+
default: 'template',
40+
});
41+
logger.info('');
42+
return strategyName === 'usage' ? SELECTED_STRATEGY.USAGE : SELECTED_STRATEGY.TEMPLATE;
43+
} else {
44+
// In case prompts are not supported, we still want to allow developers to opt
45+
// into the usage strategy by specifying an environment variable. The tests also
46+
// use the environment variable as there is no headless way to select via prompt.
47+
return !!process.env['NG_STATIC_QUERY_USAGE_STRATEGY'] ? SELECTED_STRATEGY.USAGE :
48+
SELECTED_STRATEGY.TEMPLATE;
49+
}
50+
}

‎packages/core/schematics/migrations/template-var-assignment/google3/noTemplateVariableAssignmentRule.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import * as ts from 'typescript';
1111

1212
import {NgComponentTemplateVisitor} from '../../../utils/ng_component_template';
1313
import {createHtmlSourceFile} from '../../../utils/tslint/tslint_html_source_file';
14-
import {visitAllNodes} from '../../../utils/typescript/visit_nodes';
1514
import {analyzeResolvedTemplate} from '../analyze_template';
1615

1716
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 {
2726
const failures: RuleFailure[] = [];
2827

2928
// Analyze the current source files by detecting all referenced HTML templates.
30-
visitAllNodes(sourceFile, [templateVisitor]);
29+
templateVisitor.visitNode(sourceFile);
3130

3231
const {resolvedTemplates} = templateVisitor;
3332

‎packages/core/schematics/migrations/template-var-assignment/index.ts‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import * as ts from 'typescript';
1414
import {NgComponentTemplateVisitor} from '../../utils/ng_component_template';
1515
import {getProjectTsConfigPaths} from '../../utils/project_tsconfig_paths';
1616
import {parseTsconfigFile} from '../../utils/typescript/parse_tsconfig';
17-
import {visitAllNodes} from '../../utils/typescript/visit_nodes';
1817

1918
import {analyzeResolvedTemplate} from './analyze_template';
2019

@@ -64,7 +63,7 @@ function runTemplateVariableAssignmentCheck(
6463
const rootSourceFiles = program.getRootFileNames().map(f => program.getSourceFile(f) !);
6564

6665
// Analyze source files by detecting HTML templates.
67-
rootSourceFiles.forEach(sourceFile => visitAllNodes(sourceFile, [templateVisitor]));
66+
rootSourceFiles.forEach(sourceFile => templateVisitor.visitNode(sourceFile));
6867

6968
const {resolvedTemplates} = templateVisitor;
7069
const collectedFailures: string[] = [];

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,5 +1367,22 @@ describe('static-queries migration with usage strategy', () => {
13671367
expect(tree.readContent('/src/index.ts'))
13681368
.toContain(`@${queryType}('test', { static: false }) query: any;`);
13691369
});
1370+
1371+
it(`should not prompt for migration strategy if no @${queryType} query is used`, async() => {
1372+
writeFile('/index.ts', `
1373+
import {Component, ${queryType}} from '@angular/core';
1374+
1375+
@Component({template: '<span #test></span>'})
1376+
export class NoQueriesDeclared {
1377+
}
1378+
`);
1379+
1380+
const testModule = require('../migrations/static-queries/strategy_prompt');
1381+
spyOn(testModule, 'promptForMigrationStrategy').and.callThrough();
1382+
1383+
await runMigration();
1384+
1385+
expect(testModule.promptForMigrationStrategy).toHaveBeenCalledTimes(0);
1386+
});
13701387
}
13711388
});

‎packages/core/schematics/utils/ng_component_template.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ export class NgComponentTemplateVisitor {
4747
if (node.kind === ts.SyntaxKind.ClassDeclaration) {
4848
this.visitClassDeclaration(node as ts.ClassDeclaration);
4949
}
50+
51+
ts.forEachChild(node, n => this.visitNode(n));
5052
}
5153

5254
private visitClassDeclaration(node: ts.ClassDeclaration) {

‎packages/core/schematics/utils/typescript/visit_nodes.ts‎

Lines changed: 0 additions & 16 deletions
This file was deleted.

0 commit comments

Comments
 (0)