From f197191a5f4d0d46e4c3a647937c1179ed2a4923 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 22 Oct 2019 11:24:51 +0200 Subject: [PATCH] refactor(core): better error message for undecorated-classes-with-di migration (#33315) Currently if one of the project targets could not be analyzed due to AOT compiler program failures, we gracefully proceed with the migration. This is expected, but we should not print a message at the end of the migration that the migration was _successful_. The migration was only done partially, hence it's potentially incomplete and we should make it clear that once the failures are resolved, the migration should be re-run. PR Close #33315 --- .../undecorated-classes-with-di/index.ts | 27 ++++++++++++++----- ...ecorated_classes_with_di_migration_spec.ts | 16 +++++++++++ 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/packages/core/schematics/migrations/undecorated-classes-with-di/index.ts b/packages/core/schematics/migrations/undecorated-classes-with-di/index.ts index 65ff964671..23964a7d6c 100644 --- a/packages/core/schematics/migrations/undecorated-classes-with-di/index.ts +++ b/packages/core/schematics/migrations/undecorated-classes-with-di/index.ts @@ -35,6 +35,7 @@ export default function(): Rule { const {buildPaths} = getProjectTsConfigPaths(tree); const basePath = process.cwd(); const failures: string[] = []; + let programError = false; if (!buildPaths.length) { throw new SchematicsException( @@ -43,10 +44,23 @@ export default function(): Rule { } for (const tsconfigPath of buildPaths) { - failures.push(...runUndecoratedClassesMigration(tree, tsconfigPath, basePath, ctx.logger)); + const result = runUndecoratedClassesMigration(tree, tsconfigPath, basePath, ctx.logger); + failures.push(...result.failures); + programError = programError || !!result.programError; } - if (failures.length) { + if (programError) { + ctx.logger.info('Could not migrate all undecorated classes that use dependency'); + ctx.logger.info('injection. Some project targets could not be analyzed due to'); + ctx.logger.info('TypeScript program failures.\n'); + ctx.logger.info(`${MIGRATION_RERUN_MESSAGE}\n`); + + if (failures.length) { + ctx.logger.info('Please manually fix the following failures and re-run the'); + ctx.logger.info('migration once the TypeScript program failures are resolved.'); + failures.forEach(message => ctx.logger.warn(`⮑ ${message}`)); + } + } else if (failures.length) { ctx.logger.info('Could not migrate all undecorated classes that use dependency'); ctx.logger.info('injection. Please manually fix the following failures:'); failures.forEach(message => ctx.logger.warn(`⮑ ${message}`)); @@ -55,13 +69,14 @@ export default function(): Rule { } function runUndecoratedClassesMigration( - tree: Tree, tsconfigPath: string, basePath: string, logger: logging.LoggerApi): string[] { + tree: Tree, tsconfigPath: string, basePath: string, + logger: logging.LoggerApi): {failures: string[], programError?: boolean} { const failures: string[] = []; const programData = gracefullyCreateProgram(tree, basePath, tsconfigPath, logger); // Gracefully exit if the program could not be created. if (programData === null) { - return []; + return {failures: [], programError: true}; } const {program, compiler} = programData; @@ -101,7 +116,7 @@ function runUndecoratedClassesMigration( // file in order to avoid shifted character offsets. updateRecorders.forEach(recorder => recorder.commitUpdate()); - return failures; + return {failures}; /** Gets the update recorder for the specified source file. */ function getUpdateRecorder(sourceFile: ts.SourceFile): UpdateRecorder { @@ -153,7 +168,6 @@ function gracefullyCreateProgram( `\nTypeScript project "${tsconfigPath}" has syntactical errors which could cause ` + `an incomplete migration. Please fix the following failures and rerun the migration:`); logger.error(ts.formatDiagnostics(syntacticDiagnostics, host)); - logger.info(MIGRATION_RERUN_MESSAGE); return null; } @@ -165,7 +179,6 @@ function gracefullyCreateProgram( } catch (e) { logger.warn(`\n${MIGRATION_AOT_FAILURE}. The following project failed: ${tsconfigPath}\n`); logger.error(`${e.toString()}\n`); - logger.info(MIGRATION_RERUN_MESSAGE); return null; } } diff --git a/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts b/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts index 06dfbb231d..e33e33749b 100644 --- a/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts +++ b/packages/core/schematics/test/undecorated_classes_with_di_migration_spec.ts @@ -21,6 +21,7 @@ describe('Undecorated classes with DI migration', () => { let previousWorkingDir: string; let warnOutput: string[]; let errorOutput: string[]; + let infoOutput: string[]; beforeEach(() => { runner = new SchematicTestRunner('test', require.resolve('../migrations.json')); @@ -39,11 +40,14 @@ describe('Undecorated classes with DI migration', () => { warnOutput = []; errorOutput = []; + infoOutput = []; runner.logger.subscribe(logEntry => { if (logEntry.level === 'warn') { warnOutput.push(logEntry.message); } else if (logEntry.level === 'error') { errorOutput.push(logEntry.message); + } else if (logEntry.level === 'info') { + infoOutput.push(logEntry.message); } }); @@ -112,6 +116,10 @@ describe('Undecorated classes with DI migration', () => { expect(errorOutput.length).toBe(0); expect(warnOutput.length).toBe(1); expect(warnOutput[0]).toMatch(/Class needs to declare an explicit constructor./); + expect(infoOutput.join(' ')) + .toContain( + 'Could not migrate all undecorated classes that use ' + + 'dependency injection. Please manually fix the following failures'); }); it('should add @Directive() decorator to extended base class', async() => { @@ -1442,6 +1450,10 @@ describe('Undecorated classes with DI migration', () => { /ensure there are no AOT compilation errors and rerun the migration.*project failed: tsconfig\.json/); expect(errorOutput.length).toBe(1); expect(errorOutput[0]).toMatch(/Cannot determine the module for class TestComp/); + expect(infoOutput.join(' ')) + .toContain( + 'Some project targets could not be analyzed due to ' + + 'TypeScript program failures'); }); it('should gracefully exit migration if project fails with syntactical diagnostic', async() => { @@ -1456,6 +1468,10 @@ describe('Undecorated classes with DI migration', () => { .toMatch(/project "tsconfig.json" has syntactical errors which could cause/); expect(errorOutput.length).toBe(1); expect(errorOutput[0]).toMatch(/error TS1005: 'from' expected/); + expect(infoOutput.join(' ')) + .toContain( + 'Some project targets could not be analyzed due to ' + + 'TypeScript program failures'); }); it('should not throw if resources could not be read', async() => {