Repository navigation
feat(bundler-plugins): Enable external source maps and delete them after upload in the esbuild plugin - #25648
Conversation
size-limit report 📦Compared: 7d21eb9 (PR test merge using develop at 41ac03e) Against: 4d85171 (develop) · base run #64969
|
0d05fc7 to
73fee34
Compare
Optional E2E testsSome optional checks were not successful. They do not block the merge. 4 not successful, 49 successful. See the run.
Successful checks
|
73fee34 to
4040760
Compare
3885821 to
62b848d
Compare
62b848d to
f5d5367
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
f5d5367 to
ba5d8ca
Compare
ba5d8ca to
6f13518
Compare
2b44493 to
71c0775
Compare
71c0775 to
0edb347
Compare
0edb347 to
5c128dc
Compare
5c128dc to
8d8e755
Compare
8d8e755 to
2b9c8fe
Compare
d661b73 to
2854a5a
Compare
2854a5a to
d8075e3
Compare
d8075e3 to
35e806c
Compare
35e806c to
84a11f9
Compare
84a11f9 to
6f443bc
Compare
Pull Request is not mergeable
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
esbuild offers no way to drop an output before it is written, so turning on external maps and deleting them after a successful upload is the only practical approach. Keeping maps when the upload fails is the safe default for maps the user asked for. The absWorkingDir path fix is a real improvement on its own.
The weak point is that the delete step guesses what was uploaded instead of being told. It also follows "delete only what was uploaded" even for maps the plugin created itself . In esbuild, where only entry points carry debug IDs, that guess leaves maps behind for shared chunks, lazy chunks and CSS, and leaves all maps behind on rebuilds (made visible by an older bug).
Have the upload report what it sent, delete all plugin-created maps on the unset path, and clear the wrapped-path set per build. With those changes, the esbuild plugin matches what the changelog says.
| if (hasDebugId(code)) { | ||
| uploaded.push(`${artifact}.map`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: when the decision came from kind === 'unset', the user never asked for any maps. After a successful upload, delete every .map in the metafile outputs, not just the uploaded ones. For maps the user enabled (deleteSourcemapsAfterUpload: true), keep the current rule of deleting only uploaded maps. Shared and lazy chunks without debug IDs are a separate, older gap. Could track it as follow-up work, since those chunks can't be symbolicated either way.
| // afterwards instead of being dropped from the output like in other bundlers. | ||
| if (sourceMapDecision.deleteAfterUpload && initialOptions.write !== false) { | ||
| const { shouldUpload } = sentryBuildPluginManager.sourceMaps; | ||
| const sourceMaps = await getUploadedSourceMaps(buildArtifacts, shouldUpload); |
There was a problem hiding this comment.
The delete list is calculated separately from what actually gets sent.
getUploadedSourceMaps rebuilds the upload's choices: isJsFile, a sibling .map in the metafile, shouldUpload, and hasDebugId. The upload makes its own choices with globFiles(assets ?? artifacts, { ignore }) and then prepareBundleForDebugIdUpload, which can skip a chunk (no map found by the resolveSourceMap hook, read error, and so on). upload() only returns one uploaded flag for the whole run. Wherever the two disagree, the plugin deletes a map that was never uploaded, and that copy is gone.
Suggestion: have upload() return the chunk paths it actually prepared and uploaded (it already has debugIdChunkFilePaths and the per-chunk results). Delete exactly the maps of those chunks. This removes getUploadedSourceMaps, hasDebugId and the extra file reads, and makes the deletion safe by construction.
| if (uploaded) { | ||
| await Promise.all(sourceMaps.map(sourceMap => fs.promises.rm(sourceMap, { force: true }))); | ||
| } else if (sourceMaps.length > 0) { | ||
| logger.error('The source maps were not uploaded, so they stay in the build output.'); |
There was a problem hiding this comment.
On rebuilds (context().rebuild(), watch mode) the default path leaves maps. Reproduced with esbuild.context() and two rebuild() calls:
- First rebuild: entry maps uploaded and deleted, as intended.
- Second rebuild:
e1.jshas no debug ID. The upload warns "No source maps found for any of the 4 matched build artifacts", ande1.js.mapande2.js.mapstay in the output. The "source maps were not uploaded" error does not fire, becausegetUploadedSourceMapsfinds no debug IDs and returns an empty list.
The debug ID loss is older than this PR. debugIdWrappedPaths is cleared in setup() (line 220), which runs once per context, not per build, so on a rebuild the onResolve hook skips every entry point as "already wrapped". I got the same "No source maps found" warning with an explicit sourcemap: 'external', so the upload regression is not new. What this PR adds is that the plugin now turns maps on for these builds and then leaves them on disk with no error.
Suggestion: clear debugIdWrappedPaths in an onStart hook instead of setup(). That fixes rebuild uploads and this PR's cleanup together. It is a one-line fix that could also go in its own fix(bundler-plugins) PR. A context().rebuild() test would catch this.
| }); | ||
| } | ||
|
|
||
| const sourceMapDecision = sentryBuildPluginManager.sourceMaps.decide( |
There was a problem hiding this comment.
With write: false, the plugin turns on maps it can never upload, and logs a false message.
The upload reads the metafile paths from disk. With write: false nothing is on disk, so nothing can be uploaded. The plugin still sets sourcemap: 'external', so callers that handle outputFiles themselves now get .map entries they never asked for. decide() also logs "Enabling hidden source maps. They are uploaded to Sentry and removed from the build output." at setup time, which is false here. This is the same problem #25645 fixed for Rollup's generate().
Suggestion: when initialOptions.write === false, skip the decision (or call decide(kind, { log: false }) and don't set sourcemap).
| if (options.sourcemaps?.disable !== 'disable-upload') { | ||
| await upload(buildArtifacts); | ||
| // The upload globs the paths, and glob reads backslashes as escapes. | ||
| const { uploaded } = await upload(buildArtifacts.map(artifact => artifact.split(path.sep).join('/'))); |
There was a problem hiding this comment.
I'm not sure why Bugbot resolved this, I think they're still globs? Shouldn't they get escaped with eg glob.escape() to ensure there's no * or ? chars to mess things up? Eg, what'd happen with a path like widget[id].js or something?
…ter upload in the esbuild plugin esbuild writes its output before `onEnd` runs and has no hook to drop an output, so the maps are deleted from disk after a successful upload instead. When the upload fails they stay, with an error log.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ecad382. Configure here.
| await Promise.all(sourceMaps.map(sourceMap => fs.promises.rm(sourceMap, { force: true }))); | ||
| } else if (sourceMaps.length > 0) { | ||
| logger.error('The source maps were not uploaded, so they stay in the build output.'); | ||
| } |
There was a problem hiding this comment.
Rebuilds leave generated source maps
Medium Severity
On incremental rebuilds the plugin still emits the external maps it turned on, but those rebuilds do not carry debug IDs, so upload reports a failure and the maps remain in the output. Users who never enabled source maps then get map files left on disk after the first rebuild() or watch pass.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit ecad382. Configure here.
Part of the stack for #25609. esbuild writes its output before
onEndruns and has no hook to drop an output, so the plugin setssourcemap: 'external'when the setting is unset and deletes the maps after a successful upload: every map in the metafile when the plugin turned them on, and exactly the maps the upload read from disk when the user enabled them, whichuploadSourcemapsnow reports. When the upload fails all maps stay, with an error log. Withwrite: falsethe setting is left alone, because nothing reaches the disk.The metafile lists outputs relative to
absWorkingDir, while the upload globbed them from the process working directory, so builds with anabsWorkingDirelsewhere uploaded nothing. The paths are now resolved againstabsWorkingDirfirst, and passed with forward slashes because glob reads backslashes as escapes. Integration fixtures coverignoreandassetson the staged path for esbuild.