From d6430436fb205de2c3a20efdc88ab1a3a35c7da9 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 31 Jul 2019 17:11:33 +0100 Subject: [PATCH] fix(ivy): reuse compilation scope for incremental template changes. Previously if only a component template changed then we would know to rebuild its component source file. But the compilation was incorrect if the component was part of an NgModule, since we were not capturing the compilation scope information that had a been acquired from the NgModule and was not being regenerated since we were not needing to recompile the NgModule. Now we register compilation scope information for each component, via the `ComponentScopeRegistry` interface, so that it is available for incremental compilation. The `ComponentDecoratorHandler` now reads the compilation scope from a `ComponentScopeReader` interface which is implemented as a compound reader composed of the original `LocalModuleScopeRegistry` and the `IncrementalState`. Fixes #31654 --- .../ngcc/src/analysis/decoration_analyzer.ts | 2 +- .../src/ngtsc/annotations/src/component.ts | 12 +-- .../ngtsc/annotations/test/component_spec.ts | 2 +- .../src/ngtsc/incremental/BUILD.bazel | 1 + .../src/ngtsc/incremental/src/state.ts | 79 ++++++++++++++++--- packages/compiler-cli/src/ngtsc/program.ts | 14 ++-- .../compiler-cli/src/ngtsc/scope/index.ts | 1 + .../src/ngtsc/scope/src/component_scope.ts | 67 ++++++++++++++++ .../compiler-cli/src/ngtsc/scope/src/local.ts | 18 +++-- .../test/ngtsc/incremental_spec.ts | 22 +++++- 10 files changed, 188 insertions(+), 30 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts diff --git a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts index bfc8abc24bac0..49ba5391e0520 100644 --- a/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts +++ b/packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts @@ -75,7 +75,7 @@ export class DecorationAnalyzer { new BaseDefDecoratorHandler(this.reflectionHost, this.evaluator, this.isCore), new ComponentDecoratorHandler( this.reflectionHost, this.evaluator, this.fullRegistry, this.fullMetaReader, - this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs, + this.scopeRegistry, this.scopeRegistry, this.isCore, this.resourceManager, this.rootDirs, /* defaultPreserveWhitespaces */ false, /* i18nUseExternalIds */ true, this.moduleResolver, this.cycleAnalyzer, this.refEmitter, NOOP_DEFAULT_IMPORT_RECORDER), diff --git a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts index 93d8a3ce30731..dc7a0014bb7c8 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/src/component.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/src/component.ts @@ -18,7 +18,7 @@ import {DirectiveMeta, MetadataReader, MetadataRegistry, extractDirectiveGuards} import {flattenInheritedDirectiveMetadata} from '../../metadata/src/inheritance'; import {EnumValue, PartialEvaluator} from '../../partial_evaluator'; import {ClassDeclaration, Decorator, ReflectionHost, reflectObjectLiteral} from '../../reflection'; -import {LocalModuleScopeRegistry} from '../../scope'; +import {ComponentScopeReader, LocalModuleScopeRegistry} from '../../scope'; import {AnalysisOutput, CompileResult, DecoratorHandler, DetectResult, HandlerPrecedence, ResolveResult} from '../../transform'; import {TypeCheckContext} from '../../typecheck'; import {NoopResourceDependencyRecorder, ResourceDependencyRecorder} from '../../util/src/resource_recorder'; @@ -47,8 +47,8 @@ export class ComponentDecoratorHandler implements constructor( private reflector: ReflectionHost, private evaluator: PartialEvaluator, private metaRegistry: MetadataRegistry, private metaReader: MetadataReader, - private scopeRegistry: LocalModuleScopeRegistry, private isCore: boolean, - private resourceLoader: ResourceLoader, private rootDirs: string[], + private scopeReader: ComponentScopeReader, private scopeRegistry: LocalModuleScopeRegistry, + private isCore: boolean, private resourceLoader: ResourceLoader, private rootDirs: string[], private defaultPreserveWhitespaces: boolean, private i18nUseExternalIds: boolean, private moduleResolver: ModuleResolver, private cycleAnalyzer: CycleAnalyzer, private refEmitter: ReferenceEmitter, private defaultImportRecorder: DefaultImportRecorder, @@ -327,7 +327,7 @@ export class ComponentDecoratorHandler implements preserveWhitespaces: true, leadingTriviaChars: [], }); - const scope = this.scopeRegistry.getScopeForComponent(node); + const scope = this.scopeReader.getScopeForComponent(node); const selector = analysis.meta.selector; const matcher = new SelectorMatcher(); if (scope !== null) { @@ -353,7 +353,7 @@ export class ComponentDecoratorHandler implements if (!ts.isClassDeclaration(node)) { return; } - const scope = this.scopeRegistry.getScopeForComponent(node); + const scope = this.scopeReader.getScopeForComponent(node); const matcher = new SelectorMatcher(); if (scope !== null) { for (const meta of scope.compilation.directives) { @@ -377,7 +377,7 @@ export class ComponentDecoratorHandler implements const context = node.getSourceFile(); // Check whether this component was registered with an NgModule. If so, it should be compiled // under that module's compilation scope. - const scope = this.scopeRegistry.getScopeForComponent(node); + const scope = this.scopeReader.getScopeForComponent(node); let metadata = analysis.meta; if (scope !== null) { // Replace the empty components and directives from the analyze() step with a fully expanded diff --git a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts index f265235f92dd9..b1481d11eafa4 100644 --- a/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts +++ b/packages/compiler-cli/src/ngtsc/annotations/test/component_spec.ts @@ -60,7 +60,7 @@ runInEachFileSystem(() => { const refEmitter = new ReferenceEmitter([]); const handler = new ComponentDecoratorHandler( - reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, false, + reflectionHost, evaluator, metaRegistry, metaReader, scopeRegistry, scopeRegistry, false, new NoopResourceLoader(), [''], false, true, moduleResolver, cycleAnalyzer, refEmitter, NOOP_DEFAULT_IMPORT_RECORDER); const TestCmp = getDeclaration(program, _('/entry.ts'), 'TestCmp', isNamedClassDeclaration); diff --git a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel index 239ac00f5ace3..9f3bc7655fbdf 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel +++ b/packages/compiler-cli/src/ngtsc/incremental/BUILD.bazel @@ -12,6 +12,7 @@ ts_library( "//packages/compiler-cli/src/ngtsc/metadata", "//packages/compiler-cli/src/ngtsc/partial_evaluator", "//packages/compiler-cli/src/ngtsc/reflection", + "//packages/compiler-cli/src/ngtsc/scope", "//packages/compiler-cli/src/ngtsc/util", "@npm//typescript", ], diff --git a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts index 9f5fa79791c5c..b95983f6425e2 100644 --- a/packages/compiler-cli/src/ngtsc/incremental/src/state.ts +++ b/packages/compiler-cli/src/ngtsc/incremental/src/state.ts @@ -12,13 +12,14 @@ import {Reference} from '../../imports'; import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata'; import {DependencyTracker} from '../../partial_evaluator'; import {ClassDeclaration} from '../../reflection'; +import {ComponentScopeReader, ComponentScopeRegistry, LocalModuleScope} from '../../scope'; import {ResourceDependencyRecorder} from '../../util/src/resource_recorder'; /** * Accumulates state between compilations. */ export class IncrementalState implements DependencyTracker, MetadataReader, MetadataRegistry, - ResourceDependencyRecorder { + ResourceDependencyRecorder, ComponentScopeRegistry, ComponentScopeReader { private constructor( private unchangedFiles: Set, private metadata: Map, @@ -69,32 +70,56 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta } getFileDependencies(file: ts.SourceFile): ts.SourceFile[] { - const meta = this.metadata.get(file); - return meta ? Array.from(meta.fileDependencies) : []; + if (!this.metadata.has(file)) { + return []; + } + const meta = this.metadata.get(file) !; + return Array.from(meta.fileDependencies); } getNgModuleMetadata(ref: Reference): NgModuleMeta|null { - const metadata = this.metadata.get(ref.node.getSourceFile()) || null; - return metadata && metadata.ngModuleMeta.get(ref.node) || null; + if (!this.metadata.has(ref.node.getSourceFile())) { + return null; + } + const metadata = this.metadata.get(ref.node.getSourceFile()) !; + if (!metadata.ngModuleMeta.has(ref.node)) { + return null; + } + return metadata.ngModuleMeta.get(ref.node) !; } + registerNgModuleMetadata(meta: NgModuleMeta): void { const metadata = this.ensureMetadata(meta.ref.node.getSourceFile()); metadata.ngModuleMeta.set(meta.ref.node, meta); } getDirectiveMetadata(ref: Reference): DirectiveMeta|null { - const metadata = this.metadata.get(ref.node.getSourceFile()) || null; - return metadata && metadata.directiveMeta.get(ref.node) || null; + if (!this.metadata.has(ref.node.getSourceFile())) { + return null; + } + const metadata = this.metadata.get(ref.node.getSourceFile()) !; + if (!metadata.directiveMeta.has(ref.node)) { + return null; + } + return metadata.directiveMeta.get(ref.node) !; } + registerDirectiveMetadata(meta: DirectiveMeta): void { const metadata = this.ensureMetadata(meta.ref.node.getSourceFile()); metadata.directiveMeta.set(meta.ref.node, meta); } getPipeMetadata(ref: Reference): PipeMeta|null { - const metadata = this.metadata.get(ref.node.getSourceFile()) || null; - return metadata && metadata.pipeMeta.get(ref.node) || null; + if (!this.metadata.has(ref.node.getSourceFile())) { + return null; + } + const metadata = this.metadata.get(ref.node.getSourceFile()) !; + if (!metadata.pipeMeta.has(ref.node)) { + return null; + } + return metadata.pipeMeta.get(ref.node) !; } + registerPipeMetadata(meta: PipeMeta): void { const metadata = this.ensureMetadata(meta.ref.node.getSourceFile()); metadata.pipeMeta.set(meta.ref.node, meta); @@ -105,6 +130,40 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta metadata.resourcePaths.add(resourcePath); } + registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void { + const metadata = this.ensureMetadata(clazz.getSourceFile()); + metadata.componentScope.set(clazz, scope); + } + + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { + if (!this.metadata.has(clazz.getSourceFile())) { + return null; + } + const metadata = this.metadata.get(clazz.getSourceFile()) !; + if (!metadata.componentScope.has(clazz)) { + return null; + } + return metadata.componentScope.get(clazz) !; + } + + setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void { + const metadata = this.ensureMetadata(clazz.getSourceFile()); + metadata.remoteScoping.add(clazz); + } + + getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null { + // TODO: https://angular-team.atlassian.net/browse/FW-1501 + // Handle the incremental build case where a component requires remote scoping. + // This means that if the the component's template changes, it requires the module to be + // re-emitted. + // Also, we need to make sure the cycle detector works well across rebuilds. + if (!this.metadata.has(clazz.getSourceFile())) { + return null; + } + const metadata = this.metadata.get(clazz.getSourceFile()) !; + return metadata.remoteScoping.has(clazz); + } + private ensureMetadata(sf: ts.SourceFile): FileMetadata { const metadata = this.metadata.get(sf) || new FileMetadata(); this.metadata.set(sf, metadata); @@ -131,4 +190,6 @@ class FileMetadata { directiveMeta = new Map(); ngModuleMeta = new Map(); pipeMeta = new Map(); + componentScope = new Map(); + remoteScoping = new Set(); } diff --git a/packages/compiler-cli/src/ngtsc/program.ts b/packages/compiler-cli/src/ngtsc/program.ts index fd1219422dbdb..aae2008225eb1 100644 --- a/packages/compiler-cli/src/ngtsc/program.ts +++ b/packages/compiler-cli/src/ngtsc/program.ts @@ -28,7 +28,7 @@ import {NOOP_PERF_RECORDER, PerfRecorder, PerfTracker} from './perf'; import {TypeScriptReflectionHost} from './reflection'; import {HostResourceLoader} from './resource_loader'; import {NgModuleRouteAnalyzer, entryPointKeyFor} from './routing'; -import {LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope'; +import {CompoundComponentScopeReader, LocalModuleScopeRegistry, MetadataDtsModuleScopeResolver} from './scope'; import {FactoryGenerator, FactoryInfo, GeneratedShimsHostWrapper, ShimGenerator, SummaryGenerator, TypeCheckShimGenerator, generatedFactoryTransform} from './shims'; import {ivySwitchTransform} from './switch'; import {IvyCompilation, declarationTransformFactory, ivyTransformFactory} from './transform'; @@ -476,7 +476,8 @@ export class NgtscProgram implements api.Program { const localMetaReader = new CompoundMetadataReader([localMetaRegistry, this.incrementalState]); const depScopeReader = new MetadataDtsModuleScopeResolver(dtsReader, aliasGenerator); const scopeRegistry = new LocalModuleScopeRegistry( - localMetaReader, depScopeReader, this.refEmitter, aliasGenerator); + localMetaReader, depScopeReader, this.refEmitter, aliasGenerator, this.incrementalState); + const scopeReader = new CompoundComponentScopeReader([scopeRegistry, this.incrementalState]); const metaRegistry = new CompoundMetadataRegistry([localMetaRegistry, scopeRegistry, this.incrementalState]); @@ -502,10 +503,11 @@ export class NgtscProgram implements api.Program { const handlers = [ new BaseDefDecoratorHandler(this.reflector, evaluator, this.isCore), new ComponentDecoratorHandler( - this.reflector, evaluator, metaRegistry, this.metaReader !, scopeRegistry, this.isCore, - this.resourceManager, this.rootDirs, this.options.preserveWhitespaces || false, - this.options.i18nUseExternalIds !== false, this.moduleResolver, this.cycleAnalyzer, - this.refEmitter, this.defaultImportTracker, this.incrementalState), + this.reflector, evaluator, metaRegistry, this.metaReader !, scopeReader, scopeRegistry, + this.isCore, this.resourceManager, this.rootDirs, + this.options.preserveWhitespaces || false, this.options.i18nUseExternalIds !== false, + this.moduleResolver, this.cycleAnalyzer, this.refEmitter, this.defaultImportTracker, + this.incrementalState), new DirectiveDecoratorHandler( this.reflector, evaluator, metaRegistry, this.defaultImportTracker, this.isCore), new InjectableDecoratorHandler( diff --git a/packages/compiler-cli/src/ngtsc/scope/index.ts b/packages/compiler-cli/src/ngtsc/scope/index.ts index ddebbe51de1e2..e4b1d4595d785 100644 --- a/packages/compiler-cli/src/ngtsc/scope/index.ts +++ b/packages/compiler-cli/src/ngtsc/scope/index.ts @@ -7,5 +7,6 @@ */ export {ExportScope, ScopeData} from './src/api'; +export {ComponentScopeReader, ComponentScopeRegistry, CompoundComponentScopeReader} from './src/component_scope'; export {DtsModuleScopeResolver, MetadataDtsModuleScopeResolver} from './src/dependency'; export {LocalModuleScope, LocalModuleScopeRegistry, LocalNgModuleData} from './src/local'; diff --git a/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts new file mode 100644 index 0000000000000..a262033c793bf --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/scope/src/component_scope.ts @@ -0,0 +1,67 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {ClassDeclaration} from '../../reflection'; +import {LocalModuleScope} from './local'; + +/** + * Register information about the compilation scope of components. + */ +export interface ComponentScopeRegistry { + registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void; + setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void; +} + +/** + * Read information about the compilation scope of components. + */ +export interface ComponentScopeReader { + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null; + getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null; +} + +/** + * A noop registry that doesn't do anything. + * + * This can be used in tests and cases where we don't care about the compilation scopes + * being registered. + */ +export class NoopComponentScopeRegistry implements ComponentScopeRegistry { + registerComponentScope(clazz: ClassDeclaration, scope: LocalModuleScope): void {} + setComponentAsRequiringRemoteScoping(clazz: ClassDeclaration): void {} +} + +/** + * A `ComponentScopeReader` that reads from an ordered set of child readers until it obtains the + * requested scope. + * + * This is used to combine `ComponentScopeReader`s that read from different sources (e.g. from a + * registry and from the incremental state). + */ +export class CompoundComponentScopeReader implements ComponentScopeReader { + constructor(private readers: ComponentScopeReader[]) {} + + getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { + for (const reader of this.readers) { + const meta = reader.getScopeForComponent(clazz); + if (meta !== null) { + return meta; + } + } + return null; + } + + getRequiresRemoteScope(clazz: ClassDeclaration): boolean|null { + for (const reader of this.readers) { + const requiredScoping = reader.getRequiresRemoteScope(clazz); + if (requiredScoping !== null) { + return requiredScoping; + } + } + return null; + } +} diff --git a/packages/compiler-cli/src/ngtsc/scope/src/local.ts b/packages/compiler-cli/src/ngtsc/scope/src/local.ts index 1d7a95e21f3aa..b29c0ff40a1c4 100644 --- a/packages/compiler-cli/src/ngtsc/scope/src/local.ts +++ b/packages/compiler-cli/src/ngtsc/scope/src/local.ts @@ -11,11 +11,12 @@ import * as ts from 'typescript'; import {ErrorCode, makeDiagnostic} from '../../diagnostics'; import {AliasGenerator, Reexport, Reference, ReferenceEmitter} from '../../imports'; -import {DirectiveMeta, LocalMetadataRegistry, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata'; +import {DirectiveMeta, MetadataReader, MetadataRegistry, NgModuleMeta, PipeMeta} from '../../metadata'; import {ClassDeclaration} from '../../reflection'; import {identifierOfNode, nodeNameForError} from '../../util/src/typescript'; import {ExportScope, ScopeData} from './api'; +import {ComponentScopeReader, ComponentScopeRegistry, NoopComponentScopeRegistry} from './component_scope'; import {DtsModuleScopeResolver} from './dependency'; export interface LocalNgModuleData { @@ -58,7 +59,7 @@ export interface CompilationScope extends ScopeData { * The `LocalModuleScopeRegistry` is also capable of producing `ts.Diagnostic` errors when Angular * semantics are violated. */ -export class LocalModuleScopeRegistry implements MetadataRegistry { +export class LocalModuleScopeRegistry implements MetadataRegistry, ComponentScopeReader { /** * Tracks whether the registry has been asked to produce scopes for a module or component. Once * this is true, the registry cannot accept registrations of new directives/pipes/modules as it @@ -102,7 +103,8 @@ export class LocalModuleScopeRegistry implements MetadataRegistry { constructor( private localReader: MetadataReader, private dependencyScopeReader: DtsModuleScopeResolver, - private refEmitter: ReferenceEmitter, private aliasGenerator: AliasGenerator|null) {} + private refEmitter: ReferenceEmitter, private aliasGenerator: AliasGenerator|null, + private componentScopeRegistry: ComponentScopeRegistry = new NoopComponentScopeRegistry()) {} /** * Add an NgModule's data to the registry. @@ -120,10 +122,13 @@ export class LocalModuleScopeRegistry implements MetadataRegistry { registerPipeMetadata(pipe: PipeMeta): void {} getScopeForComponent(clazz: ClassDeclaration): LocalModuleScope|null { - if (!this.declarationToModule.has(clazz)) { - return null; + const scope = !this.declarationToModule.has(clazz) ? + null : + this.getScopeOfModule(this.declarationToModule.get(clazz) !); + if (scope !== null) { + this.componentScopeRegistry.registerComponentScope(clazz, scope); } - return this.getScopeOfModule(this.declarationToModule.get(clazz) !); + return scope; } /** @@ -383,6 +388,7 @@ export class LocalModuleScopeRegistry implements MetadataRegistry { */ setComponentAsRequiringRemoteScoping(node: ClassDeclaration): void { this.remoteScoping.add(node); + this.componentScopeRegistry.setComponentAsRequiringRemoteScoping(node); } /** diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts index 0ed8697d948bb..b84b7c2b6cc39 100644 --- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts +++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts @@ -195,6 +195,25 @@ runInEachFileSystem(() => { expect(written).toContain('/foo_module.js'); }); + it('should rebuild only a Component (but with the correct CompilationScope) if its template has changed', + () => { + setupFooBarProgram(env); + + // Make a change to the template of BarComponent. + env.write('bar_component.html', '
changed
'); + + env.driveMain(); + const written = env.getFilesWrittenSinceLastFlush(); + expect(written).not.toContain('/bar_directive.js'); + expect(written).toContain('/bar_component.js'); + expect(written).not.toContain('/bar_module.js'); + expect(written).not.toContain('/foo_component.js'); + expect(written).not.toContain('/foo_pipe.js'); + expect(written).not.toContain('/foo_module.js'); + // Ensure that the used directives are included in the component's generated template. + expect(env.getContents('/built/bar_component.js')).toMatch(/directives:\s*\[.+\.BarDir\]/); + }); + it('should rebuild everything if a typings file changes', () => { setupFooBarProgram(env); @@ -280,9 +299,10 @@ runInEachFileSystem(() => { env.write('bar_component.ts', ` import {Component} from '@angular/core'; - @Component({selector: 'bar', template: 'bar'}) + @Component({selector: 'bar', templateUrl: './bar_component.html'}) export class BarCmp {} `); + env.write('bar_component.html', '
'); env.write('bar_directive.ts', ` import {Directive} from '@angular/core';