Skip to content

Commit

Permalink
fix(ivy): recompile on template change in ngc watch mode on Windows (a…
Browse files Browse the repository at this point in the history
…ngular#34015)

In angular#33551, a bug in `ngc --watch` mode was fixed so that a component is
recompiled when its template file is changed. Due to insufficient
normalization of files paths, this fix did not have the desired effect
on Windows.

Fixes angular#32869

PR Close angular#34015
  • Loading branch information
JoostK authored and sonukapoor committed Feb 13, 2020
1 parent f459b01 commit f4f4db5
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 19 deletions.
4 changes: 2 additions & 2 deletions packages/compiler-cli/src/ngtsc/annotations/src/component.ts
Expand Up @@ -273,7 +273,7 @@ export class ComponentDecoratorHandler implements
const resourceStr = this.resourceLoader.load(resourceUrl);
styles.push(resourceStr);
if (this.depTracker !== null) {
this.depTracker.addResourceDependency(node.getSourceFile(), resourceUrl);
this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl));
}
}
}
Expand Down Expand Up @@ -675,7 +675,7 @@ export class ComponentDecoratorHandler implements
resourceUrl: string): ParsedTemplateWithSource {
const templateStr = this.resourceLoader.load(resourceUrl);
if (this.depTracker !== null) {
this.depTracker.addResourceDependency(node.getSourceFile(), resourceUrl);
this.depTracker.addResourceDependency(node.getSourceFile(), absoluteFrom(resourceUrl));
}

const template = this._parseTemplate(
Expand Down
2 changes: 2 additions & 0 deletions packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel
Expand Up @@ -9,6 +9,7 @@ ts_library(
]),
deps = [
":api",
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/metadata",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
Expand All @@ -24,6 +25,7 @@ ts_library(
name = "api",
srcs = ["api.ts"],
deps = [
"//packages/compiler-cli/src/ngtsc/file_system",
"@npm//typescript",
],
)
3 changes: 2 additions & 1 deletion packages/compiler-cli/src/ngtsc/incremental/api.ts
Expand Up @@ -7,6 +7,7 @@
*/

import * as ts from 'typescript';
import {AbsoluteFsPath} from '../file_system';

/**
* Interface of the incremental build engine.
Expand All @@ -33,7 +34,7 @@ export interface DependencyTracker<T extends{fileName: string} = ts.SourceFile>
/**
* Record that the file `from` depends on the resource file `on`.
*/
addResourceDependency(from: T, on: string): void;
addResourceDependency(from: T, on: AbsoluteFsPath): void;

/**
* Record that the file `from` depends on the file `on` as well as `on`'s direct dependencies.
Expand Down
Expand Up @@ -8,6 +8,7 @@

import * as ts from 'typescript';

import {AbsoluteFsPath} from '../../file_system';
import {DependencyTracker} from '../api';

/**
Expand All @@ -28,7 +29,7 @@ export class FileDependencyGraph<T extends{fileName: string} = ts.SourceFile> im

addDependency(from: T, on: T): void { this.nodeFor(from).dependsOn.add(on.fileName); }

addResourceDependency(from: T, resource: string): void {
addResourceDependency(from: T, resource: AbsoluteFsPath): void {
this.nodeFor(from).usesResources.add(resource);
}

Expand All @@ -50,7 +51,7 @@ export class FileDependencyGraph<T extends{fileName: string} = ts.SourceFile> im
}
}

isStale(sf: T, changedTsPaths: Set<string>, changedResources: Set<string>): boolean {
isStale(sf: T, changedTsPaths: Set<string>, changedResources: Set<AbsoluteFsPath>): boolean {
return isLogicallyChanged(sf, this.nodeFor(sf), changedTsPaths, EMPTY_SET, changedResources);
}

Expand All @@ -77,7 +78,7 @@ export class FileDependencyGraph<T extends{fileName: string} = ts.SourceFile> im
*/
updateWithPhysicalChanges(
previous: FileDependencyGraph<T>, changedTsPaths: Set<string>, deletedTsPaths: Set<string>,
changedResources: Set<string>): Set<string> {
changedResources: Set<AbsoluteFsPath>): Set<string> {
const logicallyChanged = new Set<string>();

for (const sf of previous.nodes.keys()) {
Expand All @@ -99,7 +100,7 @@ export class FileDependencyGraph<T extends{fileName: string} = ts.SourceFile> im
if (!this.nodes.has(sf)) {
this.nodes.set(sf, {
dependsOn: new Set<string>(),
usesResources: new Set<string>(),
usesResources: new Set<AbsoluteFsPath>(),
});
}
return this.nodes.get(sf) !;
Expand All @@ -112,13 +113,13 @@ export class FileDependencyGraph<T extends{fileName: string} = ts.SourceFile> im
*/
function isLogicallyChanged<T extends{fileName: string}>(
sf: T, node: FileNode, changedTsPaths: ReadonlySet<string>, deletedTsPaths: ReadonlySet<string>,
changedResources: ReadonlySet<string>): boolean {
changedResources: ReadonlySet<AbsoluteFsPath>): boolean {
// A file is logically changed if it has physically changed itself (including being deleted).
if (changedTsPaths.has(sf.fileName) || deletedTsPaths.has(sf.fileName)) {
return true;
}

// A file is logically changed if one of its dependencies has physically cxhanged.
// A file is logically changed if one of its dependencies has physically changed.
for (const dep of node.dependsOn) {
if (changedTsPaths.has(dep) || deletedTsPaths.has(dep)) {
return true;
Expand All @@ -136,7 +137,7 @@ function isLogicallyChanged<T extends{fileName: string}>(

interface FileNode {
dependsOn: Set<string>;
usesResources: Set<string>;
usesResources: Set<AbsoluteFsPath>;
}

const EMPTY_SET: ReadonlySet<any> = new Set<any>();
9 changes: 5 additions & 4 deletions packages/compiler-cli/src/ngtsc/incremental/src/state.ts
Expand Up @@ -8,6 +8,7 @@

import * as ts from 'typescript';

import {AbsoluteFsPath, absoluteFrom} from '../../file_system';
import {ClassRecord, TraitCompiler} from '../../transform';
import {IncrementalBuild} from '../api';

Expand Down Expand Up @@ -52,7 +53,7 @@ export class IncrementalDriver implements IncrementalBuild<ClassRecord> {
state = {
kind: BuildStateKind.Pending,
pendingEmit: oldDriver.state.pendingEmit,
changedResourcePaths: new Set<string>(),
changedResourcePaths: new Set<AbsoluteFsPath>(),
changedTsPaths: new Set<string>(),
lastGood: oldDriver.state.lastGood,
};
Expand All @@ -61,7 +62,7 @@ export class IncrementalDriver implements IncrementalBuild<ClassRecord> {
// Merge the freshly modified resource files with any prior ones.
if (modifiedResourceFiles !== null) {
for (const resFile of modifiedResourceFiles) {
state.changedResourcePaths.add(resFile);
state.changedResourcePaths.add(absoluteFrom(resFile));
}
}

Expand Down Expand Up @@ -153,7 +154,7 @@ export class IncrementalDriver implements IncrementalBuild<ClassRecord> {
const state: PendingBuildState = {
kind: BuildStateKind.Pending,
pendingEmit: new Set<string>(tsFiles.map(sf => sf.fileName)),
changedResourcePaths: new Set<string>(),
changedResourcePaths: new Set<AbsoluteFsPath>(),
changedTsPaths: new Set<string>(),
lastGood: null,
};
Expand Down Expand Up @@ -287,7 +288,7 @@ interface PendingBuildState extends BaseBuildState {
/**
* Set of resource file paths which have changed since the last successfully analyzed build.
*/
changedResourcePaths: Set<string>;
changedResourcePaths: Set<AbsoluteFsPath>;
}

interface AnalyzedBuildState extends BaseBuildState {
Expand Down
10 changes: 6 additions & 4 deletions packages/compiler-cli/src/perform_watch.ts
Expand Up @@ -246,11 +246,13 @@ export function performWatchCompilation(host: PerformWatchHost):
}

function watchedFileChanged(event: FileChangeEvent, fileName: string) {
const normalizedPath = path.normalize(fileName);

if (cachedOptions && event === FileChangeEvent.Change &&
// TODO(chuckj): validate that this is sufficient to skip files that were written.
// This assumes that the file path we write is the same file path we will receive in the
// change notification.
path.normalize(fileName) === path.normalize(cachedOptions.project)) {
normalizedPath === path.normalize(cachedOptions.project)) {
// If the configuration file changes, forget everything and start the recompilation timer
resetOptions();
} else if (
Expand All @@ -263,12 +265,12 @@ export function performWatchCompilation(host: PerformWatchHost):
if (event === FileChangeEvent.CreateDeleteDir) {
fileCache.clear();
} else {
fileCache.delete(path.normalize(fileName));
fileCache.delete(normalizedPath);
}

if (!ignoreFilesForWatch.has(path.normalize(fileName))) {
if (!ignoreFilesForWatch.has(normalizedPath)) {
// Ignore the file if the file is one that was written by the compiler.
startTimerForRecompilation(fileName);
startTimerForRecompilation(normalizedPath);
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/compiler-cli/test/perform_watch_spec.ts
Expand Up @@ -64,7 +64,7 @@ describe('perform watch', () => {
const watchResult = performWatchCompilation(host);
expectNoDiagnostics(config.options, watchResult.firstCompileResult);

const htmlPath = path.posix.join(testSupport.basePath, 'src', 'main.html');
const htmlPath = path.join(testSupport.basePath, 'src', 'main.html');
const genPath = ivyEnabled ? path.posix.join(outDir, 'src', 'main.js') :
path.posix.join(outDir, 'src', 'main.ngfactory.js');

Expand Down

0 comments on commit f4f4db5

Please sign in to comment.