Skip to content
Closed
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
13 changes: 13 additions & 0 deletions packages/ngtools/webpack/src/ivy/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
* found in the LICENSE file at https://angular.dev/license
*/

import * as fs from 'node:fs';
import * as path from 'node:path';
import type { LoaderContext } from 'webpack';
import { AngularPluginSymbol, FileEmitterCollection } from './symbol';
Expand Down Expand Up @@ -72,6 +73,18 @@ export function angularWebpackLoader(
);
}

// Write the declaration file in the target dir
if (result.declaration && fileEmitter.compilerOptions.declaration) {
let target = this.resourcePath.replace('.ts', '.d.ts');

Choose a reason for hiding this comment

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

medium

Using String.prototype.replace() with a string argument is not robust. It only replaces the first occurrence and doesn't handle different TypeScript file extensions like .mts or .cts correctly. A regular expression should be used to safely replace the file extension and handle all variants.

Suggested change
let target = this.resourcePath.replace('.ts', '.d.ts');
let target = this.resourcePath.replace(/(\.[cm]?ts)$/, '.d$1');

Copy link
Author

Choose a reason for hiding this comment

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

good bot

if (fileEmitter.compilerOptions.declarationDir) {
if (!fileEmitter.compilerOptions.baseUrl) {
throw new Error('When declarationDir is specified, baseUrl is required as well');
}
const relDir = path.relative(fileEmitter.compilerOptions.baseUrl, target);
target = path.join(fileEmitter.compilerOptions.declarationDir, relDir);
}
fs.writeFileSync(target, result.declaration);

Choose a reason for hiding this comment

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

high

This synchronous file write has two main problems:

  1. It's a blocking I/O call that will negatively impact performance, especially in watch mode.
  2. It will throw an error if the parent directory of target does not exist.

Ideally, this should be an asynchronous operation using fs.promises.writeFile after ensuring the directory exists with fs.promises.mkdir. This would require restructuring the promise handling in this loader to await the file I/O before calling the webpack callback.

As a minimal fix that addresses the potential crash, you could use fs.mkdirSync(path.dirname(target), { recursive: true }); before this line, but the performance issue with synchronous I/O would remain a significant concern.

Copy link
Author

Choose a reason for hiding this comment

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

Obviously I expect a cached mechanism to avoid to emit the files immediately or before returning the async call. Please point to a possible usage of output-only caching support already part of the project, if one.

}
callback(undefined, resultContent, resultMap);
})
.catch((err) => {
Expand Down
9 changes: 8 additions & 1 deletion packages/ngtools/webpack/src/ivy/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ export class AngularWebpackPlugin {
private readonly requiredFilesToEmit = new Set<string>();
private readonly requiredFilesToEmitCache = new Map<string, EmitFileResult | undefined>();
private readonly fileEmitHistory = new Map<string, FileEmitHistoryItem>();
private compilerOptions!: CompilerOptions;

constructor(options: Partial<AngularWebpackPluginOptions> = {}) {
this.pluginOptions = {
Expand Down Expand Up @@ -186,6 +187,7 @@ export class AngularWebpackPlugin {

// Setup and read TypeScript and Angular compiler configuration
const { compilerOptions, rootNames, errors } = this.loadConfiguration();
this.compilerOptions = compilerOptions;

// Create diagnostics reporter and report configuration file errors
const diagnosticsReporter = createDiagnosticsReporter(compilation, (diagnostic) =>
Expand Down Expand Up @@ -325,6 +327,8 @@ export class AngularWebpackPlugin {
compilation.compiler.webpack.NormalModule.getCompilationHooks(compilation).loader.tap(
PLUGIN_NAME,
(context) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
fileEmitters!.compilerOptions = this.compilerOptions;
const loaderContext = context as typeof context & {
[AngularPluginSymbol]?: FileEmitterCollection;
};
Expand Down Expand Up @@ -681,13 +685,16 @@ export class AngularWebpackPlugin {

let content: string | undefined;
let map: string | undefined;
let declaration: string | undefined;
program.emit(
sourceFile,
(filename, data) => {
if (filename.endsWith('.map')) {
map = data;
} else if (filename.endsWith('.js')) {
content = data;
} else if (filename.endsWith('.d.ts')) {
declaration = data;
}
},
undefined,
Expand All @@ -705,7 +712,7 @@ export class AngularWebpackPlugin {
...getExtraDependencies(sourceFile),
].map(externalizePath);

return { content, map, dependencies, hash };
return { content, map, declaration, dependencies, hash };
};
}

Expand Down
5 changes: 5 additions & 0 deletions packages/ngtools/webpack/src/ivy/symbol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
* found in the LICENSE file at https://angular.dev/license
*/

import { CompilerOptions } from 'typescript';

export const AngularPluginSymbol: unique symbol = Symbol.for('@ngtools/webpack[angular-compiler]');

export interface EmitFileResult {
content?: string;
map?: string;
declaration?: string;
dependencies: readonly string[];
hash?: Uint8Array;
}
Expand All @@ -36,6 +39,8 @@ export class FileEmitterRegistration {
export class FileEmitterCollection {
#registrations: FileEmitterRegistration[] = [];

public compilerOptions!: CompilerOptions;

register(): FileEmitterRegistration {
const registration = new FileEmitterRegistration();
this.#registrations.push(registration);
Expand Down
Loading