Skip to content

Commit 509352f

Browse files
devversionalxhub
devversion
authored and
alxhub
committed
fix(core): static-query migration should gracefully exit if AOT compiler throws (#30269)
The static-query template strategy leverages the AOT compiler in order to determine the query timing. Unfortunately the AOT compiler has open bugs that can cause unexpected failures which make the template strategy unusable in rare cases. These rare exceptions need to be handled gracefully in order to avoid confusion and to provide a more smooth migration. Additionally migration strategy setup failures are now reported with stack traces as the `ng update` command does not print stack traces. This makes it easier to reproduce and report migration issues. PR Close #30269
1 parent 349935a commit 509352f

6 files changed

Lines changed: 85 additions & 31 deletions

File tree

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9+
import {logging} from '@angular-devkit/core';
910
import {Rule, SchematicContext, SchematicsException, Tree} from '@angular-devkit/schematics';
1011
import {dirname, relative} from 'path';
1112
import {from} from 'rxjs';
@@ -75,7 +76,7 @@ async function runMigration(tree: Tree, context: SchematicContext) {
7576
if (buildProjects.size) {
7677
const strategy = await promptForMigrationStrategy(logger);
7778
for (let project of Array.from(buildProjects.values())) {
78-
failures.push(...await runStaticQueryMigration(tree, project, strategy));
79+
failures.push(...await runStaticQueryMigration(tree, project, strategy, logger));
7980
}
8081
}
8182

@@ -84,7 +85,8 @@ async function runMigration(tree: Tree, context: SchematicContext) {
8485
for (const tsconfigPath of testPaths) {
8586
const project = await analyzeProject(tree, tsconfigPath, basePath);
8687
if (project) {
87-
failures.push(...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS));
88+
failures.push(
89+
...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS, logger));
8890
}
8991
}
9092

@@ -139,7 +141,8 @@ function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string):
139141
* not need to be static and can be set up with "static: false".
140142
*/
141143
async function runStaticQueryMigration(
142-
tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY) {
144+
tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY,
145+
logger: logging.LoggerApi) {
143146
const {sourceFiles, typeChecker, host, queryVisitor, tsconfigPath, basePath} = project;
144147
const printer = ts.createPrinter();
145148
const failureMessages: string[] = [];
@@ -173,10 +176,25 @@ async function runStaticQueryMigration(
173176
strategy = new QueryTemplateStrategy(tsconfigPath, classMetadata, host);
174177
}
175178

176-
// In case the strategy could not be set up properly, we just exit the
177-
// migration. We don't want to throw an exception as this could mean
178-
// that other migrations are interrupted.
179-
if (!strategy.setup()) {
179+
try {
180+
strategy.setup();
181+
} catch (e) {
182+
// In case the strategy could not be set up properly, we just exit the
183+
// migration. We don't want to throw an exception as this could mean
184+
// that other migrations are interrupted.
185+
logger.warn(
186+
`Could not setup migration strategy for "${project.tsconfigPath}". The ` +
187+
`following error has been reported:`);
188+
if (selectedStrategy === SELECTED_STRATEGY.TEMPLATE) {
189+
logger.warn(
190+
`The template migration strategy uses the Angular compiler ` +
191+
`internally and therefore projects that no longer build successfully after ` +
192+
`the update cannot use the template migration strategy. Please ensure ` +
193+
`there are no AOT compilation errors.`);
194+
}
195+
logger.error(e);
196+
logger.info(
197+
'Migration can be rerun with: "ng update @angular/core --from 7 --to 8 --migrate-only"');
180198
return [];
181199
}
182200

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -63,14 +63,12 @@ export class QueryTemplateStrategy implements TimingStrategy {
6363
];
6464

6565
if (ngDiagnostics.length) {
66-
this._printDiagnosticFailures(ngDiagnostics);
67-
return false;
66+
throw this._createDiagnosticsError(ngDiagnostics);
6867
}
6968

7069
analyzedModules.files.forEach(file => {
7170
file.directives.forEach(directive => this._analyzeDirective(directive, analyzedModules));
7271
});
73-
return true;
7472
}
7573

7674
/** Analyzes a given directive by determining the timing of all matched view queries. */
@@ -180,12 +178,12 @@ export class QueryTemplateStrategy implements TimingStrategy {
180178
.template;
181179
}
182180

183-
private _printDiagnosticFailures(diagnostics: (ts.Diagnostic|Diagnostic)[]) {
184-
console.error('Could not create Angular AOT compiler to determine query timing.');
185-
console.error('The following diagnostics were detected:\n');
186-
console.error(diagnostics.map(d => d.messageText).join(`\n`));
187-
console.error('Please make sure that there is no compilation failure. The migration');
188-
console.error('can be rerun with: "ng update @angular/core --from 7 --to 8 --migrate-only"');
181+
private _createDiagnosticsError(diagnostics: (ts.Diagnostic|Diagnostic)[]) {
182+
return new Error(
183+
`Could not create Angular AOT compiler to determine query timing.\n` +
184+
`The following diagnostics were detected:\n` +
185+
`${diagnostics.map(d => d.messageText).join(`\n `)}\n` +
186+
`Please make sure that there is no AOT compilation failure.`);
189187
}
190188

191189
private _getViewQueryUniqueKey(filePath: string, className: string, propName: string) {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {TimingResult, TimingStrategy} from '../timing-strategy';
1616
* of detecting the timing of queries based on how they are used in tests.
1717
*/
1818
export class QueryTestStrategy implements TimingStrategy {
19-
setup() { return true; }
19+
setup() {}
2020

2121
/**
2222
* Detects the timing for a given query. For queries within tests, we always

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
import {NgQueryDefinition, QueryTiming} from '../angular/query-definition';
1010

1111
export interface TimingStrategy {
12-
/** Sets up the given strategy. Should return false if the strategy could not be set up. */
13-
setup(): boolean;
12+
/** Sets up the given strategy. Throws if the strategy could not be set up. */
13+
setup(): void;
1414
/** Detects the timing result for a given query. */
1515
detectTiming(query: NgQueryDefinition): TimingResult;
1616
}

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

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,7 @@ const STATIC_QUERY_LIFECYCLE_HOOKS = {
3737
export class QueryUsageStrategy implements TimingStrategy {
3838
constructor(private classMetadata: ClassMetadataMap, private typeChecker: ts.TypeChecker) {}
3939

40-
setup() {
41-
// No setup is needed for this strategy and therefore we always return "true" as
42-
// the setup is successful.
43-
return true;
44-
}
40+
setup() {}
4541

4642
/**
4743
* Analyzes the usage of the given query and determines the query timing based

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

Lines changed: 49 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ describe('static-queries migration with template strategy', () => {
1919
let tmpDirPath: string;
2020
let previousWorkingDir: string;
2121
let warnOutput: string[];
22+
let errorOutput: string[];
2223

2324
beforeEach(() => {
2425
runner = new SchematicTestRunner('test', require.resolve('../migrations.json'));
@@ -36,9 +37,12 @@ describe('static-queries migration with template strategy', () => {
3637
}));
3738

3839
warnOutput = [];
40+
errorOutput = [];
3941
runner.logger.subscribe(logEntry => {
4042
if (logEntry.level === 'warn') {
4143
warnOutput.push(logEntry.message);
44+
} else if (logEntry.level === 'error') {
45+
errorOutput.push(logEntry.message);
4246
}
4347
});
4448

@@ -457,17 +461,55 @@ describe('static-queries migration with template strategy', () => {
457461
// **NOTE**: Analysis will fail as there is no "NgModule" that declares the component.
458462
`);
459463

460-
spyOn(console, 'error');
461-
462464
// We don't expect an error to be thrown as this could interrupt other
463465
// migrations which are scheduled with "ng update" in the CLI.
464466
await runMigration();
465467

466-
expect(console.error)
467-
.toHaveBeenCalledWith('Could not create Angular AOT compiler to determine query timing.');
468-
expect(console.error)
469-
.toHaveBeenCalledWith(
470-
jasmine.stringMatching(/Cannot determine the module for class MyComp/));
468+
expect(errorOutput.length).toBe(1);
469+
expect(errorOutput[0])
470+
.toMatch(/^Error: Could not create Angular AOT compiler to determine query timing./);
471+
expect(errorOutput[0]).toMatch(/Cannot determine the module for class MyComp/);
472+
});
473+
474+
it('should gracefully exit migration if AOT compiler throws exception', async() => {
475+
writeFile('/my-component.ts', `
476+
import {Component, ViewChild} from '@angular/core';
477+
478+
@Component({template: '<span #test></span>'})
479+
export class MyComp {
480+
@ViewChild('test') query: any;
481+
}
482+
`);
483+
writeFile('/app-module.ts', `
484+
import {NgModule} from '@angular/core';
485+
import {MyComp} from './components';
486+
487+
@NgModule({declarations: [MyComp]})
488+
export class MyModule {}
489+
`);
490+
491+
writeFile('/components.ts', `export * from './my-component';`);
492+
writeFile('/index.ts', `export * from './app-module';`);
493+
494+
// Enable flat-module bundling in order to simulate a common AOT compiler
495+
// failure that can happen in CLI projects that use flat-module bundling
496+
// e.g. with ng-packagr. https://github.com/angular/angular/issues/20931
497+
writeFile('/tsconfig.json', JSON.stringify({
498+
compilerOptions: {
499+
experimentalDecorators: true,
500+
lib: ['es2015'],
501+
},
502+
angularCompilerOptions: {
503+
flatModuleId: 'flat-module',
504+
flatModuleOutFile: 'flat-module-bundle.js',
505+
},
506+
files: ['index.ts']
507+
}));
508+
509+
await runMigration();
510+
511+
expect(errorOutput.length).toBe(1);
512+
expect(errorOutput[0]).toMatch(/^TypeError: Cannot read property 'module' of undefined/);
471513
});
472514

473515
it('should add a todo for content queries which are not detectable', async() => {

0 commit comments

Comments
 (0)