From b0868915aea3cef2e650bd4e5337c5d7fa673537 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 29 Sep 2017 14:55:44 -0700 Subject: [PATCH 1/7] =?UTF-8?q?perf(compiler):=20don=E2=80=99t=20emit=20su?= =?UTF-8?q?mmaries=20for=20jit=20by=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This re-adds the flag `enableSummariesForJit` to the compiler options that already existed in Angular 4. --- packages/bazel/src/ng_module.bzl | 1 + .../integrationtest/tsconfig-xi18n.json | 3 +- packages/compiler-cli/src/transformers/api.ts | 6 +++ .../compiler-cli/src/transformers/program.ts | 2 +- packages/compiler-cli/test/ngc_spec.ts | 44 ++++++++++++++----- .../test/transformers/program_spec.ts | 15 ++++--- packages/compiler/src/aot/compiler.ts | 16 ++++--- packages/compiler/src/aot/compiler_options.ts | 1 - .../compiler/src/aot/summary_serializer.ts | 24 +++++----- 9 files changed, 76 insertions(+), 36 deletions(-) diff --git a/packages/bazel/src/ng_module.bzl b/packages/bazel/src/ng_module.bzl index fceadb3a7d..68d008a980 100644 --- a/packages/bazel/src/ng_module.bzl +++ b/packages/bazel/src/ng_module.bzl @@ -71,6 +71,7 @@ def _ngc_tsconfig(ctx, files, srcs, **kwargs): "angularCompilerOptions": { "generateCodeForLibraries": False, "allowEmptyCodegenFiles": True, + "enableSummariesforJit": True, # FIXME: wrong place to de-dupe "expectedOut": depset([o.path for o in expected_outs]).to_list() } diff --git a/packages/compiler-cli/integrationtest/tsconfig-xi18n.json b/packages/compiler-cli/integrationtest/tsconfig-xi18n.json index 50af710615..b99e0f28b5 100644 --- a/packages/compiler-cli/integrationtest/tsconfig-xi18n.json +++ b/packages/compiler-cli/integrationtest/tsconfig-xi18n.json @@ -3,7 +3,8 @@ // For TypeScript 1.8, we have to lay out generated files // in the same source directory with your code. "genDir": ".", - "debug": true + "debug": true, + "enableSummariesForJit": true }, "compilerOptions": { diff --git a/packages/compiler-cli/src/transformers/api.ts b/packages/compiler-cli/src/transformers/api.ts index 7829f956f0..54bb786602 100644 --- a/packages/compiler-cli/src/transformers/api.ts +++ b/packages/compiler-cli/src/transformers/api.ts @@ -144,6 +144,12 @@ export interface CompilerOptions extends ts.CompilerOptions { /** generate all possible generated files */ allowEmptyCodegenFiles?: boolean; + + /** + * Whether to generate .ngsummary.ts files that allow to use AOTed artifacts + * in JIT mode. This is off by default. + */ + enableSummariesForJit?: boolean; } export interface CompilerHost extends ts.CompilerHost { diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index e24c4209ae..54761e238a 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -533,7 +533,7 @@ function getAotCompilerOptions(options: CompilerOptions): AotCompilerOptions { locale: options.i18nInLocale, i18nFormat: options.i18nInFormat || options.i18nOutFormat, translations, missingTranslation, enableLegacyTemplate: options.enableLegacyTemplate, - enableSummariesForJit: true, + enableSummariesForJit: options.enableSummariesForJit, preserveWhitespaces: options.preserveWhitespaces, fullTemplateTypeCheck: options.fullTemplateTypeCheck, allowEmptyCodegenFiles: options.allowEmptyCodegenFiles, diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts index 61cd86d2f6..e787aeacf9 100644 --- a/packages/compiler-cli/test/ngc_spec.ts +++ b/packages/compiler-cli/test/ngc_spec.ts @@ -336,18 +336,21 @@ describe('ngc transformer command-line', () => { }); } - function expectAllGeneratedFilesToExist() { + function expectAllGeneratedFilesToExist(enableSummariesForJit = true) { modules.forEach(moduleName => { if (/module|comp/.test(moduleName)) { shouldExist(moduleName + '.ngfactory.js'); shouldExist(moduleName + '.ngfactory.d.ts'); - shouldExist(moduleName + '.ngsummary.js'); - shouldExist(moduleName + '.ngsummary.d.ts'); } else { shouldNotExist(moduleName + '.ngfactory.js'); shouldNotExist(moduleName + '.ngfactory.d.ts'); + } + if (enableSummariesForJit) { shouldExist(moduleName + '.ngsummary.js'); shouldExist(moduleName + '.ngsummary.d.ts'); + } else { + shouldNotExist(moduleName + '.ngsummary.js'); + shouldNotExist(moduleName + '.ngsummary.d.ts'); } shouldExist(moduleName + '.ngsummary.json'); shouldNotExist(moduleName + '.ngfactory.metadata.json'); @@ -359,10 +362,11 @@ describe('ngc transformer command-line', () => { shouldExist('emulated.css.shim.ngstyle.d.ts'); } - it('should emit generated files from sources', () => { + it('should emit generated files from sources with summariesForJit', () => { writeConfig(`{ "extends": "./tsconfig-base.json", "angularCompilerOptions": { + "enableSummariesForJit": true }, "include": ["src/**/*.ts"] }`); @@ -370,7 +374,22 @@ describe('ngc transformer command-line', () => { expect(exitCode).toEqual(0); outDir = path.resolve(basePath, 'built', 'src'); expectJsDtsMetadataJsonToExist(); - expectAllGeneratedFilesToExist(); + expectAllGeneratedFilesToExist(true); + }); + + it('should not emit generated files from sources without summariesForJit', () => { + writeConfig(`{ + "extends": "./tsconfig-base.json", + "angularCompilerOptions": { + "enableSummariesForJit": false + }, + "include": ["src/**/*.ts"] + }`); + const exitCode = main(['-p', path.join(basePath, 'tsconfig.json')], errorSpy); + expect(exitCode).toEqual(0); + outDir = path.resolve(basePath, 'built', 'src'); + expectJsDtsMetadataJsonToExist(); + expectAllGeneratedFilesToExist(false); }); it('should emit generated files from libraries', () => { @@ -408,7 +427,8 @@ describe('ngc transformer command-line', () => { writeConfig(`{ "extends": "./tsconfig-base.json", "angularCompilerOptions": { - "skipTemplateCodegen": false + "skipTemplateCodegen": false, + "enableSummariesForJit": true }, "compilerOptions": { "outDir": "built" @@ -880,7 +900,8 @@ describe('ngc transformer command-line', () => { write('tsconfig-ng.json', `{ "extends": "./tsconfig-base.json", "angularCompilerOptions": { - "generateCodeForLibraries": true + "generateCodeForLibraries": true, + "enableSummariesForJit": true }, "compilerOptions": { "outDir": "." @@ -896,7 +917,8 @@ describe('ngc transformer command-line', () => { write('lib1/tsconfig-lib1.json', `{ "extends": "../tsconfig-base.json", "angularCompilerOptions": { - "generateCodeForLibraries": false + "generateCodeForLibraries": false, + "enableSummariesForJit": true }, "compilerOptions": { "rootDir": ".", @@ -919,7 +941,8 @@ describe('ngc transformer command-line', () => { write('lib2/tsconfig-lib2.json', `{ "extends": "../tsconfig-base.json", "angularCompilerOptions": { - "generateCodeForLibraries": false + "generateCodeForLibraries": false, + "enableSummariesForJit": true }, "compilerOptions": { "rootDir": ".", @@ -941,7 +964,8 @@ describe('ngc transformer command-line', () => { write('app/tsconfig-app.json', `{ "extends": "../tsconfig-base.json", "angularCompilerOptions": { - "generateCodeForLibraries": false + "generateCodeForLibraries": false, + "enableSummariesForJit": true }, "compilerOptions": { "rootDir": ".", diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index 9db67d4aa7..eca0ae9207 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -300,7 +300,10 @@ describe('ng program', () => { export * from './main'; `, }); - const options = testSupport.createCompilerOptions({allowEmptyCodegenFiles: true}); + const options = testSupport.createCompilerOptions({ + allowEmptyCodegenFiles: true, + enableSummariesForJit: true, + }); const host = ng.createCompilerHost({options}); const written = new Map < string, { original: ts.SourceFile[]|undefined; @@ -384,11 +387,11 @@ describe('ng program', () => { testSupport.shouldExist('built/main.ngfactory.js'); testSupport.shouldExist('built/main.ngfactory.d.ts'); testSupport.shouldExist('built/main.ngsummary.json'); - testSupport.shouldNotExist('build/node_modules/lib/index.js'); - testSupport.shouldNotExist('build/node_modules/lib/index.d.ts'); - testSupport.shouldNotExist('build/node_modules/lib/index.ngfactory.js'); - testSupport.shouldNotExist('build/node_modules/lib/index.ngfactory.d.ts'); - testSupport.shouldNotExist('build/node_modules/lib/index.ngsummary.json'); + testSupport.shouldNotExist('built/node_modules/lib/index.js'); + testSupport.shouldNotExist('built/node_modules/lib/index.d.ts'); + testSupport.shouldNotExist('built/node_modules/lib/index.ngfactory.js'); + testSupport.shouldNotExist('built/node_modules/lib/index.ngfactory.d.ts'); + testSupport.shouldNotExist('built/node_modules/lib/index.ngsummary.json'); }); describe('createSrcToOutPathMapper', () => { diff --git a/packages/compiler/src/aot/compiler.ts b/packages/compiler/src/aot/compiler.ts index 2f1f7017cd..4da4cc9208 100644 --- a/packages/compiler/src/aot/compiler.ts +++ b/packages/compiler/src/aot/compiler.ts @@ -98,7 +98,9 @@ export class AotCompiler { if (this._options.allowEmptyCodegenFiles || file.directives.length || file.pipes.length || file.injectables.length || file.ngModules.length || file.exportsNonSourceFiles) { genFileNames.push(ngfactoryFilePath(file.fileName, true)); - genFileNames.push(summaryForJitFileName(file.fileName, true)); + if (this._options.enableSummariesForJit) { + genFileNames.push(summaryForJitFileName(file.fileName, true)); + } } const fileSuffix = splitTypescriptSuffix(file.fileName, true)[1]; file.directives.forEach((dirSymbol) => { @@ -376,7 +378,9 @@ export class AotCompiler { metadata: this._metadataResolver.getInjectableSummary(ref) !.type })) ]; - const forJitOutputCtx = this._createOutputContext(summaryForJitFileName(srcFileName, true)); + const forJitOutputCtx = this._options.enableSummariesForJit ? + this._createOutputContext(summaryForJitFileName(srcFileName, true)) : + null; const {json, exportAs} = serializeSummaries( srcFileName, forJitOutputCtx, this._summaryResolver, this._symbolResolver, symbolSummaries, typeData); @@ -387,11 +391,11 @@ export class AotCompiler { ])); }); const summaryJson = new GeneratedFile(srcFileName, summaryFileName(srcFileName), json); - if (this._options.enableSummariesForJit) { - return [summaryJson, this._codegenSourceModule(srcFileName, forJitOutputCtx)]; + const result = [summaryJson]; + if (forJitOutputCtx) { + result.push(this._codegenSourceModule(srcFileName, forJitOutputCtx)); } - - return [summaryJson]; + return result; } private _compileModule(outputCtx: OutputContext, ngModule: CompileNgModuleMetadata): void { diff --git a/packages/compiler/src/aot/compiler_options.ts b/packages/compiler/src/aot/compiler_options.ts index 1149a4cddf..b7bd69d320 100644 --- a/packages/compiler/src/aot/compiler_options.ts +++ b/packages/compiler/src/aot/compiler_options.ts @@ -14,7 +14,6 @@ export interface AotCompilerOptions { translations?: string; missingTranslation?: MissingTranslationStrategy; enableLegacyTemplate?: boolean; - /** TODO(tbosch): remove this flag as it is always on in the new ngc */ enableSummariesForJit?: boolean; preserveWhitespaces?: boolean; fullTemplateTypeCheck?: boolean; diff --git a/packages/compiler/src/aot/summary_serializer.ts b/packages/compiler/src/aot/summary_serializer.ts index 43de1da9bf..765b06d38d 100644 --- a/packages/compiler/src/aot/summary_serializer.ts +++ b/packages/compiler/src/aot/summary_serializer.ts @@ -15,14 +15,14 @@ import {ResolvedStaticSymbol, StaticSymbolResolver} from './static_symbol_resolv import {summaryForJitFileName, summaryForJitName} from './util'; export function serializeSummaries( - srcFileName: string, forJitCtx: OutputContext, summaryResolver: SummaryResolver, - symbolResolver: StaticSymbolResolver, symbols: ResolvedStaticSymbol[], types: { + srcFileName: string, forJitCtx: OutputContext | null, + summaryResolver: SummaryResolver, symbolResolver: StaticSymbolResolver, + symbols: ResolvedStaticSymbol[], types: { summary: CompileTypeSummary, metadata: CompileNgModuleMetadata | CompileDirectiveMetadata | CompilePipeMetadata | CompileTypeMetadata }[]): {json: string, exportAs: {symbol: StaticSymbol, exportAs: string}[]} { const toJsonSerializer = new ToJsonSerializer(symbolResolver, summaryResolver, srcFileName); - const forJitSerializer = new ForJitSerializer(forJitCtx, symbolResolver); // for symbols, we use everything except for the class metadata itself // (we keep the statics though), as the class metadata is contained in the @@ -33,18 +33,20 @@ export function serializeSummaries( // Add type summaries. types.forEach(({summary, metadata}) => { - forJitSerializer.addSourceType(summary, metadata); toJsonSerializer.addSummary( {symbol: summary.type.reference, metadata: undefined, type: summary}); }); - toJsonSerializer.unprocessedSymbolSummariesBySymbol.forEach((summary) => { - if (summaryResolver.isLibraryFile(summary.symbol.filePath) && summary.type) { - forJitSerializer.addLibType(summary.type); - } - }); - const {json, exportAs} = toJsonSerializer.serialize(); - forJitSerializer.serialize(exportAs); + if (forJitCtx) { + const forJitSerializer = new ForJitSerializer(forJitCtx, symbolResolver); + types.forEach(({summary, metadata}) => { forJitSerializer.addSourceType(summary, metadata); }); + toJsonSerializer.unprocessedSymbolSummariesBySymbol.forEach((summary) => { + if (summaryResolver.isLibraryFile(summary.symbol.filePath) && summary.type) { + forJitSerializer.addLibType(summary.type); + } + }); + forJitSerializer.serialize(exportAs); + } return {json, exportAs}; } From 745b59f49cacb55c54d11e948c011fbbf8b724f7 Mon Sep 17 00:00:00 2001 From: Tobias Bosch Date: Fri, 29 Sep 2017 15:02:11 -0700 Subject: [PATCH 2/7] perf(compiler): only emit changed files for incremental compilation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For now, we always create all generated files, but diff them before we pass them to TypeScript. For the user files, we compare the programs and only emit changed TypeScript files. This also adds more diagnostic messages if the `—diagnostics` flag is passed to the command line. --- packages/compiler-cli/src/main.ts | 7 +- packages/compiler-cli/src/ngtools_api2.ts | 11 +- packages/compiler-cli/src/perform_compile.ts | 13 +- packages/compiler-cli/src/perform_watch.ts | 30 +--- packages/compiler-cli/src/transformers/api.ts | 14 +- .../src/transformers/compiler_host.ts | 4 +- .../transformers/node_emitter_transform.ts | 9 +- .../compiler-cli/src/transformers/program.ts | 148 ++++++++++++++--- .../compiler-cli/src/transformers/util.ts | 15 +- .../compiler-cli/test/perform_watch_spec.ts | 6 +- packages/compiler-cli/test/test_support.ts | 5 +- .../test/transformers/compiler_host_spec.ts | 2 +- .../test/transformers/program_spec.ts | 80 +++++++-- packages/compiler/src/aot/generated_file.ts | 17 +- packages/compiler/src/output/output_ast.ts | 157 +++++++++++++++++- 15 files changed, 424 insertions(+), 94 deletions(-) diff --git a/packages/compiler-cli/src/main.ts b/packages/compiler-cli/src/main.ts index 626904b7b8..b34cf29596 100644 --- a/packages/compiler-cli/src/main.ts +++ b/packages/compiler-cli/src/main.ts @@ -18,7 +18,7 @@ import * as api from './transformers/api'; import * as ngc from './transformers/entry_points'; import {GENERATED_FILES} from './transformers/util'; -import {exitCodeFromResult, performCompilation, readConfiguration, formatDiagnostics, Diagnostics, ParsedConfiguration, PerformCompilationResult} from './perform_compile'; +import {exitCodeFromResult, performCompilation, readConfiguration, formatDiagnostics, Diagnostics, ParsedConfiguration, PerformCompilationResult, filterErrorsAndWarnings} from './perform_compile'; import {performWatchCompilation, createPerformWatchHost} from './perform_watch'; import {isSyntaxError} from '@angular/compiler'; @@ -130,8 +130,9 @@ export function readCommandLineAndConfiguration( function reportErrorsAndExit( options: api.CompilerOptions, allDiagnostics: Diagnostics, consoleError: (s: string) => void = console.error): number { - if (allDiagnostics.length) { - consoleError(formatDiagnostics(options, allDiagnostics)); + const errorsAndWarnings = filterErrorsAndWarnings(allDiagnostics); + if (errorsAndWarnings.length) { + consoleError(formatDiagnostics(options, errorsAndWarnings)); } return exitCodeFromResult(allDiagnostics); } diff --git a/packages/compiler-cli/src/ngtools_api2.ts b/packages/compiler-cli/src/ngtools_api2.ts index 9c0ab6584b..d4c1c0ab2f 100644 --- a/packages/compiler-cli/src/ngtools_api2.ts +++ b/packages/compiler-cli/src/ngtools_api2.ts @@ -19,9 +19,11 @@ import {ParseSourceSpan} from '@angular/compiler'; import * as ts from 'typescript'; import {formatDiagnostics as formatDiagnosticsOrig} from './perform_compile'; +import {Program as ProgramOrig} from './transformers/api'; import {createCompilerHost as createCompilerOrig} from './transformers/compiler_host'; import {createProgram as createProgramOrig} from './transformers/program'; + // Interfaces from ./transformers/api; export interface Diagnostic { messageText: string; @@ -92,12 +94,6 @@ export interface TsEmitArguments { export interface TsEmitCallback { (args: TsEmitArguments): ts.EmitResult; } -export interface LibrarySummary { - fileName: string; - text: string; - sourceFile?: ts.SourceFile; -} - export interface Program { getTsProgram(): ts.Program; getTsOptionDiagnostics(cancellationToken?: ts.CancellationToken): ts.Diagnostic[]; @@ -116,7 +112,6 @@ export interface Program { customTransformers?: CustomTransformers, emitCallback?: TsEmitCallback }): ts.EmitResult; - getLibrarySummaries(): LibrarySummary[]; } // Wrapper for createProgram. @@ -124,7 +119,7 @@ export function createProgram( {rootNames, options, host, oldProgram}: {rootNames: string[], options: CompilerOptions, host: CompilerHost, oldProgram?: Program}): Program { - return createProgramOrig({rootNames, options, host, oldProgram}); + return createProgramOrig({rootNames, options, host, oldProgram: oldProgram as ProgramOrig}); } // Wrapper for createCompilerHost. diff --git a/packages/compiler-cli/src/perform_compile.ts b/packages/compiler-cli/src/perform_compile.ts index 18fc69b9eb..0bfcf98fbb 100644 --- a/packages/compiler-cli/src/perform_compile.ts +++ b/packages/compiler-cli/src/perform_compile.ts @@ -13,11 +13,16 @@ import * as ts from 'typescript'; import * as api from './transformers/api'; import * as ng from './transformers/entry_points'; +import {createMessageDiagnostic} from './transformers/util'; const TS_EXT = /\.ts$/; export type Diagnostics = Array; +export function filterErrorsAndWarnings(diagnostics: Diagnostics): Diagnostics { + return diagnostics.filter(d => d.category !== ts.DiagnosticCategory.Message); +} + export function formatDiagnostics(options: api.CompilerOptions, diags: Diagnostics): string { if (diags && diags.length) { const tsFormatHost: ts.FormatDiagnosticsHost = { @@ -123,7 +128,7 @@ export interface PerformCompilationResult { } export function exitCodeFromResult(diags: Diagnostics | undefined): number { - if (!diags || diags.length === 0) { + if (!diags || filterErrorsAndWarnings(diags).length === 0) { // If we have a result and didn't get any errors, we succeeded. return 0; } @@ -154,7 +159,13 @@ export function performCompilation({rootNames, options, host, oldProgram, emitCa program = ng.createProgram({rootNames, host, options, oldProgram}); + const beforeDiags = Date.now(); allDiagnostics.push(...gatherDiagnostics(program !)); + if (options.diagnostics) { + const afterDiags = Date.now(); + allDiagnostics.push( + createMessageDiagnostic(`Time for diagnostics: ${afterDiags - beforeDiags}ms.`)); + } if (!hasErrors(allDiagnostics)) { emitResult = program !.emit({emitCallback, customTransformers, emitFlags}); diff --git a/packages/compiler-cli/src/perform_watch.ts b/packages/compiler-cli/src/perform_watch.ts index c83ae86b21..c5acc570f0 100644 --- a/packages/compiler-cli/src/perform_watch.ts +++ b/packages/compiler-cli/src/perform_watch.ts @@ -13,27 +13,7 @@ import * as ts from 'typescript'; import {Diagnostics, ParsedConfiguration, PerformCompilationResult, exitCodeFromResult, performCompilation, readConfiguration} from './perform_compile'; import * as api from './transformers/api'; import {createCompilerHost} from './transformers/entry_points'; - -const ChangeDiagnostics = { - Compilation_complete_Watching_for_file_changes: { - category: ts.DiagnosticCategory.Message, - messageText: 'Compilation complete. Watching for file changes.', - code: api.DEFAULT_ERROR_CODE, - source: api.SOURCE - }, - Compilation_failed_Watching_for_file_changes: { - category: ts.DiagnosticCategory.Message, - messageText: 'Compilation failed. Watching for file changes.', - code: api.DEFAULT_ERROR_CODE, - source: api.SOURCE - }, - File_change_detected_Starting_incremental_compilation: { - category: ts.DiagnosticCategory.Message, - messageText: 'File change detected. Starting incremental compilation.', - code: api.DEFAULT_ERROR_CODE, - source: api.SOURCE - }, -}; +import {createMessageDiagnostic} from './transformers/util'; function totalCompilationTimeDiagnostic(timeInMillis: number): api.Diagnostic { let duration: string; @@ -231,9 +211,11 @@ export function performWatchCompilation(host: PerformWatchHost): const exitCode = exitCodeFromResult(compileResult.diagnostics); if (exitCode == 0) { cachedProgram = compileResult.program; - host.reportDiagnostics([ChangeDiagnostics.Compilation_complete_Watching_for_file_changes]); + host.reportDiagnostics( + [createMessageDiagnostic('Compilation complete. Watching for file changes.')]); } else { - host.reportDiagnostics([ChangeDiagnostics.Compilation_failed_Watching_for_file_changes]); + host.reportDiagnostics( + [createMessageDiagnostic('Compilation failed. Watching for file changes.')]); } return compileResult.diagnostics; @@ -285,7 +267,7 @@ export function performWatchCompilation(host: PerformWatchHost): function recompile() { timerHandleForRecompilation = undefined; host.reportDiagnostics( - [ChangeDiagnostics.File_change_detected_Starting_incremental_compilation]); + [createMessageDiagnostic('File change detected. Starting incremental compilation.')]); doCompilation(); } } \ No newline at end of file diff --git a/packages/compiler-cli/src/transformers/api.ts b/packages/compiler-cli/src/transformers/api.ts index 54bb786602..d5aff54dc9 100644 --- a/packages/compiler-cli/src/transformers/api.ts +++ b/packages/compiler-cli/src/transformers/api.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ParseSourceSpan} from '@angular/compiler'; +import {GeneratedFile, ParseSourceSpan} from '@angular/compiler'; import * as ts from 'typescript'; export const DEFAULT_ERROR_CODE = 100; @@ -220,6 +220,9 @@ export interface TsEmitArguments { export interface TsEmitCallback { (args: TsEmitArguments): ts.EmitResult; } +/** + * @internal + */ export interface LibrarySummary { fileName: string; text: string; @@ -306,6 +309,13 @@ export interface Program { * Returns the .d.ts / .ngsummary.json / .ngfactory.d.ts files of libraries that have been emitted * in this program or previous programs with paths that emulate the fact that these libraries * have been compiled before with no outDir. + * + * @internal */ - getLibrarySummaries(): LibrarySummary[]; + getLibrarySummaries(): Map; + + /** + * @internal + */ + getEmittedGeneratedFiles(): Map; } diff --git a/packages/compiler-cli/src/transformers/compiler_host.ts b/packages/compiler-cli/src/transformers/compiler_host.ts index 85e7702f4c..acdf9f5c0d 100644 --- a/packages/compiler-cli/src/transformers/compiler_host.ts +++ b/packages/compiler-cli/src/transformers/compiler_host.ts @@ -58,7 +58,6 @@ export class TsCompilerAotCompilerTypeCheckHostAdapter extends private generatedSourceFiles = new Map(); private generatedCodeFor = new Map(); private emitter = new TypeScriptEmitter(); - private librarySummaries = new Map(); getCancellationToken: () => ts.CancellationToken; getDefaultLibLocation: () => string; trace: (s: string) => void; @@ -68,9 +67,8 @@ export class TsCompilerAotCompilerTypeCheckHostAdapter extends constructor( private rootFiles: string[], options: CompilerOptions, context: CompilerHost, private metadataProvider: MetadataProvider, private codeGenerator: CodeGenerator, - librarySummaries: LibrarySummary[]) { + private librarySummaries = new Map()) { super(options, context); - librarySummaries.forEach(summary => this.librarySummaries.set(summary.fileName, summary)); this.moduleResolutionCache = ts.createModuleResolutionCache( this.context.getCurrentDirectory !(), this.context.getCanonicalFileName.bind(this.context)); const basePath = this.options.basePath !; diff --git a/packages/compiler-cli/src/transformers/node_emitter_transform.ts b/packages/compiler-cli/src/transformers/node_emitter_transform.ts index 47b9354ade..0153e90848 100644 --- a/packages/compiler-cli/src/transformers/node_emitter_transform.ts +++ b/packages/compiler-cli/src/transformers/node_emitter_transform.ts @@ -10,6 +10,7 @@ import {GeneratedFile} from '@angular/compiler'; import * as ts from 'typescript'; import {TypeScriptNodeEmitter} from './node_emitter'; +import {GENERATED_FILES} from './util'; const PREAMBLE = `/** * @fileoverview This file is generated by the Angular template compiler. @@ -18,17 +19,17 @@ const PREAMBLE = `/** * tslint:disable */`; -export function getAngularEmitterTransformFactory(generatedFiles: GeneratedFile[]): () => +export function getAngularEmitterTransformFactory(generatedFiles: Map): () => (sourceFile: ts.SourceFile) => ts.SourceFile { return function() { - const map = new Map(generatedFiles.filter(g => g.stmts && g.stmts.length) - .map<[string, GeneratedFile]>(g => [g.genFileUrl, g])); const emitter = new TypeScriptNodeEmitter(); return function(sourceFile: ts.SourceFile): ts.SourceFile { - const g = map.get(sourceFile.fileName); + const g = generatedFiles.get(sourceFile.fileName); if (g && g.stmts) { const [newSourceFile] = emitter.updateSourceFile(sourceFile, g.stmts, PREAMBLE); return newSourceFile; + } else if (GENERATED_FILES.test(sourceFile.fileName)) { + return ts.updateSourceFileNode(sourceFile, []); } return sourceFile; }; diff --git a/packages/compiler-cli/src/transformers/program.ts b/packages/compiler-cli/src/transformers/program.ts index 54761e238a..205a4f40f7 100644 --- a/packages/compiler-cli/src/transformers/program.ts +++ b/packages/compiler-cli/src/transformers/program.ts @@ -18,7 +18,13 @@ import {CompilerHost, CompilerOptions, CustomTransformers, DEFAULT_ERROR_CODE, D import {CodeGenerator, TsCompilerAotCompilerTypeCheckHostAdapter, getOriginalReferences} from './compiler_host'; import {LowerMetadataCache, getExpressionLoweringTransformFactory} from './lower_expressions'; import {getAngularEmitterTransformFactory} from './node_emitter_transform'; -import {GENERATED_FILES, StructureIsReused, tsStructureIsReused} from './util'; +import {GENERATED_FILES, StructureIsReused, createMessageDiagnostic, tsStructureIsReused} from './util'; + +/** + * Maximum number of files that are emitable via calling ts.Program.emit + * passing individual targetSourceFiles. + */ +const MAX_FILE_COUNT_FOR_SINGLE_FILE_EMIT = 20; const emptyModules: NgAnalyzedModules = { ngModules: [], @@ -32,18 +38,20 @@ const defaultEmitCallback: TsEmitCallback = program.emit( targetSourceFile, writeFile, cancellationToken, emitOnlyDtsFiles, customTransformers); - class AngularCompilerProgram implements Program { private metadataCache: LowerMetadataCache; - private oldProgramLibrarySummaries: LibrarySummary[] = []; + private oldProgramLibrarySummaries: Map|undefined; + private oldProgramEmittedGeneratedFiles: Map|undefined; // Note: This will be cleared out as soon as we create the _tsProgram private oldTsProgram: ts.Program|undefined; private emittedLibrarySummaries: LibrarySummary[]|undefined; + private emittedGeneratedFiles: GeneratedFile[]|undefined; // Lazily initialized fields private _typeCheckHost: TypeCheckHost; private _compiler: AotCompiler; private _tsProgram: ts.Program; + private _changedNonGenFileNames: string[]|undefined; private _analyzedModules: NgAnalyzedModules|undefined; private _structuralDiagnostics: Diagnostic[]|undefined; private _programWithStubs: ts.Program|undefined; @@ -60,6 +68,7 @@ class AngularCompilerProgram implements Program { this.oldTsProgram = oldProgram ? oldProgram.getTsProgram() : undefined; if (oldProgram) { this.oldProgramLibrarySummaries = oldProgram.getLibrarySummaries(); + this.oldProgramEmittedGeneratedFiles = oldProgram.getEmittedGeneratedFiles(); } if (options.flatModuleOutFile) { @@ -81,10 +90,26 @@ class AngularCompilerProgram implements Program { this.metadataCache = new LowerMetadataCache({quotedNames: true}, !!options.strictMetadataEmit); } - getLibrarySummaries(): LibrarySummary[] { - const result = [...this.oldProgramLibrarySummaries]; + getLibrarySummaries(): Map { + const result = new Map(); + if (this.oldProgramLibrarySummaries) { + this.oldProgramLibrarySummaries.forEach((summary, fileName) => result.set(fileName, summary)); + } if (this.emittedLibrarySummaries) { - result.push(...this.emittedLibrarySummaries); + this.emittedLibrarySummaries.forEach( + (summary, fileName) => result.set(summary.fileName, summary)); + } + return result; + } + + getEmittedGeneratedFiles(): Map { + const result = new Map(); + if (this.oldProgramEmittedGeneratedFiles) { + this.oldProgramEmittedGeneratedFiles.forEach( + (genFile, fileName) => result.set(fileName, genFile)); + } + if (this.emittedGeneratedFiles) { + this.emittedGeneratedFiles.forEach((genFile) => result.set(genFile.genFileUrl, genFile)); } return result; } @@ -142,6 +167,7 @@ class AngularCompilerProgram implements Program { customTransformers?: CustomTransformers, emitCallback?: TsEmitCallback } = {}): ts.EmitResult { + const emitStart = Date.now(); if (emitFlags & EmitFlags.I18nBundle) { const locale = this.options.i18nOutLocale || null; const file = this.options.i18nOutFile || null; @@ -153,7 +179,7 @@ class AngularCompilerProgram implements Program { 0) { return {emitSkipped: true, diagnostics: [], emittedFiles: []}; } - const {genFiles, genDiags} = this.generateFilesForEmit(emitFlags); + let {genFiles, genDiags} = this.generateFilesForEmit(emitFlags); if (genDiags.length) { return { diagnostics: genDiags, @@ -161,11 +187,11 @@ class AngularCompilerProgram implements Program { emittedFiles: [], }; } - const emittedLibrarySummaries = this.emittedLibrarySummaries = []; - + this.emittedGeneratedFiles = genFiles; const outSrcMapping: Array<{sourceFile: ts.SourceFile, outFileName: string}> = []; const genFileByFileName = new Map(); genFiles.forEach(genFile => genFileByFileName.set(genFile.genFileUrl, genFile)); + this.emittedLibrarySummaries = []; const writeTsFile: ts.WriteFileCallback = (outFileName, outData, writeByteOrderMark, onError?, sourceFiles?) => { const sourceFile = sourceFiles && sourceFiles.length == 1 ? sourceFiles[0] : null; @@ -176,7 +202,8 @@ class AngularCompilerProgram implements Program { } this.writeFile(outFileName, outData, writeByteOrderMark, onError, genFile, sourceFiles); }; - + const tsCustomTansformers = this.calculateTransforms(genFileByFileName, customTransformers); + const emitOnlyDtsFiles = (emitFlags & (EmitFlags.DTS | EmitFlags.JS)) == EmitFlags.DTS; // Restore the original references before we emit so TypeScript doesn't emit // a reference to the .d.ts file. const augmentedReferences = new Map(); @@ -187,16 +214,44 @@ class AngularCompilerProgram implements Program { sourceFile.referencedFiles = originalReferences; } } + const genTsFiles: GeneratedFile[] = []; + const genJsonFiles: GeneratedFile[] = []; + genFiles.forEach(gf => { + if (gf.stmts) { + genTsFiles.push(gf); + } + if (gf.source) { + genJsonFiles.push(gf); + } + }); let emitResult: ts.EmitResult; + let emittedUserTsCount: number; try { - emitResult = emitCallback({ - program: this.tsProgram, - host: this.host, - options: this.options, - writeFile: writeTsFile, - emitOnlyDtsFiles: (emitFlags & (EmitFlags.DTS | EmitFlags.JS)) == EmitFlags.DTS, - customTransformers: this.calculateTransforms(genFiles, customTransformers) - }); + const emitChangedFilesOnly = this._changedNonGenFileNames && + this._changedNonGenFileNames.length < MAX_FILE_COUNT_FOR_SINGLE_FILE_EMIT; + if (emitChangedFilesOnly) { + const fileNamesToEmit = + [...this._changedNonGenFileNames !, ...genTsFiles.map(gf => gf.genFileUrl)]; + emitResult = mergeEmitResults( + fileNamesToEmit.map((fileName) => emitResult = emitCallback({ + program: this.tsProgram, + host: this.host, + options: this.options, + writeFile: writeTsFile, emitOnlyDtsFiles, + customTransformers: tsCustomTansformers, + targetSourceFile: this.tsProgram.getSourceFile(fileName), + }))); + emittedUserTsCount = this._changedNonGenFileNames !.length; + } else { + emitResult = emitCallback({ + program: this.tsProgram, + host: this.host, + options: this.options, + writeFile: writeTsFile, emitOnlyDtsFiles, + customTransformers: tsCustomTansformers + }); + emittedUserTsCount = this.tsProgram.getSourceFiles().length - genTsFiles.length; + } } finally { // Restore the references back to the augmented value to ensure that the // checks that TypeScript makes for project structure reuse will succeed. @@ -207,10 +262,10 @@ class AngularCompilerProgram implements Program { if (!outSrcMapping.length) { // if no files were emitted by TypeScript, also don't emit .json files + emitResult.diagnostics.push(createMessageDiagnostic(`Emitted no files.`)); return emitResult; } - let sampleSrcFileName: string|undefined; let sampleOutFileName: string|undefined; if (outSrcMapping.length) { @@ -220,16 +275,16 @@ class AngularCompilerProgram implements Program { const srcToOutPath = createSrcToOutPathMapper(this.options.outDir, sampleSrcFileName, sampleOutFileName); if (emitFlags & EmitFlags.Codegen) { - genFiles.forEach(gf => { - if (gf.source) { - const outFileName = srcToOutPath(gf.genFileUrl); - this.writeFile(outFileName, gf.source, false, undefined, gf); - } + genJsonFiles.forEach(gf => { + const outFileName = srcToOutPath(gf.genFileUrl); + this.writeFile(outFileName, gf.source !, false, undefined, gf); }); } + let metadataJsonCount = 0; if (emitFlags & EmitFlags.Metadata) { this.tsProgram.getSourceFiles().forEach(sf => { if (!sf.isDeclarationFile && !GENERATED_FILES.test(sf.fileName)) { + metadataJsonCount++; const metadata = this.metadataCache.getMetadata(sf); const metadataText = JSON.stringify([metadata]); const outFileName = srcToOutPath(sf.fileName.replace(/\.ts$/, '.metadata.json')); @@ -237,6 +292,15 @@ class AngularCompilerProgram implements Program { } }); } + const emitEnd = Date.now(); + if (this.options.diagnostics) { + emitResult.diagnostics.push(createMessageDiagnostic([ + `Emitted in ${emitEnd - emitStart}ms`, + `- ${emittedUserTsCount} user ts files`, + `- ${genTsFiles.length} generated ts files`, + `- ${genJsonFiles.length + metadataJsonCount} generated json files`, + ].join('\n'))); + } return emitResult; } @@ -281,8 +345,9 @@ class AngularCompilerProgram implements Program { (this._semanticDiagnostics = this.generateSemanticDiagnostics()); } - private calculateTransforms(genFiles: GeneratedFile[], customTransformers?: CustomTransformers): - ts.CustomTransformers { + private calculateTransforms( + genFiles: Map, + customTransformers?: CustomTransformers): ts.CustomTransformers { const beforeTs: ts.TransformerFactory[] = []; if (!this.options.disableExpressionLowering) { beforeTs.push(getExpressionLoweringTransformFactory(this.metadataCache)); @@ -353,6 +418,16 @@ class AngularCompilerProgram implements Program { sourceFiles.push(sf.fileName); } }); + if (oldTsProgram) { + // TODO(tbosch): if one of the files contains a `const enum` + // always emit all files! + const changedNonGenFileNames = this._changedNonGenFileNames = [] as string[]; + tmpProgram.getSourceFiles().forEach(sf => { + if (!GENERATED_FILES.test(sf.fileName) && oldTsProgram.getSourceFile(sf.fileName) !== sf) { + changedNonGenFileNames.push(sf.fileName); + } + }); + } return {tmpProgram, sourceFiles, hostAdapter, rootNames}; } @@ -418,7 +493,14 @@ class AngularCompilerProgram implements Program { if (!(emitFlags & EmitFlags.Codegen)) { return {genFiles: [], genDiags: []}; } - const genFiles = this.compiler.emitAllImpls(this.analyzedModules); + let genFiles = this.compiler.emitAllImpls(this.analyzedModules); + if (this.oldProgramEmittedGeneratedFiles) { + const oldProgramEmittedGeneratedFiles = this.oldProgramEmittedGeneratedFiles; + genFiles = genFiles.filter(genFile => { + const oldGenFile = oldProgramEmittedGeneratedFiles.get(genFile.genFileUrl); + return !oldGenFile || !genFile.isEquivalent(oldGenFile); + }); + } return {genFiles, genDiags: []}; } catch (e) { // TODO(tbosch): check whether we can actually have syntax errors here, @@ -649,3 +731,15 @@ export function i18nGetExtension(formatName: string): string { throw new Error(`Unsupported format "${formatName}"`); } + +function mergeEmitResults(emitResults: ts.EmitResult[]): ts.EmitResult { + const diagnostics: ts.Diagnostic[] = []; + let emitSkipped = true; + const emittedFiles: string[] = []; + for (const er of emitResults) { + diagnostics.push(...er.diagnostics); + emitSkipped = emitSkipped || er.emitSkipped; + emittedFiles.push(...er.emittedFiles); + } + return {diagnostics, emitSkipped, emittedFiles}; +} diff --git a/packages/compiler-cli/src/transformers/util.ts b/packages/compiler-cli/src/transformers/util.ts index bd2b78b71c..ec774fcde3 100644 --- a/packages/compiler-cli/src/transformers/util.ts +++ b/packages/compiler-cli/src/transformers/util.ts @@ -8,6 +8,8 @@ import * as ts from 'typescript'; +import {DEFAULT_ERROR_CODE, Diagnostic, SOURCE} from './api'; + export const GENERATED_FILES = /(.*?)\.(ngfactory|shim\.ngstyle|ngstyle|ngsummary)\.(js|d\.ts|ts)$/; export const enum StructureIsReused {Not = 0, SafeModules = 1, Completely = 2} @@ -15,4 +17,15 @@ export const enum StructureIsReused {Not = 0, SafeModules = 1, Completely = 2} // Note: This is an internal property in TypeScript. Use it only for assertions and tests. export function tsStructureIsReused(program: ts.Program): StructureIsReused { return (program as any).structureIsReused; -} \ No newline at end of file +} + +export function createMessageDiagnostic(messageText: string): ts.Diagnostic&Diagnostic { + return { + file: undefined, + start: undefined, + length: undefined, + category: ts.DiagnosticCategory.Message, messageText, + code: DEFAULT_ERROR_CODE, + source: SOURCE, + }; +} diff --git a/packages/compiler-cli/test/perform_watch_spec.ts b/packages/compiler-cli/test/perform_watch_spec.ts index 6bac15f01c..ae0184e2c2 100644 --- a/packages/compiler-cli/test/perform_watch_spec.ts +++ b/packages/compiler-cli/test/perform_watch_spec.ts @@ -86,10 +86,9 @@ describe('perform watch', () => { // trigger a single file change // -> all other files should be cached - fs.unlinkSync(mainNgFactory); host.triggerFileChange(FileChangeEvent.Change, utilTsPath); + expectNoDiagnostics(config.options, host.diagnostics); - expect(fs.existsSync(mainNgFactory)).toBe(true); expect(fileExistsSpy !).not.toHaveBeenCalledWith(mainTsPath); expect(fileExistsSpy !).toHaveBeenCalledWith(utilTsPath); expect(getSourceFileSpy !).not.toHaveBeenCalledWith(mainTsPath, ts.ScriptTarget.ES5); @@ -97,11 +96,10 @@ describe('perform watch', () => { // trigger a folder change // -> nothing should be cached - fs.unlinkSync(mainNgFactory); host.triggerFileChange( FileChangeEvent.CreateDeleteDir, path.resolve(testSupport.basePath, 'src')); + expectNoDiagnostics(config.options, host.diagnostics); - expect(fs.existsSync(mainNgFactory)).toBe(true); expect(fileExistsSpy !).toHaveBeenCalledWith(mainTsPath); expect(fileExistsSpy !).toHaveBeenCalledWith(utilTsPath); expect(getSourceFileSpy !).toHaveBeenCalledWith(mainTsPath, ts.ScriptTarget.ES5); diff --git a/packages/compiler-cli/test/test_support.ts b/packages/compiler-cli/test/test_support.ts index 8845e2275d..38dd9e9678 100644 --- a/packages/compiler-cli/test/test_support.ts +++ b/packages/compiler-cli/test/test_support.ts @@ -110,8 +110,9 @@ export function setup(): TestSupport { } export function expectNoDiagnostics(options: ng.CompilerOptions, diags: ng.Diagnostics) { - if (diags.length) { - throw new Error(`Expected no diagnostics: ${ng.formatDiagnostics(options, diags)}`); + const errorDiags = diags.filter(d => d.category !== ts.DiagnosticCategory.Message); + if (errorDiags.length) { + throw new Error(`Expected no diagnostics: ${ng.formatDiagnostics(options, errorDiags)}`); } } diff --git a/packages/compiler-cli/test/transformers/compiler_host_spec.ts b/packages/compiler-cli/test/transformers/compiler_host_spec.ts index e2579a7e53..d597fb8ce2 100644 --- a/packages/compiler-cli/test/transformers/compiler_host_spec.ts +++ b/packages/compiler-cli/test/transformers/compiler_host_spec.ts @@ -51,7 +51,7 @@ describe('NgCompilerHost', () => { } = {}) { return new TsCompilerAotCompilerTypeCheckHostAdapter( ['/tmp/index.ts'], options, ngHost, new MetadataCollector(), codeGenerator, - librarySummaries); + new Map(librarySummaries.map(entry => [entry.fileName, entry] as[string, LibrarySummary]))); } describe('fileNameToModuleName', () => { diff --git a/packages/compiler-cli/test/transformers/program_spec.ts b/packages/compiler-cli/test/transformers/program_spec.ts index eca0ae9207..def4fd1db4 100644 --- a/packages/compiler-cli/test/transformers/program_spec.ts +++ b/packages/compiler-cli/test/transformers/program_spec.ts @@ -57,17 +57,20 @@ describe('ng program', () => { } function compile( - oldProgram?: ng.Program, overrideOptions?: ng.CompilerOptions, - rootNames?: string[]): ng.Program { + oldProgram?: ng.Program, overrideOptions?: ng.CompilerOptions, rootNames?: string[], + host?: CompilerHost): ng.Program { const options = testSupport.createCompilerOptions(overrideOptions); if (!rootNames) { rootNames = [path.resolve(testSupport.basePath, 'src/index.ts')]; } - + if (!host) { + host = ng.createCompilerHost({options}); + } const program = ng.createProgram({ rootNames: rootNames, options, - host: ng.createCompilerHost({options}), oldProgram, + host, + oldProgram, }); expectNoDiagnosticsInProgram(options, program); program.emit(); @@ -153,6 +156,59 @@ describe('ng program', () => { .toBe(false); }); + it('should only emit changed files', () => { + testSupport.writeFiles({ + 'src/index.ts': createModuleAndCompSource('comp', 'index.html'), + 'src/index.html': `Start` + }); + const options: ng.CompilerOptions = {declaration: false}; + const host = ng.createCompilerHost({options}); + const originalGetSourceFile = host.getSourceFile; + const fileCache = new Map(); + host.getSourceFile = (fileName: string) => { + if (fileCache.has(fileName)) { + return fileCache.get(fileName); + } + const sf = originalGetSourceFile.call(host, fileName); + fileCache.set(fileName, sf); + return sf; + }; + + const written = new Map(); + host.writeFile = (fileName: string, data: string) => written.set(fileName, data); + + // compile libraries + const p1 = compile(undefined, options, undefined, host); + + // first compile without libraries + const p2 = compile(p1, options, undefined, host); + expect(written.has(path.resolve(testSupport.basePath, 'built/src/index.js'))).toBe(true); + let ngFactoryContent = + written.get(path.resolve(testSupport.basePath, 'built/src/index.ngfactory.js')); + expect(ngFactoryContent).toMatch(/Start/); + + // no change -> no emit + written.clear(); + const p3 = compile(p2, options, undefined, host); + expect(written.size).toBe(0); + + // change a user file + written.clear(); + fileCache.delete(path.resolve(testSupport.basePath, 'src/index.ts')); + const p4 = compile(p3, options, undefined, host); + expect(written.size).toBe(1); + expect(written.has(path.resolve(testSupport.basePath, 'built/src/index.js'))).toBe(true); + + // change a file that is input to generated files + written.clear(); + testSupport.writeFiles({'src/index.html': 'Hello'}); + const p5 = compile(p4, options, undefined, host); + expect(written.size).toBe(1); + ngFactoryContent = + written.get(path.resolve(testSupport.basePath, 'built/src/index.ngfactory.js')); + expect(ngFactoryContent).toMatch(/Hello/); + }); + it('should store library summaries on emit', () => { compileLib('lib'); testSupport.writeFiles({ @@ -163,17 +219,19 @@ describe('ng program', () => { ` }); const p1 = compile(); - expect(p1.getLibrarySummaries().some( - sf => /node_modules\/lib\/index\.ngfactory\.d\.ts$/.test(sf.fileName))) + expect(Array.from(p1.getLibrarySummaries().values()) + .some(sf => /node_modules\/lib\/index\.ngfactory\.d\.ts$/.test(sf.fileName))) .toBe(true); - expect(p1.getLibrarySummaries().some( - sf => /node_modules\/lib\/index\.ngsummary\.json$/.test(sf.fileName))) + expect(Array.from(p1.getLibrarySummaries().values()) + .some(sf => /node_modules\/lib\/index\.ngsummary\.json$/.test(sf.fileName))) .toBe(true); - expect( - p1.getLibrarySummaries().some(sf => /node_modules\/lib\/index\.d\.ts$/.test(sf.fileName))) + expect(Array.from(p1.getLibrarySummaries().values()) + .some(sf => /node_modules\/lib\/index\.d\.ts$/.test(sf.fileName))) .toBe(true); - expect(p1.getLibrarySummaries().some(sf => /src\/main.*$/.test(sf.fileName))).toBe(false); + expect(Array.from(p1.getLibrarySummaries().values()) + .some(sf => /src\/main.*$/.test(sf.fileName))) + .toBe(false); }); it('should reuse the old ts program completely if nothing changed', () => { diff --git a/packages/compiler/src/aot/generated_file.ts b/packages/compiler/src/aot/generated_file.ts index 226126f4f4..c072e57a90 100644 --- a/packages/compiler/src/aot/generated_file.ts +++ b/packages/compiler/src/aot/generated_file.ts @@ -7,7 +7,7 @@ */ import {sourceUrl} from '../compile_metadata'; -import {Statement} from '../output/output_ast'; +import {Statement, areAllEquivalent} from '../output/output_ast'; import {TypeScriptEmitter} from '../output/ts_emitter'; export class GeneratedFile { @@ -24,6 +24,21 @@ export class GeneratedFile { this.stmts = sourceOrStmts; } } + + isEquivalent(other: GeneratedFile): boolean { + if (this.genFileUrl !== other.genFileUrl) { + return false; + } + if (this.source) { + return this.source === other.source; + } + if (other.stmts == null) { + return false; + } + // Note: the constructor guarantees that if this.source is not filled, + // then this.stmts is. + return areAllEquivalent(this.stmts !, other.stmts !); + } } export function toTypeScript(file: GeneratedFile, preamble: string = ''): string { diff --git a/packages/compiler/src/output/output_ast.ts b/packages/compiler/src/output/output_ast.ts index afa3567f19..ab0e561d56 100644 --- a/packages/compiler/src/output/output_ast.ts +++ b/packages/compiler/src/output/output_ast.ts @@ -104,6 +104,27 @@ export enum BinaryOperator { BiggerEquals } +export function nullSafeIsEquivalent( + base: T | null, other: T | null) { + if (base == null || other == null) { + return base == other; + } + return base.isEquivalent(other); +} + +export function areAllEquivalent( + base: T[], other: T[]) { + const len = base.length; + if (len !== other.length) { + return false; + } + for (let i = 0; i < len; i++) { + if (!base[i].isEquivalent(other[i])) { + return false; + } + } + return true; +} export abstract class Expression { public type: Type|null; @@ -116,6 +137,12 @@ export abstract class Expression { abstract visitExpression(visitor: ExpressionVisitor, context: any): any; + /** + * Calculates whether this expression produces the same value as the given expression. + * Note: We don't check Types nor ParseSourceSpans nor function arguments. + */ + abstract isEquivalent(e: Expression): boolean; + prop(name: string, sourceSpan?: ParseSourceSpan|null): ReadPropExpr { return new ReadPropExpr(this, name, null, sourceSpan); } @@ -222,6 +249,10 @@ export class ReadVarExpr extends Expression { this.builtin = name; } } + isEquivalent(e: Expression): boolean { + return e instanceof ReadVarExpr && this.name === e.name && this.builtin === e.builtin; + } + visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitReadVarExpr(this, context); } @@ -242,6 +273,9 @@ export class WriteVarExpr extends Expression { super(type || value.type, sourceSpan); this.value = value; } + isEquivalent(e: Expression): boolean { + return e instanceof WriteVarExpr && this.name === e.name && this.value.isEquivalent(e.value); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitWriteVarExpr(this, context); @@ -261,6 +295,10 @@ export class WriteKeyExpr extends Expression { super(type || value.type, sourceSpan); this.value = value; } + isEquivalent(e: Expression): boolean { + return e instanceof WriteKeyExpr && this.receiver.isEquivalent(e.receiver) && + this.index.isEquivalent(e.index) && this.value.isEquivalent(e.value); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitWriteKeyExpr(this, context); } @@ -275,6 +313,10 @@ export class WritePropExpr extends Expression { super(type || value.type, sourceSpan); this.value = value; } + isEquivalent(e: Expression): boolean { + return e instanceof WritePropExpr && this.receiver.isEquivalent(e.receiver) && + this.name === e.name && this.value.isEquivalent(e.value); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitWritePropExpr(this, context); } @@ -301,6 +343,10 @@ export class InvokeMethodExpr extends Expression { this.builtin = method; } } + isEquivalent(e: Expression): boolean { + return e instanceof InvokeMethodExpr && this.receiver.isEquivalent(e.receiver) && + this.name === e.name && this.builtin === e.builtin && areAllEquivalent(this.args, e.args); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitInvokeMethodExpr(this, context); } @@ -313,6 +359,10 @@ export class InvokeFunctionExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof InvokeFunctionExpr && this.fn.isEquivalent(e.fn) && + areAllEquivalent(this.args, e.args); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitInvokeFunctionExpr(this, context); } @@ -325,6 +375,10 @@ export class InstantiateExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof InstantiateExpr && this.classExpr.isEquivalent(e.classExpr) && + areAllEquivalent(this.args, e.args); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitInstantiateExpr(this, context); } @@ -332,9 +386,14 @@ export class InstantiateExpr extends Expression { export class LiteralExpr extends Expression { - constructor(public value: any, type?: Type|null, sourceSpan?: ParseSourceSpan|null) { + constructor( + public value: number|string|boolean|null|undefined, type?: Type|null, + sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof LiteralExpr && this.value === e.value; + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitLiteralExpr(this, context); } @@ -347,6 +406,10 @@ export class ExternalExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof ExternalExpr && this.value.name === e.value.name && + this.value.moduleName === e.value.moduleName && this.value.runtime === e.value.runtime; + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitExternalExpr(this, context); } @@ -355,6 +418,7 @@ export class ExternalExpr extends Expression { export class ExternalReference { constructor(public moduleName: string|null, public name: string|null, public runtime?: any|null) { } + // Note: no isEquivalent method here as we use this as an interface too. } export class ConditionalExpr extends Expression { @@ -365,6 +429,10 @@ export class ConditionalExpr extends Expression { super(type || trueCase.type, sourceSpan); this.trueCase = trueCase; } + isEquivalent(e: Expression): boolean { + return e instanceof ConditionalExpr && this.condition.isEquivalent(e.condition) && + this.trueCase.isEquivalent(e.trueCase) && nullSafeIsEquivalent(this.falseCase, e.falseCase); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitConditionalExpr(this, context); } @@ -375,6 +443,9 @@ export class NotExpr extends Expression { constructor(public condition: Expression, sourceSpan?: ParseSourceSpan|null) { super(BOOL_TYPE, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof NotExpr && this.condition.isEquivalent(e.condition); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitNotExpr(this, context); } @@ -384,6 +455,9 @@ export class AssertNotNull extends Expression { constructor(public condition: Expression, sourceSpan?: ParseSourceSpan|null) { super(condition.type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof AssertNotNull && this.condition.isEquivalent(e.condition); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitAssertNotNullExpr(this, context); } @@ -393,6 +467,9 @@ export class CastExpr extends Expression { constructor(public value: Expression, type?: Type|null, sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof CastExpr && this.value.isEquivalent(e.value); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitCastExpr(this, context); } @@ -401,6 +478,8 @@ export class CastExpr extends Expression { export class FnParam { constructor(public name: string, public type: Type|null = null) {} + + isEquivalent(param: FnParam): boolean { return this.name === param.name; } } @@ -410,6 +489,10 @@ export class FunctionExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof FunctionExpr && areAllEquivalent(this.params, e.params) && + areAllEquivalent(this.statements, e.statements); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitFunctionExpr(this, context); } @@ -429,6 +512,10 @@ export class BinaryOperatorExpr extends Expression { super(type || lhs.type, sourceSpan); this.lhs = lhs; } + isEquivalent(e: Expression): boolean { + return e instanceof BinaryOperatorExpr && this.operator === e.operator && + this.lhs.isEquivalent(e.lhs) && this.rhs.isEquivalent(e.rhs); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitBinaryOperatorExpr(this, context); } @@ -441,6 +528,10 @@ export class ReadPropExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof ReadPropExpr && this.receiver.isEquivalent(e.receiver) && + this.name === e.name; + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitReadPropExpr(this, context); } @@ -456,6 +547,10 @@ export class ReadKeyExpr extends Expression { sourceSpan?: ParseSourceSpan|null) { super(type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof ReadKeyExpr && this.receiver.isEquivalent(e.receiver) && + this.index.isEquivalent(e.index); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitReadKeyExpr(this, context); } @@ -471,6 +566,9 @@ export class LiteralArrayExpr extends Expression { super(type, sourceSpan); this.entries = entries; } + isEquivalent(e: Expression): boolean { + return e instanceof LiteralArrayExpr && areAllEquivalent(this.entries, e.entries); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitLiteralArrayExpr(this, context); } @@ -478,6 +576,9 @@ export class LiteralArrayExpr extends Expression { export class LiteralMapEntry { constructor(public key: string, public value: Expression, public quoted: boolean) {} + isEquivalent(e: LiteralMapEntry): boolean { + return this.key === e.key && this.value.isEquivalent(e.value); + } } export class LiteralMapExpr extends Expression { @@ -489,6 +590,9 @@ export class LiteralMapExpr extends Expression { this.valueType = type.valueType; } } + isEquivalent(e: Expression): boolean { + return e instanceof LiteralMapExpr && areAllEquivalent(this.entries, e.entries); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitLiteralMapExpr(this, context); } @@ -498,6 +602,9 @@ export class CommaExpr extends Expression { constructor(public parts: Expression[], sourceSpan?: ParseSourceSpan|null) { super(parts[parts.length - 1].type, sourceSpan); } + isEquivalent(e: Expression): boolean { + return e instanceof CommaExpr && areAllEquivalent(this.parts, e.parts); + } visitExpression(visitor: ExpressionVisitor, context: any): any { return visitor.visitCommaExpr(this, context); } @@ -547,6 +654,11 @@ export abstract class Statement { this.modifiers = modifiers || []; this.sourceSpan = sourceSpan || null; } + /** + * Calculates whether this statement produces the same value as the given statement. + * Note: We don't check Types nor ParseSourceSpans nor function arguments. + */ + abstract isEquivalent(stmt: Statement): boolean; abstract visitStatement(visitor: StatementVisitor, context: any): any; @@ -562,7 +674,10 @@ export class DeclareVarStmt extends Statement { super(modifiers, sourceSpan); this.type = type || value.type; } - + isEquivalent(stmt: Statement): boolean { + return stmt instanceof DeclareVarStmt && this.name === stmt.name && + this.value.isEquivalent(stmt.value); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitDeclareVarStmt(this, context); } @@ -576,6 +691,10 @@ export class DeclareFunctionStmt extends Statement { super(modifiers, sourceSpan); this.type = type || null; } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof DeclareFunctionStmt && areAllEquivalent(this.params, stmt.params) && + areAllEquivalent(this.statements, stmt.statements); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitDeclareFunctionStmt(this, context); @@ -586,6 +705,9 @@ export class ExpressionStatement extends Statement { constructor(public expr: Expression, sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof ExpressionStatement && this.expr.isEquivalent(stmt.expr); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitExpressionStmt(this, context); @@ -597,6 +719,9 @@ export class ReturnStatement extends Statement { constructor(public value: Expression, sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof ReturnStatement && this.value.isEquivalent(stmt.value); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitReturnStmt(this, context); } @@ -617,6 +742,7 @@ export class ClassField extends AbstractClassPart { constructor(public name: string, type?: Type|null, modifiers: StmtModifier[]|null = null) { super(type, modifiers); } + isEquivalent(f: ClassField) { return this.name === f.name; } } @@ -626,6 +752,9 @@ export class ClassMethod extends AbstractClassPart { type?: Type|null, modifiers: StmtModifier[]|null = null) { super(type, modifiers); } + isEquivalent(m: ClassMethod) { + return this.name === m.name && areAllEquivalent(this.body, m.body); + } } @@ -635,6 +764,9 @@ export class ClassGetter extends AbstractClassPart { modifiers: StmtModifier[]|null = null) { super(type, modifiers); } + isEquivalent(m: ClassGetter) { + return this.name === m.name && areAllEquivalent(this.body, m.body); + } } @@ -646,6 +778,14 @@ export class ClassStmt extends Statement { sourceSpan?: ParseSourceSpan|null) { super(modifiers, sourceSpan); } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof ClassStmt && this.name === stmt.name && + nullSafeIsEquivalent(this.parent, stmt.parent) && + areAllEquivalent(this.fields, stmt.fields) && + areAllEquivalent(this.getters, stmt.getters) && + this.constructorMethod.isEquivalent(stmt.constructorMethod) && + areAllEquivalent(this.methods, stmt.methods); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitDeclareClassStmt(this, context); } @@ -658,6 +798,11 @@ export class IfStmt extends Statement { public falseCase: Statement[] = [], sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof IfStmt && this.condition.isEquivalent(stmt.condition) && + areAllEquivalent(this.trueCase, stmt.trueCase) && + areAllEquivalent(this.falseCase, stmt.falseCase); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitIfStmt(this, context); } @@ -668,6 +813,7 @@ export class CommentStmt extends Statement { constructor(public comment: string, sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: Statement): boolean { return stmt instanceof CommentStmt; } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitCommentStmt(this, context); } @@ -680,6 +826,10 @@ export class TryCatchStmt extends Statement { sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: Statement): boolean { + return stmt instanceof TryCatchStmt && areAllEquivalent(this.bodyStmts, stmt.bodyStmts) && + areAllEquivalent(this.catchStmts, stmt.catchStmts); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitTryCatchStmt(this, context); } @@ -690,6 +840,9 @@ export class ThrowStmt extends Statement { constructor(public error: Expression, sourceSpan?: ParseSourceSpan|null) { super(null, sourceSpan); } + isEquivalent(stmt: ThrowStmt): boolean { + return stmt instanceof TryCatchStmt && this.error.isEquivalent(stmt.error); + } visitStatement(visitor: StatementVisitor, context: any): any { return visitor.visitThrowStmt(this, context); } From 4e6aa9c2db797bbafd7abc750b48fa206bad8be7 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 25 Sep 2017 12:39:51 +0300 Subject: [PATCH 3/7] fix(upgrade): ensure downgraded components are destroyed in the Angular zone --- .../src/common/downgrade_component_adapter.ts | 4 +- .../downgrade_component_adapter_spec.ts | 4 +- .../integration/downgrade_module_spec.ts | 38 ++++++++++++++++++- 3 files changed, 42 insertions(+), 4 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 13bb1ed9a4..9f66465133 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -203,11 +203,13 @@ export class DowngradeComponentAdapter { } registerCleanup(needsNgZone: boolean) { + const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); + this.element.on !('$destroy', () => { this.componentScope.$destroy(); this.componentRef.injector.get(TestabilityRegistry) .unregisterApplication(this.componentRef.location.nativeElement); - this.componentRef.destroy(); + destroyComponentRef(); if (needsNgZone) { this.appRef.detachView(this.componentRef.hostView); } diff --git a/packages/upgrade/test/common/downgrade_component_adapter_spec.ts b/packages/upgrade/test/common/downgrade_component_adapter_spec.ts index d2656ecf1e..7f91397afe 100644 --- a/packages/upgrade/test/common/downgrade_component_adapter_spec.ts +++ b/packages/upgrade/test/common/downgrade_component_adapter_spec.ts @@ -122,7 +122,7 @@ export function main() { let $compile = undefined as any; let $parse = undefined as any; let componentFactory: ComponentFactory; // testbed - let wrapCallback = undefined as any; + let wrapCallback = (cb: any) => cb; content = `

new component

@@ -183,7 +183,7 @@ export function main() { expect(registry.getAllTestabilities().length).toEqual(0); adapter.createComponent([]); expect(registry.getAllTestabilities().length).toEqual(1); - adapter.registerCleanup(true); + adapter.registerCleanup(); element.remove !(); expect(registry.getAllTestabilities().length).toEqual(0); }); diff --git a/packages/upgrade/test/static/integration/downgrade_module_spec.ts b/packages/upgrade/test/static/integration/downgrade_module_spec.ts index 4544e5c7b7..d3617d7b63 100644 --- a/packages/upgrade/test/static/integration/downgrade_module_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_module_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, StaticProvider, destroyPlatform} from '@angular/core'; +import {Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, StaticProvider, destroyPlatform} from '@angular/core'; import {async, fakeAsync, tick} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; @@ -170,6 +170,42 @@ export function main() { }); })); + it('should destroy components inside the Angular zone', async(() => { + let destroyedInTheZone = false; + + @Component({selector: 'ng2', template: ''}) + class Ng2Component implements OnDestroy { + ngOnDestroy() { destroyedInTheZone = NgZone.isInAngularZone(); } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module('ng1', [lazyModuleName]) + .directive( + 'ng2', downgradeComponent({component: Ng2Component, propagateDigest})); + + const element = html(''); + const $injector = angular.bootstrap(element, [ng1Module.name]); + const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; + + // Wait for the module to be bootstrapped. + setTimeout(() => { + $rootScope.$apply('hideNg2 = true'); + expect(destroyedInTheZone).toBe(true); + }); + })); + it('should propagate input changes inside the Angular zone', async(() => { let ng2Component: Ng2Component; From 29dbc3b67c8feae1fe2c131229c59fe38e58435a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 25 Sep 2017 13:22:35 +0300 Subject: [PATCH 4/7] refactor(upgrade): remove redundant call to `appRef.detach()` Once a `ViewRef` is attached to the `ApplicationRef`, destroying the corresponding `ComponentRef` will automatically detach the `ViewRef`. --- .../upgrade/src/common/downgrade_component.ts | 2 +- .../src/common/downgrade_component_adapter.ts | 10 ++-- .../integration/downgrade_module_spec.ts | 49 ++++++++++++++++++- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component.ts b/packages/upgrade/src/common/downgrade_component.ts index 14ebc80738..a6d736582c 100644 --- a/packages/upgrade/src/common/downgrade_component.ts +++ b/packages/upgrade/src/common/downgrade_component.ts @@ -115,7 +115,7 @@ export function downgradeComponent(info: { facade.createComponent(projectableNodes); facade.setupInputs(needsNgZone, info.propagateDigest); facade.setupOutputs(); - facade.registerCleanup(needsNgZone); + facade.registerCleanup(); injectorPromise.resolve(facade.getInjector()); diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 9f66465133..314cd8cf3c 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -25,7 +25,6 @@ export class DowngradeComponentAdapter { private componentRef: ComponentRef; private component: any; private changeDetector: ChangeDetectorRef; - private appRef: ApplicationRef; constructor( private element: angular.IAugmentedJQuery, private attrs: angular.IAttributes, @@ -35,7 +34,6 @@ export class DowngradeComponentAdapter { private componentFactory: ComponentFactory, private wrapCallback: (cb: () => T) => () => T) { this.componentScope = scope.$new(); - this.appRef = parentInjector.get(ApplicationRef); } compileContents(): Node[][] { @@ -154,7 +152,8 @@ export class DowngradeComponentAdapter { // Attach the view so that it will be dirty-checked. if (needsNgZone) { - this.appRef.attachView(this.componentRef.hostView); + const appRef = this.parentInjector.get(ApplicationRef); + appRef.attachView(this.componentRef.hostView); } } @@ -202,7 +201,7 @@ export class DowngradeComponentAdapter { } } - registerCleanup(needsNgZone: boolean) { + registerCleanup() { const destroyComponentRef = this.wrapCallback(() => this.componentRef.destroy()); this.element.on !('$destroy', () => { @@ -210,9 +209,6 @@ export class DowngradeComponentAdapter { this.componentRef.injector.get(TestabilityRegistry) .unregisterApplication(this.componentRef.location.nativeElement); destroyComponentRef(); - if (needsNgZone) { - this.appRef.detachView(this.componentRef.hostView); - } }); } diff --git a/packages/upgrade/test/static/integration/downgrade_module_spec.ts b/packages/upgrade/test/static/integration/downgrade_module_spec.ts index d3617d7b63..71012a6942 100644 --- a/packages/upgrade/test/static/integration/downgrade_module_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_module_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, StaticProvider, destroyPlatform} from '@angular/core'; +import {ApplicationRef, Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, StaticProvider, ViewRef, destroyPlatform} from '@angular/core'; import {async, fakeAsync, tick} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; @@ -306,6 +306,53 @@ export function main() { }); })); + it('should detach hostViews from the ApplicationRef once destroyed', async(() => { + let ng2Component: Ng2Component; + + @Component({selector: 'ng2', template: ''}) + class Ng2Component { + constructor(public appRef: ApplicationRef) { + ng2Component = this; + spyOn(appRef, 'attachView').and.callThrough(); + spyOn(appRef, 'detachView').and.callThrough(); + } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module('ng1', [lazyModuleName]) + .directive( + 'ng2', downgradeComponent({component: Ng2Component, propagateDigest})); + + const element = html(''); + const $injector = angular.bootstrap(element, [ng1Module.name]); + const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; + + // Wait for the module to be bootstrapped. + setTimeout(() => { + const hostView: ViewRef = + (ng2Component.appRef.attachView as jasmine.Spy).calls.mostRecent().args[0]; + + expect(hostView.destroyed).toBe(false); + + $rootScope.$apply('hideNg2 = true'); + + expect(hostView.destroyed).toBe(true); + expect(ng2Component.appRef.detachView).toHaveBeenCalledWith(hostView); + }); + })); + it('should only retrieve the Angular zone once (and cache it for later use)', fakeAsync(() => { let count = 0; From 546eb7d8517d50eda5982645c562fca16997de3c Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 25 Sep 2017 14:44:24 +0300 Subject: [PATCH 5/7] test(upgrade): ensure AngularJS is bootstraped inside the Angular zone in tests --- packages/upgrade/test/static/test_helpers.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/upgrade/test/static/test_helpers.ts b/packages/upgrade/test/static/test_helpers.ts index a384f7e9cd..c1d12988b1 100644 --- a/packages/upgrade/test/static/test_helpers.ts +++ b/packages/upgrade/test/static/test_helpers.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {PlatformRef, Type} from '@angular/core'; +import {NgZone, PlatformRef, Type} from '@angular/core'; import * as angular from '@angular/upgrade/src/common/angular1'; import {$ROOT_SCOPE} from '@angular/upgrade/src/common/constants'; import {UpgradeModule} from '@angular/upgrade/static'; @@ -18,11 +18,19 @@ export function bootstrap( // We bootstrap the Angular module first; then when it is ready (async) we bootstrap the AngularJS // module on the bootstrap element (also ensuring that AngularJS errors will fail the test). return platform.bootstrapModule(Ng2Module).then(ref => { + const ngZone = ref.injector.get(NgZone); const upgrade = ref.injector.get(UpgradeModule); const failHardModule: any = ($provide: angular.IProvideService) => { $provide.value('$exceptionHandler', (err: any) => { throw err; }); }; - upgrade.bootstrap(element, [failHardModule, ng1Module.name]); + + // The `bootstrap()` helper is used for convenience in tests, so that we don't have to inject + // and call `upgrade.bootstrap()` on every Angular module. + // In order to closer emulate what happens in real application, ensure AngularJS is bootstrapped + // inside the Angular zone. + // + ngZone.run(() => upgrade.bootstrap(element, [failHardModule, ng1Module.name])); + return upgrade; }); } From 617b3d2ba5ca71397298d99a9d65eebb335e1480 Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Mon, 25 Sep 2017 16:10:23 +0300 Subject: [PATCH 6/7] fix(upgrade): correctly run change detection when `propagateDigest` is false --- .../src/common/downgrade_component_adapter.ts | 2 +- .../integration/downgrade_component_spec.ts | 38 ++++++++++++++++++- 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 314cd8cf3c..906bd21869 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -151,7 +151,7 @@ export class DowngradeComponentAdapter { } // Attach the view so that it will be dirty-checked. - if (needsNgZone) { + if (needsNgZone || !propagateDigest) { const appRef = this.parentInjector.get(ApplicationRef); appRef.attachView(this.componentRef.hostView); } diff --git a/packages/upgrade/test/static/integration/downgrade_component_spec.ts b/packages/upgrade/test/static/integration/downgrade_component_spec.ts index bbc3e525ec..01906ec5c2 100644 --- a/packages/upgrade/test/static/integration/downgrade_component_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_component_spec.ts @@ -7,7 +7,7 @@ */ import {ChangeDetectorRef, Compiler, Component, ComponentFactoryResolver, EventEmitter, Injector, Input, NgModule, NgModuleRef, OnChanges, OnDestroy, SimpleChanges, destroyPlatform} from '@angular/core'; -import {async} from '@angular/core/testing'; +import {async, fakeAsync, tick} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; import * as angular from '@angular/upgrade/src/common/angular1'; @@ -274,6 +274,42 @@ export function main() { }); })); + it('should still run normal Angular change-detection regardless of `propagateDigest`', + fakeAsync(() => { + let ng2Component: Ng2Component; + + @Component({selector: 'ng2', template: '{{ value }}'}) + class Ng2Component { + value = 'foo'; + constructor() { setTimeout(() => this.value = 'bar', 1000); } + } + + @NgModule({ + imports: [BrowserModule, UpgradeModule], + declarations: [Ng2Component], + entryComponents: [Ng2Component] + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const ng1Module = + angular.module('ng1', []) + .directive( + 'ng2A', downgradeComponent({component: Ng2Component, propagateDigest: true})) + .directive( + 'ng2B', downgradeComponent({component: Ng2Component, propagateDigest: false})); + + const element = html(' | '); + + bootstrap(platformBrowserDynamic(), Ng2Module, element, ng1Module).then(upgrade => { + expect(element.textContent).toBe('foo | foo'); + + tick(1000); + expect(element.textContent).toBe('bar | bar'); + }); + })); + it('should initialize inputs in time for `ngOnChanges`', async(() => { @Component({ selector: 'ng2', From eef7d8aa11baf394d9448bbffe24ed700480a96a Mon Sep 17 00:00:00 2001 From: George Kalpakas Date: Tue, 26 Sep 2017 03:07:12 +0300 Subject: [PATCH 7/7] fix(upgrade): call `ngOnInit()` after `ngOnChanges()` (on components with inputs) Fixes #18913 --- .../src/common/downgrade_component_adapter.ts | 17 +- .../integration/downgrade_module_spec.ts | 177 +++++++++++++++++- 2 files changed, 178 insertions(+), 16 deletions(-) diff --git a/packages/upgrade/src/common/downgrade_component_adapter.ts b/packages/upgrade/src/common/downgrade_component_adapter.ts index 906bd21869..74509791f5 100644 --- a/packages/upgrade/src/common/downgrade_component_adapter.ts +++ b/packages/upgrade/src/common/downgrade_component_adapter.ts @@ -99,7 +99,7 @@ export class DowngradeComponentAdapter { })(input.prop); attrs.$observe(input.attr, observeFn); - // Use `$watch()` (in addition to `$observe()`) in order to initialize the input in time + // Use `$watch()` (in addition to `$observe()`) in order to initialize the input in time // for `ngOnChanges()`. This is necessary if we are already in a `$digest`, which means that // `ngOnChanges()` (which is called by a watcher) will run before the `$observe()` callback. let unwatch: Function|null = this.componentScope.$watch(() => { @@ -138,8 +138,7 @@ export class DowngradeComponentAdapter { (this.component).ngOnChanges(inputChanges !); } - // If opted out of propagating digests, invoke change detection - // when inputs change + // If opted out of propagating digests, invoke change detection when inputs change. if (!propagateDigest) { detectChanges(); } @@ -150,10 +149,16 @@ export class DowngradeComponentAdapter { this.componentScope.$watch(this.wrapCallback(detectChanges)); } - // Attach the view so that it will be dirty-checked. + // If necessary, attach the view so that it will be dirty-checked. + // (Allow time for the initial input values to be set and `ngOnChanges()` to be called.) if (needsNgZone || !propagateDigest) { - const appRef = this.parentInjector.get(ApplicationRef); - appRef.attachView(this.componentRef.hostView); + let unwatch: Function|null = this.componentScope.$watch(() => { + unwatch !(); + unwatch = null; + + const appRef = this.parentInjector.get(ApplicationRef); + appRef.attachView(this.componentRef.hostView); + }); } } diff --git a/packages/upgrade/test/static/integration/downgrade_module_spec.ts b/packages/upgrade/test/static/integration/downgrade_module_spec.ts index 71012a6942..52e2da16dd 100644 --- a/packages/upgrade/test/static/integration/downgrade_module_spec.ts +++ b/packages/upgrade/test/static/integration/downgrade_module_spec.ts @@ -6,7 +6,7 @@ * found in the LICENSE file at https://angular.io/license */ -import {ApplicationRef, Component, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, StaticProvider, ViewRef, destroyPlatform} from '@angular/core'; +import {AfterContentChecked, AfterContentInit, AfterViewChecked, AfterViewInit, ApplicationRef, Component, DoCheck, Inject, Injector, Input, NgModule, NgZone, OnChanges, OnDestroy, OnInit, StaticProvider, ViewRef, destroyPlatform} from '@angular/core'; import {async, fakeAsync, tick} from '@angular/core/testing'; import {BrowserModule} from '@angular/platform-browser'; import {platformBrowserDynamic} from '@angular/platform-browser-dynamic'; @@ -16,7 +16,7 @@ import {$ROOT_SCOPE, INJECTOR_KEY, LAZY_MODULE_REF} from '@angular/upgrade/src/c import {LazyModuleRef} from '@angular/upgrade/src/common/util'; import {downgradeComponent, downgradeModule} from '@angular/upgrade/static'; -import {html} from '../test_helpers'; +import {html, multiTrim} from '../test_helpers'; export function main() { @@ -306,6 +306,162 @@ export function main() { }); })); + it('should run the lifecycle hooks in the correct order', async(() => { + const logs: string[] = []; + let rootScope: angular.IRootScopeService; + + @Component({ + selector: 'ng2', + template: ` + {{ value }} + + + ` + }) + class Ng2Component implements AfterContentChecked, + AfterContentInit, AfterViewChecked, AfterViewInit, DoCheck, OnChanges, OnDestroy, + OnInit { + @Input() value = 'foo'; + + ngAfterContentChecked() { this.log('AfterContentChecked'); } + ngAfterContentInit() { this.log('AfterContentInit'); } + ngAfterViewChecked() { this.log('AfterViewChecked'); } + ngAfterViewInit() { this.log('AfterViewInit'); } + ngDoCheck() { this.log('DoCheck'); } + ngOnChanges() { this.log('OnChanges'); } + ngOnDestroy() { this.log('OnDestroy'); } + ngOnInit() { this.log('OnInit'); } + + private log(hook: string) { logs.push(`${hook}(${this.value})`); } + } + + @NgModule({ + declarations: [Ng2Component], + entryComponents: [Ng2Component], + imports: [BrowserModule], + }) + class Ng2Module { + ngDoBootstrap() {} + } + + const bootstrapFn = (extraProviders: StaticProvider[]) => + platformBrowserDynamic(extraProviders).bootstrapModule(Ng2Module); + const lazyModuleName = downgradeModule(bootstrapFn); + const ng1Module = + angular.module('ng1', [lazyModuleName]) + .directive('ng2', downgradeComponent({component: Ng2Component, propagateDigest})) + .run(($rootScope: angular.IRootScopeService) => { + rootScope = $rootScope; + rootScope.value = 'bar'; + }); + + const element = + html('
Content
'); + angular.bootstrap(element, [ng1Module.name]); + + setTimeout(() => { // Wait for the module to be bootstrapped. + setTimeout(() => { // Wait for `$evalAsync()` to propagate inputs. + const button = element.querySelector('button') !; + + // Once initialized. + expect(multiTrim(element.textContent)).toBe('bar Content'); + expect(logs).toEqual([ + // `ngOnChanges()` call triggered directly through the `inputChanges` $watcher. + 'OnChanges(bar)', + // Initial CD triggered directly through the `detectChanges()` or `inputChanges` + // $watcher (for `propagateDigest` true/false respectively). + 'OnInit(bar)', + 'DoCheck(bar)', + 'AfterContentInit(bar)', + 'AfterContentChecked(bar)', + 'AfterViewInit(bar)', + 'AfterViewChecked(bar)', + ...(propagateDigest ? + [ + // CD triggered directly through the `detectChanges()` $watcher (2nd + // $digest). + 'DoCheck(bar)', + 'AfterContentChecked(bar)', + 'AfterViewChecked(bar)', + ] : + []), + // CD triggered due to entering/leaving the NgZone (in `downgradeFn()`). + 'DoCheck(bar)', + 'AfterContentChecked(bar)', + 'AfterViewChecked(bar)', + ]); + logs.length = 0; + + // Change inputs and run `$digest`. + rootScope.$apply('value = "baz"'); + expect(multiTrim(element.textContent)).toBe('baz Content'); + expect(logs).toEqual([ + // `ngOnChanges()` call triggered directly through the `inputChanges` $watcher. + 'OnChanges(baz)', + // `propagateDigest: true` (3 CD runs): + // - CD triggered due to entering/leaving the NgZone (in `inputChanges` + // $watcher). + // - CD triggered directly through the `detectChanges()` $watcher. + // - CD triggered due to entering/leaving the NgZone (in `detectChanges` + // $watcher). + // `propagateDigest: false` (2 CD runs): + // - CD triggered directly through the `inputChanges` $watcher. + // - CD triggered due to entering/leaving the NgZone (in `inputChanges` + // $watcher). + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ...(propagateDigest ? + [ + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ] : + []), + ]); + logs.length = 0; + + // Run `$digest` (without changing inputs). + rootScope.$digest(); + expect(multiTrim(element.textContent)).toBe('baz Content'); + expect(logs).toEqual( + propagateDigest ? + [ + // CD triggered directly through the `detectChanges()` $watcher. + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + // CD triggered due to entering/leaving the NgZone (in the above $watcher). + 'DoCheck(baz)', + 'AfterContentChecked(baz)', + 'AfterViewChecked(baz)', + ] : + []); + logs.length = 0; + + // Trigger change detection (without changing inputs). + button.click(); + expect(multiTrim(element.textContent)).toBe('qux Content'); + expect(logs).toEqual([ + 'DoCheck(qux)', + 'AfterContentChecked(qux)', + 'AfterViewChecked(qux)', + ]); + logs.length = 0; + + // Destroy the component. + rootScope.$apply('hideNg2 = true'); + expect(logs).toEqual([ + 'OnDestroy(qux)', + ]); + logs.length = 0; + }); + }); + })); + it('should detach hostViews from the ApplicationRef once destroyed', async(() => { let ng2Component: Ng2Component; @@ -339,17 +495,18 @@ export function main() { const $injector = angular.bootstrap(element, [ng1Module.name]); const $rootScope = $injector.get($ROOT_SCOPE) as angular.IRootScopeService; - // Wait for the module to be bootstrapped. - setTimeout(() => { - const hostView: ViewRef = - (ng2Component.appRef.attachView as jasmine.Spy).calls.mostRecent().args[0]; + setTimeout(() => { // Wait for the module to be bootstrapped. + setTimeout(() => { // Wait for the hostView to be attached (during the `$digest`). + const hostView: ViewRef = + (ng2Component.appRef.attachView as jasmine.Spy).calls.mostRecent().args[0]; - expect(hostView.destroyed).toBe(false); + expect(hostView.destroyed).toBe(false); - $rootScope.$apply('hideNg2 = true'); + $rootScope.$apply('hideNg2 = true'); - expect(hostView.destroyed).toBe(true); - expect(ng2Component.appRef.detachView).toHaveBeenCalledWith(hostView); + expect(hostView.destroyed).toBe(true); + expect(ng2Component.appRef.detachView).toHaveBeenCalledWith(hostView); + }); }); }));