feat: provide custom builder with esbuild plugins#2175
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces custom Angular builders (application and dev-server) to the live-preview project to support custom esbuild plugins, updating angular.json and build scripts accordingly. Feedback highlights several critical issues: a typo in the builder replacement mapping that breaks ng serve, an application builder implementation that breaks watch mode and discards metadata, dead code and potential runtime failures in the dynamic plugin loading logic, and the use of a non-cross-platform cp command in package.json.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const BUILDER_REPLACEMENTS: Record<string, string> = { | ||
| '@siemens/angular-builder:application': '@angular/build:application' | ||
| }; |
There was a problem hiding this comment.
There is a typo in the BUILDER_REPLACEMENTS mapping. The key is set to '@siemens/angular-builder:application', but the custom builder package name defined in projects/live-preview/package.json is @siemens/live-preview, making the builder name @siemens/live-preview:application.
Because of this mismatch, executeDevServerBuilder will not recognize the custom builder as a supported application builder and will throw an error, completely breaking ng serve.
| const BUILDER_REPLACEMENTS: Record<string, string> = { | |
| '@siemens/angular-builder:application': '@angular/build:application' | |
| }; | |
| const BUILDER_REPLACEMENTS: Record<string, string> = { | |
| '@siemens/live-preview:application': '@angular/build:application' | |
| }; |
| export default createBuilder( | ||
| async (options: ApplicationOptions, context: BuilderContext): Promise<BuilderOutput> => { | ||
| const plugins = options.plugins ? await loadPlugins(options.plugins, context) : []; | ||
|
|
||
| const buildOptions = { ...options }; | ||
| delete buildOptions.plugins; | ||
|
|
||
| try { | ||
| for await (const output of buildApplication( | ||
| buildOptions as ApplicationBuilderOptions, | ||
| context, | ||
| { codePlugins: plugins } | ||
| )) { | ||
| if (!output.success) { | ||
| return output; | ||
| } | ||
| } | ||
|
|
||
| return { success: true }; | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : String(error); | ||
| context.logger.error(`Build failed: ${errorMessage}`); | ||
| return { | ||
| success: false, | ||
| error: errorMessage | ||
| }; | ||
| } | ||
| } | ||
| ); |
There was a problem hiding this comment.
The current implementation of the application builder returns a Promise<BuilderOutput> and uses a for await...of loop to wait for the generator returned by buildApplication to complete.
This has two major issues:
- Breaks Watch Mode: In watch mode (
--watch),buildApplicationis an infinite async generator that yields new outputs on every file change. Because the builder returns a singlePromise, it will never resolve (or will only resolve on the first failure), causing the build process to hang or fail to report subsequent successful builds. - Discards Metadata: Returning
{ success: true }discards important metadata returned bybuildApplication(such asoutputPaths,configuration, etc.) which might be used by other builders or integration tools.
Recommendation:
Change the builder handler to an async generator (async function*) and use yield* to forward all outputs directly to the architect. This fully supports watch mode and preserves all builder output metadata.
Additionally, index.ts files should primarily serve as re-export barrels. Since this file contains builder execution logic, consider renaming it to a more descriptive name (e.g., builder.ts or main.ts) to clarify its purpose and avoid using index.ts for executing code.
export default createBuilder<ApplicationOptions>(
async function* (options, context) {
const plugins = options.plugins ? await loadPlugins(options.plugins, context) : [];
const buildOptions = { ...options };
delete buildOptions.plugins;
try {
yield* buildApplication(
buildOptions as ApplicationBuilderOptions,
context,
{ codePlugins: plugins }
);
} catch (error) {
const errorMessage = error instanceof Error ? error.message : String(error);
context.logger.error(`Build failed: ${errorMessage}`);
yield {
success: false,
error: errorMessage
};
}
}
);References
- Do not introduce side-effect imports in index.ts files. index.ts files should primarily serve as re-export barrels. If an index.ts file needs to execute code, consider renaming it to a more descriptive name (e.g., main.ts or extract.ts) to clarify its purpose and avoid bypassing the intent of pure re-export barrels.
| let plugin: Plugin | Plugin[] | ((...args: unknown[]) => Plugin | Plugin[]); | ||
|
|
||
| if (module.default) { | ||
| plugin = module.default; | ||
| } else if (typeof module === 'function') { | ||
| plugin = module; | ||
| } else { | ||
| plugin = module; | ||
| } |
There was a problem hiding this comment.
The dynamic import(moduleUrl) statement always returns a Module Namespace Object, which is of type 'object'. Therefore, the check typeof module === 'function' is dead code and will never be true.
Furthermore, if module.default is not defined, the code falls back to setting plugin = module (the namespace object itself). Pushing a Module Namespace Object directly to the plugins array will cause esbuild to fail because it is not a valid plugin object (it lacks the required name and setup properties).
Since both ES modules (via export default) and CommonJS modules (via module.exports = ...) resolve their main export to module.default when dynamically imported in Node.js, we should strictly require and use module.default.
const plugin = module.default;
if (!plugin) {
throw new Error(`Module ${pluginPath} does not have a default export.`);
}
michael-smt
left a comment
There was a problem hiding this comment.
I might be out of the loop, but what's the reason for this? 🙂
| "plugins": { | ||
| "type": "array", | ||
| "description": "List of paths to esbuild plugins. Each path should resolve to a module that exports an esbuild Plugin or Plugin array.", | ||
| "items": { | ||
| "type": "string" | ||
| }, | ||
| "default": [] | ||
| }, |
There was a problem hiding this comment.
As far as I understand this is a copy of the Angular builder's schema extended with this section. Generating this file might be better, so we don't have to remember to keep this file up-to-date with Angular changes.
There was a problem hiding this comment.
Same here as with the application schema, I think this would be better if copied at builder's build time.
b735a4d to
772d849
Compare
5e128a0 to
a8230a8
Compare
No description provided.