fix(@angular/build): prepend deploy-url to file loader output paths#32937
fix(@angular/build): prepend deploy-url to file loader output paths#32937maruthang wants to merge 1 commit intoangular:mainfrom
Conversation
When using the `file` loader (either via `loader` config or import attributes), the `deploy-url` was not prepended to the imported file's URL in the JavaScript output. This was because esbuild's global `publicPath` option cannot be used as it also affects code-splitting chunk paths. This change introduces a targeted esbuild plugin that post-processes the JS output to prepend the publicPath (deploy-url) only to file loader asset references, identified from the build metafile. This ensures code-splitting chunk paths remain unaffected. Fixes angular#32789
There was a problem hiding this comment.
Code Review
This pull request introduces a new esbuild plugin to prepend the publicPath (deploy-url) to file loader asset references in JavaScript bundles, ensuring assets are correctly linked when a deployment URL is used. The review feedback highlights several areas for improvement: expanding file extension support to include .mjs and .cjs, handling asset path resolution more accurately when files are in subdirectories, and optimizing the string replacement logic to improve performance and prevent partial path matching issues.
| for (const outputPath of Object.keys(result.metafile.outputs)) { | ||
| if ( | ||
| !outputPath.endsWith('.js') && | ||
| !outputPath.endsWith('.css') && | ||
| !outputPath.endsWith('.map') | ||
| ) { | ||
| assetPaths.push(outputPath); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for identifying assets should be updated to exclude other JavaScript extensions such as .mjs and .cjs. Currently, if a build produces .mjs files (which is common in server bundles or when outExtension is configured), they will be incorrectly treated as assets and potentially modified if referenced elsewhere.
| for (const outputPath of Object.keys(result.metafile.outputs)) { | |
| if ( | |
| !outputPath.endsWith('.js') && | |
| !outputPath.endsWith('.css') && | |
| !outputPath.endsWith('.map') | |
| ) { | |
| assetPaths.push(outputPath); | |
| } | |
| } | |
| for (const outputPath of Object.keys(result.metafile.outputs)) { | |
| if ( | |
| !/\.[cm]?js$/.test(outputPath) && | |
| !outputPath.endsWith('.css') && | |
| !outputPath.endsWith('.map') | |
| ) { | |
| assetPaths.push(outputPath); | |
| } | |
| } |
| if (!outputFile.path.endsWith('.js')) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This check should also include .mjs and .cjs files to ensure that asset references are updated in all JavaScript output formats, not just those ending in .js.
| if (!outputFile.path.endsWith('.js')) { | |
| continue; | |
| } | |
| if (!/\.[cm]?js$/.test(outputFile.path)) { | |
| continue; | |
| } |
| for (const assetPath of assetPaths) { | ||
| const originalRef = './' + assetPath; | ||
| const updatedRef = normalizedPublicPath + assetPath; | ||
|
|
||
| if (text.includes(originalRef)) { | ||
| text = text.replaceAll(originalRef, updatedRef); | ||
| modified = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current path matching and replacement logic is likely to fail when the build output is in a subdirectory. You should calculate the relative path from the JS file to the asset for matching, and strip the base output directory from assetPath when constructing the updatedRef.
| for (const assetPath of assetPaths) { | ||
| const originalRef = './' + assetPath; | ||
| const updatedRef = normalizedPublicPath + assetPath; | ||
|
|
||
| if (text.includes(originalRef)) { | ||
| text = text.replaceAll(originalRef, updatedRef); | ||
| modified = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Iterating over all assets and performing text.replaceAll for each one is inefficient (O(N*M)). Consider using a single regular expression to perform all replacements in a single pass. Additionally, sorting assetPaths by length in descending order is recommended to prevent incorrect partial replacements if one asset path is a prefix of another.
PR Checklist
PR Type
What is the current behavior?
When using esbuild's
fileloader (either vialoaderbuild option or import attributes), thedeploy-urlis not prepended to imported file URLs in JavaScript output. For example,import svgUrl from './image.svg'resolves to./media/image.svginstead of<deploy-url>/media/image.svg.This is because esbuild's global
publicPathoption cannot be set — it would also affect code-splitting chunk import paths, breaking lazy loading.Issue Number: #32789
What is the new behavior?
A targeted esbuild
onEndplugin post-processes JS output files to prependpublicPath(deploy-url) only to file loader asset references. Asset paths are identified from the build metafile (non-JS, non-CSS, non-sourcemap outputs). Code-splitting chunk paths remain unaffected.Example:
var svg_default = "./media/image.svg";var svg_default = "https://un5neftqgjkmem4kvumj8.julianrbryant.com/assets/media/image.svg";No regression risk: The plugin only modifies JS string references to media assets. It does not affect:
publicPathin the stylesheet bundler)ComponentStylesheetBundler)Does this PR introduce a breaking change?