Skip to content

fix(@ngtools/webpack): fix rebuilds for transitive and global type deps #16164

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 19, 2019
Merged

fix(@ngtools/webpack): fix rebuilds for transitive and global type deps #16164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions packages/angular_devkit/build_angular/test/browser/rebuild_spec_large.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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')),
Expand Down
63 changes: 48 additions & 15 deletions packages/ngtools/webpack/src/angular_compiler_plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,9 @@ export class AngularCompilerPlugin {
// even whe only a single file gets updated.
private _hadFullJitEmit: boolean | undefined;
private _unusedFiles = new Set<string>();
private _typeDeps = new Set<string>();
private _changedFileExtensions = new Set(['ts', 'tsx', 'html', 'css', 'js', 'json']);
private _nodeModulesRegExp = /[\\\/]node_modules[\\\/]/;

// Webpack plugin.
private _firstRun = true;
Expand Down Expand Up @@ -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
Expand All @@ -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;
Expand All @@ -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()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be simpler to do something like?

this._typeDeps = newTsProgram
      .getSourceFiles()
      .filter(sf => sf.isDeclarationFile)
      .map(({ fileName }) => fileName);

in _createOrUpdateProgram which would also result for this fix to be available for VE compilations

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried adding this at the end of the _createOrUpdateProgram method:

    console.log(newTsProgram!
      .getSourceFiles()
      .filter(sf => sf.isDeclarationFile && !sf.fileName.includes('node_modules'))
      .map(({ fileName }) => fileName))

Then I ran this test. It logged the following:

[ 'D:/work/cli/tests/angular_devkit/build_angular/test-project-host-hello-world-app-moqnlwoki9/src/typings.d.ts' ]
[ 'D:/work/cli/tests/angular_devkit/build_angular/test-project-host-hello-world-app-moqnlwoki9/src/typings.d.ts' ]
[ 'D:/work/cli/tests/angular_devkit/build_angular/test-project-host-hello-world-app-moqnlwoki9/src/typings.d.ts' ]
[ 'D:/work/cli/tests/angular_devkit/build_angular/test-project-host-hello-world-app-moqnlwoki9/src/typings.d.ts' ]
[ 'D:/work/cli/tests/angular_devkit/build_angular/test-project-host-hello-world-app-moqnlwoki9/src/typings.d.ts' ]

The typings only file was detected, but not the interface files.

.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.
Expand All @@ -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.
Expand All @@ -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) {
Expand Down Expand Up @@ -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 '
Expand Down Expand Up @@ -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.
Expand Down
4 changes: 4 additions & 0 deletions packages/ngtools/webpack/src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
})
Expand Down