From 4aecf9253bf0854e765b1f80ebf384e9a31ef113 Mon Sep 17 00:00:00 2001 From: Matt Lewis Date: Thu, 27 Jun 2019 19:25:00 -0400 Subject: [PATCH] fix(ivy): support older CLI versions that do not pass a list of changed files (#31322) Versions of CLI prior to angular/angular-cli@0e339ee did not expose the host.getModifiedResourceFiles() method. This meant that null was being passed through to the IncrementalState.reconcile() method to indicate that there were either no changes or the host didn't support that method. This commit fixes a bug where we were checking for undefined rather than null when deciding whether any resource files had changed, causing a null reference error to be thrown. This bug was not caught by the unit testing because the tests set up the changed files via a slightly different process, not having access to the CompilerHost, and these test were making the erroneous assumption that undefined indicated that there were no changed files. PR Close #31322 --- packages/compiler-cli/src/main.ts | 2 +- .../src/ngtsc/incremental/src/state.ts | 2 +- packages/compiler-cli/src/perform_compile.ts | 4 ++-- packages/compiler-cli/test/ngtsc/env.ts | 8 +++++++- .../compiler-cli/test/ngtsc/incremental_spec.ts | 17 +++++++++++++++++ 5 files changed, 28 insertions(+), 5 deletions(-) diff --git a/packages/compiler-cli/src/main.ts b/packages/compiler-cli/src/main.ts index 13ca9d5c22..050d6d089e 100644 --- a/packages/compiler-cli/src/main.ts +++ b/packages/compiler-cli/src/main.ts @@ -26,7 +26,7 @@ export function main( config?: NgcParsedConfiguration, customTransformers?: api.CustomTransformers, programReuse?: { program: api.Program | undefined, }, - modifiedResourceFiles?: Set): number { + modifiedResourceFiles?: Set| null): number { let {project, rootNames, options, errors: configErrors, watch, emitFlags} = config || readNgcCommandLineAndConfiguration(args); if (configErrors.length) { diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts index 983dde26a5..9f5fa79791 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts @@ -112,7 +112,7 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta } private hasChangedResourceDependencies(sf: ts.SourceFile): boolean { - if (this.modifiedResourceFiles === undefined || !this.metadata.has(sf)) { + if (this.modifiedResourceFiles === null || !this.metadata.has(sf)) { return false; } const resourceDeps = this.metadata.get(sf) !.resourcePaths; diff --git a/packages/compiler-cli/src/perform_compile.ts b/packages/compiler-cli/src/perform_compile.ts index 4d27b2b323..f7ea7bbb59 100644 --- a/packages/compiler-cli/src/perform_compile.ts +++ b/packages/compiler-cli/src/perform_compile.ts @@ -224,7 +224,7 @@ export function exitCodeFromResult(diags: Diagnostics | undefined): number { export function performCompilation( {rootNames, options, host, oldProgram, emitCallback, mergeEmitResultsCallback, gatherDiagnostics = defaultGatherDiagnostics, customTransformers, - emitFlags = api.EmitFlags.Default, modifiedResourceFiles}: { + emitFlags = api.EmitFlags.Default, modifiedResourceFiles = null}: { rootNames: string[], options: api.CompilerOptions, host?: api.CompilerHost, @@ -234,7 +234,7 @@ export function performCompilation( gatherDiagnostics?: (program: api.Program) => Diagnostics, customTransformers?: api.CustomTransformers, emitFlags?: api.EmitFlags, - modifiedResourceFiles?: Set, + modifiedResourceFiles?: Set| null, }): PerformCompilationResult { let program: api.Program|undefined; let emitResult: ts.EmitResult|undefined; diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts index efb1fe6793..1f1639688d 100644 --- a/packages/compiler-cli/test/ngtsc/env.ts +++ b/packages/compiler-cli/test/ngtsc/env.ts @@ -26,7 +26,7 @@ import {setWrapHostForTest} from '../../src/transformers/compiler_host'; export class NgtscTestEnvironment { private multiCompileHostExt: MultiCompileHostExt|null = null; private oldProgram: Program|null = null; - private changedResources: Set|undefined = undefined; + private changedResources: Set|null = null; private constructor( private fs: FileSystem, readonly outDir: AbsoluteFsPath, readonly basePath: AbsoluteFsPath) {} @@ -107,6 +107,12 @@ export class NgtscTestEnvironment { this.multiCompileHostExt.flushWrittenFileTracking(); } + /** + * Older versions of the CLI do not provide the `CompilerHost.getModifiedResourceFiles()` method. + * This results in the `changedResources` set being `null`. + */ + simulateLegacyCLICompilerHost() { this.changedResources = null; } + getFilesWrittenSinceLastFlush(): Set { if (this.multiCompileHostExt === null) { throw new Error(`Not tracking written files - call enableMultipleCompilations()`); diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index ee288769d3..0ed8697d94 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -23,6 +23,23 @@ runInEachFileSystem(() => { env.tsconfig(); }); + it('should not crash if CLI does not provide getModifiedResourceFiles()', () => { + env.write('component1.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'cmp', templateUrl: './component1.template.html'}) + export class Cmp1 {} + `); + env.write('component1.template.html', 'cmp1'); + env.driveMain(); + + // Simulate a change to `component1.html` + env.flushWrittenFileTracking(); + env.invalidateCachedFile('component1.html'); + env.simulateLegacyCLICompilerHost(); + env.driveMain(); + }); + it('should skip unchanged services', () => { env.write('service.ts', ` import {Injectable} from '@angular/core';