From 15f8638b1cbc376a8a769ea0b1e62a8f4613cd31 Mon Sep 17 00:00:00 2001 From: JoostK Date: Thu, 31 Oct 2019 20:30:05 +0100 Subject: [PATCH] fix(ivy): ensure module scope is rebuild on dependent change (#33522) During incremental compilations, ngtsc needs to know which metadata from a previous compilation can be reused, versus which metadata has to be recomputed as some dependency was updated. Changes to directives/components should cause the NgModule in which they are declared to be recompiled, as the NgModule's compilation is dependent on its directives/components. When a dependent source file of a directive/component is updated, however, a more subtle dependency should also cause to NgModule's source file to be invalidated. During the reconciliation of state from a previous compilation into the new program, the component's source file is invalidated because one of its dependency has changed, ergo the NgModule needs to be invalidated as well. Up until now, this implicit dependency was not imposed on the NgModule. Additionally, any change to a dependent file may influence the module scope to change, so all components within the module must be invalidated as well. This commit fixes the bug by introducing additional file dependencies, as to ensure a proper rebuild of the module scope and its components. Fixes #32416 PR Close #33522 --- .../src/ngtsc/incremental/src/state.ts | 7 +++ .../src/ngtsc/transform/src/compilation.ts | 38 ++++++++++--- .../test/ngtsc/incremental_spec.ts | 56 +++++++++++++++++++ 3 files changed, 94 insertions(+), 7 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts index b95983f642..0fc92f7709 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts @@ -69,6 +69,13 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta metadata.fileDependencies.add(dep); } + trackFileDependencies(deps: ts.SourceFile[], src: ts.SourceFile) { + const metadata = this.ensureMetadata(src); + for (const dep of deps) { + metadata.fileDependencies.add(dep); + } + } + getFileDependencies(file: ts.SourceFile): ts.SourceFile[] { if (!this.metadata.has(file)) { return []; diff --git a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts index 381e5a2c48..8feac815a9 100644 --- a/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts +++ b/packages/compiler-cli/src/ngtsc/transform/src/compilation.ts @@ -307,13 +307,37 @@ export class IvyCompilation { const recordSpan = this.perf.start('recordDependencies'); this.scopeRegistry !.getCompilationScopes().forEach(scope => { const file = scope.declaration.getSourceFile(); - // Register the file containing the NgModule where the declaration is declared. - this.incrementalState.trackFileDependency(scope.ngModule.getSourceFile(), file); - scope.directives.forEach( - directive => - this.incrementalState.trackFileDependency(directive.ref.node.getSourceFile(), file)); - scope.pipes.forEach( - pipe => this.incrementalState.trackFileDependency(pipe.ref.node.getSourceFile(), file)); + const ngModuleFile = scope.ngModule.getSourceFile(); + + // A change to any dependency of the declaration causes the declaration to be invalidated, + // which requires the NgModule to be invalidated as well. + const deps = this.incrementalState.getFileDependencies(file); + this.incrementalState.trackFileDependencies(deps, ngModuleFile); + + // A change to the NgModule file should cause the declaration itself to be invalidated. + this.incrementalState.trackFileDependency(ngModuleFile, file); + + // A change to any directive/pipe in the compilation scope should cause the declaration to be + // invalidated. + scope.directives.forEach(directive => { + const dirSf = directive.ref.node.getSourceFile(); + + // When a directive in scope is updated, the declaration needs to be recompiled as e.g. + // a selector may have changed. + this.incrementalState.trackFileDependency(dirSf, file); + + // When any of the dependencies of the declaration changes, the NgModule scope may be + // affected so a component within scope must be recompiled. Only components need to be + // recompiled, as directives are not dependent upon the compilation scope. + if (directive.isComponent) { + this.incrementalState.trackFileDependencies(deps, dirSf); + } + }); + scope.pipes.forEach(pipe => { + // When a pipe in scope is updated, the declaration needs to be recompiled as e.g. + // the pipe's name may have changed. + this.incrementalState.trackFileDependency(pipe.ref.node.getSourceFile(), file); + }); }); this.perf.stop(recordSpan); } diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index b84b7c2b6c..f46f25590c 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -165,6 +165,62 @@ runInEachFileSystem(() => { expect(written).not.toContain('/foo_module.js'); }); + // https://github.com/angular/angular/issues/32416 + it('should rebuild full NgModule scope when a dependency of a declaration has changed', () => { + env.write('component1.ts', ` + import {Component} from '@angular/core'; + import {SELECTOR} from './dep'; + + @Component({selector: SELECTOR, template: 'cmp'}) + export class Cmp1 {} + `); + env.write('component2.ts', ` + import {Component} from '@angular/core'; + + @Component({selector: 'cmp2', template: 'cmp2'}) + export class Cmp2 {} + `); + env.write('dep.ts', ` + export const SELECTOR = 'cmp'; + `); + env.write('directive.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: 'dir'}) + export class Dir {} + `); + env.write('pipe.ts', ` + import {Pipe} from '@angular/core'; + + @Pipe({name: 'myPipe'}) + export class MyPipe {} + `); + env.write('module.ts', ` + import {NgModule} from '@angular/core'; + import {Cmp1} from './component1'; + import {Cmp2} from './component2'; + import {Dir} from './directive'; + import {MyPipe} from './pipe'; + + @NgModule({declarations: [Cmp1, Cmp2, Dir, MyPipe]}) + export class Mod {} + `); + env.driveMain(); + + // Pretend a change was made to 'dep'. Since this may affect the NgModule scope, like it does + // here if the selector is updated, all components in the module scope need to be recompiled. + env.flushWrittenFileTracking(); + env.invalidateCachedFile('dep.ts'); + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).not.toContain('/directive.js'); + expect(written).not.toContain('/pipe.js'); + expect(written).toContain('/component1.js'); + expect(written).toContain('/component2.js'); + expect(written).toContain('/dep.js'); + expect(written).toContain('/module.js'); + }); + it('should rebuild components where their NgModule declared dependencies have changed', () => { setupFooBarProgram(env);