Skip to content

fix(@ngtools/webpack): recursive look up unused files #16148

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.

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 2 commits into from
Nov 12, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,40 @@ describe('Browser Builder unused files warnings', () => {
await run.stop();
});

it('should not show warning when excluded files are unused', async () => {
if (veEnabled) {
// TODO: https://github.com/angular/angular-cli/issues/15056
pending('Only supported in Ivy.');

return;
}

const ignoredFiles = {
'src/file.d.ts': 'export type MyType = number;',
'src/file.ngsummary.ts': 'export const hello = 42;',
'src/file.ngfactory.ts': 'export const hello = 42;',
'src/file.ngstyle.ts': 'export const hello = 42;',
'src/file.ng_typecheck__.ts': 'export const hello = 42;',
};

host.writeMultipleFiles(ignoredFiles);

host.replaceInFile(
'src/tsconfig.app.json',
'"main.ts"',
`"main.ts", ${Object.keys(ignoredFiles).map(f => `"${f.replace('src/', '')}"`).join(',')}`,
);

const logger = new TestLogger('unused-files-warnings');
const run = await architect.scheduleTarget(targetSpec, undefined, { logger });
const output = await run.result as BrowserBuilderOutput;
expect(output.success).toBe(true);
expect(logger.includes(warningMessageSuffix)).toBe(false);
logger.clear();

await run.stop();
});

it('should not show warning when type files are used', async () => {
if (veEnabled) {
// TODO: https://github.com/angular/angular-cli/issues/15056
Expand Down Expand Up @@ -94,6 +128,36 @@ describe('Browser Builder unused files warnings', () => {
await run.stop();
});

it('should not show warning when type files are used transitively', async () => {
if (veEnabled) {
// TODO: https://github.com/angular/angular-cli/issues/15056
pending('Only supported in Ivy.');

return;
}

host.writeMultipleFiles({
'src/app/type.ts':
`import {Myinterface} from './interface'; export type MyType = Myinterface;`,
'src/app/interface.ts': 'export interface Myinterface {nbr: number;}',
});

host.replaceInFile(
'src/app/app.component.ts',
`'@angular/core';`,
`'@angular/core';\nimport { MyType } from './type';\n`,
);

const logger = new TestLogger('unused-files-warnings');
const run = await architect.scheduleTarget(targetSpec, undefined, { logger });
const output = await run.result as BrowserBuilderOutput;
expect(output.success).toBe(true);
expect(logger.includes(warningMessageSuffix)).toBe(false);
logger.clear();

await run.stop();
});

it('works for rebuilds', async () => {
if (veEnabled) {
// TODO: https://github.com/angular/angular-cli/issues/15056
Expand Down
54 changes: 27 additions & 27 deletions packages/ngtools/webpack/src/angular_compiler_plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -611,33 +611,28 @@ export class AngularCompilerPlugin {
// 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)\.ts|ng_typecheck__\.ts)$/;
const fileExcludeRegExp = /(\.(d|ngfactory|ngstyle|ngsummary)\.ts|ng_typecheck__\.ts)$/;

const usedFiles = new Set<string>();
for (const compilationModule of compilation.modules) {
if (!compilationModule.resource) {
continue;
// 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)) {
unusedSourceFileNames.delete(fileName);
this.getDependencies(fileName, false).forEach(f => removeSourceFile(f));
}
};

usedFiles.add(forwardSlashPath(compilationModule.resource));

// We need the below for dependencies which
// are not emitted such as type only TS files
for (const dependency of compilationModule.buildInfo.fileDependencies) {
usedFiles.add(forwardSlashPath(dependency));
}
}

const sourceFiles = program.getSourceFiles();
for (const { fileName } of sourceFiles) {
if (
fileExcludeRegExp.test(fileName)
|| usedFiles.has(fileName)
|| this._unusedFiles.has(fileName)
) {
continue;
}
// 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);

// Anything that remains is unused, because it wasn't referenced directly or transitively
// on the files in the compilation.
for (const fileName of unusedSourceFileNames) {
compilation.warnings.push(
`${fileName} is part of the TypeScript compilation but it's unused.\n` +
`Add only entry points to the 'files' or 'include' properties in your tsconfig.`,
Expand Down Expand Up @@ -1207,7 +1202,7 @@ export class AngularCompilerPlugin {
return { outputText, sourceMap, errorDependencies };
}

getDependencies(fileName: string): string[] {
getDependencies(fileName: string, includeResources = true): string[] {
const resolvedFileName = this._compilerHost.resolve(fileName);
const sourceFile = this._compilerHost.getSourceFile(resolvedFileName, ts.ScriptTarget.Latest);
if (!sourceFile) {
Expand Down Expand Up @@ -1241,14 +1236,19 @@ export class AngularCompilerPlugin {
})
.filter(x => x) as string[];

const resourceImports = findResources(sourceFile)
.map(resourcePath => resolve(dirname(resolvedFileName), normalize(resourcePath)));
let resourceImports: string[] = [], resourceDependencies: string[] = [];
if (includeResources) {
resourceImports = findResources(sourceFile)
.map(resourcePath => resolve(dirname(resolvedFileName), normalize(resourcePath)));
resourceDependencies =
this.getResourceDependencies(this._compilerHost.denormalizePath(resolvedFileName));
}

// These paths are meant to be used by the loader so we must denormalize them.
const uniqueDependencies = new Set([
...esImports,
...resourceImports,
...this.getResourceDependencies(this._compilerHost.denormalizePath(resolvedFileName)),
...resourceDependencies,
].map((p) => p && this._compilerHost.denormalizePath(p)));

return [...uniqueDependencies];
Expand Down