Skip to content

Commit

Permalink
fix(ivy): support older CLI versions that do not pass a list of chang…
Browse files Browse the repository at this point in the history
…ed 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
  • Loading branch information
mattlewis92 authored and mhevery committed Jul 18, 2019
1 parent 60f58bf commit 4aecf92
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 5 deletions.
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function main(
config?: NgcParsedConfiguration, customTransformers?: api.CustomTransformers, programReuse?: {
program: api.Program | undefined,
},
modifiedResourceFiles?: Set<string>): number {
modifiedResourceFiles?: Set<string>| null): number {
let {project, rootNames, options, errors: configErrors, watch, emitFlags} =
config || readNgcCommandLineAndConfiguration(args);
if (configErrors.length) {
Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/ngtsc/incremental/src/state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/perform_compile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -234,7 +234,7 @@ export function performCompilation(
gatherDiagnostics?: (program: api.Program) => Diagnostics,
customTransformers?: api.CustomTransformers,
emitFlags?: api.EmitFlags,
modifiedResourceFiles?: Set<string>,
modifiedResourceFiles?: Set<string>| null,
}): PerformCompilationResult {
let program: api.Program|undefined;
let emitResult: ts.EmitResult|undefined;
Expand Down
8 changes: 7 additions & 1 deletion packages/compiler-cli/test/ngtsc/env.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>|undefined = undefined;
private changedResources: Set<string>|null = null;

private constructor(
private fs: FileSystem, readonly outDir: AbsoluteFsPath, readonly basePath: AbsoluteFsPath) {}
Expand Down Expand Up @@ -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<string> {
if (this.multiCompileHostExt === null) {
throw new Error(`Not tracking written files - call enableMultipleCompilations()`);
Expand Down
17 changes: 17 additions & 0 deletions packages/compiler-cli/test/ngtsc/incremental_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down

0 comments on commit 4aecf92

Please sign in to comment.