Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

Closed
wants to merge 1 commit into from
Closed

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/compiler-cli/src/main.ts
View file
Open in desktop
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
View file
Open in desktop
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alxhub this is the key change. The IncrementalState class takes a Set<string>|null for its modifiedResourceFiles parameter. The modifiedResourceFiles property could never be undefined, instead it could be a Set<string> or it could be null.

To tighten up the types across the system we changed everything else so that all properties and variables that could hold this modified resource files value could also never by undefined.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative approach would have been to change every type to Set<string>|undefined instead. But my understanding is that you prefer to use null when something is explicitly not being provided.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return false;
}
const resourceDeps = this.metadata.get(sf) !.resourcePaths;
Expand Down
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/perform_compile.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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