From b9e00b68b5153c89f76083dfe1243721c79a90f1 Mon Sep 17 00:00:00 2001 From: Filipe Silva Date: Tue, 12 Nov 2019 14:22:48 +0000 Subject: [PATCH] fix(@ngtools/webpack): fix rebuilds for transitive and global type deps Fix #15856 --- .../test/browser/rebuild_spec_large.ts | 46 ++++++++++++++ .../webpack/src/angular_compiler_plugin.ts | 63 ++++++++++++++----- packages/ngtools/webpack/src/loader.ts | 4 ++ 3 files changed, 98 insertions(+), 15 deletions(-) diff --git a/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts b/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts index 56b23dabbd15..d7afe4e5f618 100644 --- a/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts +++ b/packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts @@ -222,6 +222,52 @@ describe('Browser Builder rebuilds', () => { .toPromise(); }); + it('rebuilds on transitive type-only file changes', async () => { + if (veEnabled) { + // TODO: https://github.com/angular/angular-cli/issues/15056 + pending('Only supported in Ivy.'); + + return; + } + host.writeMultipleFiles({ + 'src/interface1.ts': ` + import { Interface2 } from './interface2'; + export interface Interface1 extends Interface2 { } + `, + 'src/interface2.ts': ` + import { Interface3 } from './interface3'; + export interface Interface2 extends Interface3 { } + `, + 'src/interface3.ts': `export interface Interface3 { nbr: number; }`, + }); + host.appendToFile('src/main.ts', ` + import { Interface1 } from './interface1'; + const something: Interface1 = { nbr: 43 }; + `); + + const overrides = { watch: true }; + const run = await architect.scheduleTarget(target, overrides); + let buildNumber = 0; + await run.output + .pipe( + debounceTime(rebuildDebounceTime), + tap(buildEvent => expect(buildEvent.success).toBe(true)), + tap(() => { + // NOTE: this only works for transitive type deps after the first build, and only if the + // typedep file was there on the previous build. + // Make sure the first rebuild is triggered on a direct dep (typedep or not). + buildNumber++; + if (buildNumber < 4) { + host.appendToFile(`src/interface${buildNumber}.ts`, `export type MyType = string;`); + } else { + host.appendToFile(`src/typings.d.ts`, `export type MyType = string;`); + } + }), + take(5), + ) + .toPromise(); + }); + it('rebuilds after errors in JIT', async () => { const origContent = virtualFs.fileBufferToString( host.scopedSync().read(normalize('src/app/app.component.ts')), diff --git a/packages/ngtools/webpack/src/angular_compiler_plugin.ts b/packages/ngtools/webpack/src/angular_compiler_plugin.ts index 46cbc70418d4..5b89e575572d 100644 --- a/packages/ngtools/webpack/src/angular_compiler_plugin.ts +++ b/packages/ngtools/webpack/src/angular_compiler_plugin.ts @@ -110,7 +110,9 @@ export class AngularCompilerPlugin { // even whe only a single file gets updated. private _hadFullJitEmit: boolean | undefined; private _unusedFiles = new Set(); + private _typeDeps = new Set(); private _changedFileExtensions = new Set(['ts', 'tsx', 'html', 'css', 'js', 'json']); + private _nodeModulesRegExp = /[\\\/]node_modules[\\\/]/; // Webpack plugin. private _firstRun = true; @@ -592,7 +594,7 @@ export class AngularCompilerPlugin { } } - private _warnOnUnusedFiles(compilation: compilation.Compilation) { + private _checkUnusedFiles(compilation: compilation.Compilation) { // Only do the unused TS files checks when under Ivy // since previously we did include unused files in the compilation // See: https://github.com/angular/angular-cli/pull/15030 @@ -601,6 +603,7 @@ export class AngularCompilerPlugin { return; } + // Bail if there's no TS program. Nothing to do in that case. const program = this._getTsProgram(); if (!program) { return; @@ -609,26 +612,36 @@ export class AngularCompilerPlugin { // Exclude the following files from unused checks // - ngfactories & ngstyle might not have a correspondent // JS file example `@angular/core/core.ngfactory.ts`. - // - .d.ts files might not have a correspondent JS file due to bundling. // - __ng_typecheck__.ts will never be requested. - const fileExcludeRegExp = /(\.(d|ngfactory|ngstyle|ngsummary)\.ts|ng_typecheck__\.ts)$/; + const fileExcludeRegExp = /(\.(ngfactory|ngstyle|ngsummary)\.ts|ng_typecheck__\.ts)$/; + + // Start all the source file names we care about. + // Ignore matches to the regexp above, files we've already reported once before, and + // node_modules. + const sourceFiles = program.getSourceFiles() + .map(x => this._compilerHost.denormalizePath(x.fileName)) + .filter(f => !(fileExcludeRegExp.test(f) || this._unusedFiles.has(f) + || this._nodeModulesRegExp.test(f))); + + // Make a set with the sources, but exclude .d.ts files since those are type-only. + const unusedSourceFileNames = new Set(sourceFiles.filter(f => !f.endsWith('.d.ts'))); + // Separately keep track of type-only deps. + const typeDepFileNames = new Set(sourceFiles); - // Start with a set of all the source file names we care about. - const unusedSourceFileNames = new Set( - program.getSourceFiles() - .map(x => this._compilerHost.denormalizePath(x.fileName)) - .filter(f => !(fileExcludeRegExp.test(f) || this._unusedFiles.has(f))), - ); // This function removes a source file name and all its dependencies from the set. - const removeSourceFile = (fileName: string) => { - if (unusedSourceFileNames.has(fileName)) { + const removeSourceFile = (fileName: string, originalModule = false) => { + if (unusedSourceFileNames.has(fileName) + || (originalModule && typeDepFileNames.has(fileName))) { unusedSourceFileNames.delete(fileName); + if (originalModule) { + typeDepFileNames.delete(fileName); + } this.getDependencies(fileName, false).forEach(f => removeSourceFile(f)); } }; - // Go over all the modules in the webpack compilation and remove them from the set. - compilation.modules.forEach(m => m.resource ? removeSourceFile(m.resource) : null); + // Go over all the modules in the webpack compilation and remove them from the sets. + compilation.modules.forEach(m => m.resource ? removeSourceFile(m.resource, true) : null); // Anything that remains is unused, because it wasn't referenced directly or transitively // on the files in the compilation. @@ -638,7 +651,15 @@ export class AngularCompilerPlugin { `Add only entry points to the 'files' or 'include' properties in your tsconfig.`, ); this._unusedFiles.add(fileName); + // Remove the truly unused from the type dep list. + typeDepFileNames.delete(fileName); } + + // At this point we know what the type deps are. + // These are the TS files that weren't part of the compilation modules, aren't unused, but were + // part of the TS original source list. + // Next build we add them to the TS entry points so that they trigger rebuilds. + this._typeDeps = typeDepFileNames; } // Registration hook for webpack plugin. @@ -657,7 +678,7 @@ export class AngularCompilerPlugin { // cleanup if not watching compiler.hooks.thisCompilation.tap('angular-compiler', compilation => { compilation.hooks.finishModules.tap('angular-compiler', () => { - this._warnOnUnusedFiles(compilation); + this._checkUnusedFiles(compilation); let rootCompiler = compiler; while (rootCompiler.parentCompilation) { @@ -1188,7 +1209,7 @@ export class AngularCompilerPlugin { let msg = `${fileName} is missing from the TypeScript compilation. ` + `Please make sure it is in your tsconfig via the 'files' or 'include' property.`; - if (/(\\|\/)node_modules(\\|\/)/.test(fileName)) { + if (this._nodeModulesRegExp.test(fileName)) { msg += '\nThe missing file seems to be part of a third party library. ' + 'TS files in published libraries are often a sign of a badly packaged library. ' + 'Please open an issue in the library repository to alert its author and ask them ' @@ -1267,6 +1288,18 @@ export class AngularCompilerPlugin { return this._resourceLoader.getResourceDependencies(resolvedFileName); } + getTypeDependencies(fileName: string): string[] { + // We currently add all type deps directly to the main path. + // If there's no main path or the lookup isn't the main path, bail. + if (!this._mainPath || this._compilerHost.resolve(fileName) != this._mainPath) { + return []; + } + + // Note: this set is always for the previous build, not the current build. + // It should be better than not having rebuilds on type deps but isn't 100% correct. + return Array.from(this._typeDeps); + } + // This code mostly comes from `performCompilation` in `@angular/compiler-cli`. // It skips the program creation because we need to use `loadNgStructureAsync()`, // and uses CustomTransformers. diff --git a/packages/ngtools/webpack/src/loader.ts b/packages/ngtools/webpack/src/loader.ts index 046b0d9d7485..3fd1d51e4b6e 100644 --- a/packages/ngtools/webpack/src/loader.ts +++ b/packages/ngtools/webpack/src/loader.ts @@ -100,6 +100,10 @@ export function ngcLoader(this: loader.LoaderContext) { styleDependencies.forEach(dep => this.addDependency(dep)); } + // Add type-only dependencies that should trigger a rebuild when they change. + const typeDependencies = plugin.getTypeDependencies(sourceFileName); + typeDependencies.forEach(dep => this.addDependency(dep)); + timeEnd(timeLabel); cb(null, result.outputText, result.sourceMap as any); })