From af2def721f29010a9d8926b106b8ee94a632aee8 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Sun, 5 May 2019 12:19:40 +0200 Subject: [PATCH] refactor(core): static-query migration should gracefully exit if AOT compiler throws 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. --- .../migrations/static-queries/index.ts | 32 ++++++++--- .../template_strategy/template_strategy.ts | 16 +++--- .../strategies/test_strategy/test_strategy.ts | 2 +- .../strategies/timing-strategy.ts | 4 +- .../usage_strategy/usage_strategy.ts | 6 +- .../static_queries_migration_template_spec.ts | 56 ++++++++++++++++--- 6 files changed, 85 insertions(+), 31 deletions(-) diff --git a/packages/core/schematics/migrations/static-queries/index.ts b/packages/core/schematics/migrations/static-queries/index.ts index 6ea16c484e1bb1..089d369d7c06d0 100644 --- a/packages/core/schematics/migrations/static-queries/index.ts +++ b/packages/core/schematics/migrations/static-queries/index.ts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ +import {logging} from '@angular-devkit/core'; import {Rule, SchematicContext, SchematicsException, Tree} from '@angular-devkit/schematics'; import {dirname, relative} from 'path'; import {from} from 'rxjs'; @@ -75,7 +76,7 @@ async function runMigration(tree: Tree, context: SchematicContext) { if (buildProjects.size) { const strategy = await promptForMigrationStrategy(logger); for (let project of Array.from(buildProjects.values())) { - failures.push(...await runStaticQueryMigration(tree, project, strategy)); + failures.push(...await runStaticQueryMigration(tree, project, strategy, logger)); } } @@ -84,7 +85,8 @@ async function runMigration(tree: Tree, context: SchematicContext) { for (const tsconfigPath of testPaths) { const project = await analyzeProject(tree, tsconfigPath, basePath); if (project) { - failures.push(...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS)); + failures.push( + ...await runStaticQueryMigration(tree, project, SELECTED_STRATEGY.TESTS, logger)); } } @@ -139,7 +141,8 @@ function analyzeProject(tree: Tree, tsconfigPath: string, basePath: string): * not need to be static and can be set up with "static: false". */ async function runStaticQueryMigration( - tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY) { + tree: Tree, project: AnalyzedProject, selectedStrategy: SELECTED_STRATEGY, + logger: logging.LoggerApi) { const {sourceFiles, typeChecker, host, queryVisitor, tsconfigPath, basePath} = project; const printer = ts.createPrinter(); const failureMessages: string[] = []; @@ -173,10 +176,25 @@ async function runStaticQueryMigration( strategy = new QueryTemplateStrategy(tsconfigPath, classMetadata, host); } - // In case the strategy could not be set up properly, we just exit the - // migration. We don't want to throw an exception as this could mean - // that other migrations are interrupted. - if (!strategy.setup()) { + try { + strategy.setup(); + } catch (e) { + // In case the strategy could not be set up properly, we just exit the + // migration. We don't want to throw an exception as this could mean + // that other migrations are interrupted. + logger.warn( + `Could not setup migration strategy for "${project.tsconfigPath}". The ` + + `following error has been reported:`); + if (selectedStrategy === SELECTED_STRATEGY.TEMPLATE) { + logger.warn( + `The template migration strategy uses the Angular compiler ` + + `internally and therefore projects that no longer build successfully after ` + + `the update cannot use the template migration strategy. Please ensure ` + + `there are no AOT compilation errors.`); + } + logger.error(e); + logger.info( + 'Migration can be rerun with: "ng update @angular/core --from 7 --to 8 --migrate-only"'); return []; } diff --git a/packages/core/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts b/packages/core/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts index c19c9a6c0834db..dc2c489f2cf3dd 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/template_strategy/template_strategy.ts @@ -63,14 +63,12 @@ export class QueryTemplateStrategy implements TimingStrategy { ]; if (ngDiagnostics.length) { - this._printDiagnosticFailures(ngDiagnostics); - return false; + throw this._createDiagnosticsError(ngDiagnostics); } analyzedModules.files.forEach(file => { file.directives.forEach(directive => this._analyzeDirective(directive, analyzedModules)); }); - return true; } /** Analyzes a given directive by determining the timing of all matched view queries. */ @@ -180,12 +178,12 @@ export class QueryTemplateStrategy implements TimingStrategy { .template; } - private _printDiagnosticFailures(diagnostics: (ts.Diagnostic|Diagnostic)[]) { - console.error('Could not create Angular AOT compiler to determine query timing.'); - console.error('The following diagnostics were detected:\n'); - console.error(diagnostics.map(d => d.messageText).join(`\n`)); - console.error('Please make sure that there is no compilation failure. The migration'); - console.error('can be rerun with: "ng update @angular/core --from 7 --to 8 --migrate-only"'); + private _createDiagnosticsError(diagnostics: (ts.Diagnostic|Diagnostic)[]) { + return new Error( + `Could not create Angular AOT compiler to determine query timing.\n` + + `The following diagnostics were detected:\n` + + `${diagnostics.map(d => d.messageText).join(`\n `)}\n` + + `Please make sure that there is no AOT compilation failure.`); } private _getViewQueryUniqueKey(filePath: string, className: string, propName: string) { diff --git a/packages/core/schematics/migrations/static-queries/strategies/test_strategy/test_strategy.ts b/packages/core/schematics/migrations/static-queries/strategies/test_strategy/test_strategy.ts index 30c783c0aaae51..7488db2e6236c0 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/test_strategy/test_strategy.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/test_strategy/test_strategy.ts @@ -16,7 +16,7 @@ import {TimingResult, TimingStrategy} from '../timing-strategy'; * of detecting the timing of queries based on how they are used in tests. */ export class QueryTestStrategy implements TimingStrategy { - setup() { return true; } + setup() {} /** * Detects the timing for a given query. For queries within tests, we always diff --git a/packages/core/schematics/migrations/static-queries/strategies/timing-strategy.ts b/packages/core/schematics/migrations/static-queries/strategies/timing-strategy.ts index 404c0aa167b45c..b530f60055d1b0 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/timing-strategy.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/timing-strategy.ts @@ -9,8 +9,8 @@ import {NgQueryDefinition, QueryTiming} from '../angular/query-definition'; export interface TimingStrategy { - /** Sets up the given strategy. Should return false if the strategy could not be set up. */ - setup(): boolean; + /** Sets up the given strategy. Throws if the strategy could not be set up. */ + setup(): void; /** Detects the timing result for a given query. */ detectTiming(query: NgQueryDefinition): TimingResult; } diff --git a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts index 75d31337c99575..735ec58faa7ef8 100644 --- a/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts +++ b/packages/core/schematics/migrations/static-queries/strategies/usage_strategy/usage_strategy.ts @@ -37,11 +37,7 @@ const STATIC_QUERY_LIFECYCLE_HOOKS = { export class QueryUsageStrategy implements TimingStrategy { constructor(private classMetadata: ClassMetadataMap, private typeChecker: ts.TypeChecker) {} - setup() { - // No setup is needed for this strategy and therefore we always return "true" as - // the setup is successful. - return true; - } + setup() {} /** * Analyzes the usage of the given query and determines the query timing based diff --git a/packages/core/schematics/test/static_queries_migration_template_spec.ts b/packages/core/schematics/test/static_queries_migration_template_spec.ts index e9ae5ad0a3fe0c..5357b183dc814e 100644 --- a/packages/core/schematics/test/static_queries_migration_template_spec.ts +++ b/packages/core/schematics/test/static_queries_migration_template_spec.ts @@ -19,6 +19,7 @@ describe('static-queries migration with template strategy', () => { let tmpDirPath: string; let previousWorkingDir: string; let warnOutput: string[]; + let errorOutput: string[]; beforeEach(() => { runner = new SchematicTestRunner('test', require.resolve('../test-migrations.json')); @@ -36,9 +37,12 @@ describe('static-queries migration with template strategy', () => { })); warnOutput = []; + errorOutput = []; runner.logger.subscribe(logEntry => { if (logEntry.level === 'warn') { warnOutput.push(logEntry.message); + } else if (logEntry.level === 'error') { + errorOutput.push(logEntry.message); } }); @@ -457,17 +461,55 @@ describe('static-queries migration with template strategy', () => { // **NOTE**: Analysis will fail as there is no "NgModule" that declares the component. `); - spyOn(console, 'error'); - // We don't expect an error to be thrown as this could interrupt other // migrations which are scheduled with "ng update" in the CLI. await runMigration(); - expect(console.error) - .toHaveBeenCalledWith('Could not create Angular AOT compiler to determine query timing.'); - expect(console.error) - .toHaveBeenCalledWith( - jasmine.stringMatching(/Cannot determine the module for class MyComp/)); + expect(errorOutput.length).toBe(1); + expect(errorOutput[0]) + .toMatch(/^Error: Could not create Angular AOT compiler to determine query timing./); + expect(errorOutput[0]).toMatch(/Cannot determine the module for class MyComp/); + }); + + it('should gracefully exit migration if AOT compiler throws exception', async() => { + writeFile('/my-component.ts', ` + import {Component, ViewChild} from '@angular/core'; + + @Component({template: ''}) + export class MyComp { + @ViewChild('test') query: any; + } + `); + writeFile('/app-module.ts', ` + import {NgModule} from '@angular/core'; + import {MyComp} from './components'; + + @NgModule({declarations: [MyComp]}) + export class MyModule {} + `); + + writeFile('/components.ts', `export * from './my-component';`); + writeFile('/index.ts', `export * from './app-module';`); + + // Enable flat-module bundling in order to simulate a common AOT compiler + // failure that can happen in CLI projects that use flat-module bundling + // e.g. with ng-packager. https://github.com/angular/angular/issues/20931 + writeFile('/tsconfig.json', JSON.stringify({ + compilerOptions: { + experimentalDecorators: true, + lib: ['es2015'], + }, + angularCompilerOptions: { + flatModuleId: 'flat-module', + flatModuleOutFile: 'flat-module-bundle.js', + }, + files: ['index.ts'] + })); + + await runMigration(); + + expect(errorOutput.length).toBe(1); + expect(errorOutput[0]).toMatch(/^TypeError: Cannot read property 'module' of undefined/); }); it('should add a todo for content queries which are not detectable', async() => {