Repository navigation
feat(bundler-plugins): Enable hidden source maps and remove them after upload in the Rollup plugin - #25645
feat(bundler-plugins): Enable hidden source maps and remove them after upload in the Rollup plugin#25645timfish wants to merge 1 commit into
Conversation
size-limit report 📦Compared: dfb5047 (PR test merge using develop at a3dae70) Against: a3dae70 (develop) · base run #64482 ✅ Baseline matches: this diff contains only this PR's changes.
|
aaa1a70 to
02c2650
Compare
Optional E2E testsSome optional checks were not successful. They do not block the merge. 2 not successful, 52 successful. See the run.
Successful checks
|
02c2650 to
d86c896
Compare
d86c896 to
bc36a67
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
bc36a67 to
b325be7
Compare
b325be7 to
846b3e0
Compare
846b3e0 to
9a1327f
Compare
ce7fa61 to
17d0fb2
Compare
17d0fb2 to
a80e4ec
Compare
Replaced by a newer auto-merge request
| it('warns when source maps are turned off', async () => { | ||
| const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => undefined); | ||
| const build = await rollup({ input: entry, plugins: [sentryRollupPlugin({ ...credentials, silent: false })] }); | ||
| await build.write({ dir: outDir, sourcemap: false }); | ||
|
|
||
| expect(fs.readdirSync(outDir)).toEqual(['entry.js']); | ||
| expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('Source map generation is turned off')); | ||
| }); |
There was a problem hiding this comment.
I imagine this could be noisy for someone that wants to just disable sourcemaps?
There was a problem hiding this comment.
The warning fires once per plugin instance per build. The manager dedupes by message, so several outputs or several writeBundle calls log it once. It fires only when an upload could run, ie. not in development mode, and only with auth token, org and project set.
I think if users have sourcemaps disabled but are using our plugin with everything configured, we should be warning them.
bff6f89 to
3f92231
Compare
3f92231 to
17702f7
Compare
17702f7 to
56f613c
Compare
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
The approach is sound. It beats writing maps and deleting them with globs afterward: no window where maps sit on disk, no glob mistakes, no stale map pairing. The staging lifecycle (signatures, keep(), discard()) handles watch mode and failed writes with care.
The weak point is the state that links outputOptions to generateBundle. It is keyed by directory, and that is not specific enough once outputs run in parallel. The staging path also skips the upload filters the glob path applies, so assets and ignore behave differently depending on whether the user set sourcemap.
Fix the issues below, and it's good to land imo.
| const decision = sentryBuildPluginManager.sourceMaps.decide( | ||
| sourceMapKindFromRollupSetting(rawOutputOptions.sourcemap), | ||
| ); | ||
| decisions.set(getOutputDestination(rawOutputOptions), decision); |
There was a problem hiding this comment.
With this, it looks like, parallel outputs to the same directory share one decision, so the plugin deletes the user's maps or leaks its own, because decisions is keyed only by output directory. The Rollup CLI writes all outputs at once in a Promise.all. So every outputOptions hook runs before any generateBundle, and the last output's decision wins for all outputs in that directory. Writing ESM and CJS side by side into dist/ is a common library setup.
Reproduced with Rollup 4 and the built plugin, two outputs into one dir:
[{ format: "es", entryFileNames: "[name].mjs", sourcemap: true }, { format: "cjs", entryFileNames: "[name].cjs" }]: the user's.mjs.mapfiles are deleted, althoughdeleteSourcemapsAfterUploadis unset. The.mjsfiles still end in//# sourceMappingURL=entry1.mjs.map, so the reference now points at a missing file. The changelog says "Source maps you enabled yourself stay in the build output, as before."- Same two outputs in reverse order: the plugin turns on hidden maps for the CJS output, but the decision for the directory is now the ESM one (keep). The hidden
.cjs.mapfiles the plugin created are written to disk and kept. That's what we're trying to prevent.
The unit test "uploads each output on its own" writes one output after the other into different directories, so it didn't catch this.
Suggestion: key the decision by output, not by directory. One option: in generateBundle, read the normalized outputOptions.sourcemap. If it is 'hidden' and the plugin set it, the plugin owns those maps. To tell "the plugin set hidden" apart from "the user set hidden", record the raw setting per output in a key that includes format and the entryFileNames/file pattern along with the directory, or keep a list per directory and match on the normalized options. Add a test with Promise.all([build.write(a), build.write(b)]) to one directory, with mixed settings, in both orders.
| continue; | ||
| } | ||
|
|
||
| const result = await staging.stage({ |
There was a problem hiding this comment.
Staging ignores sourcemaps.assets and sourcemaps.ignore. stageSourceMaps stages every JS chunk that has a map. Then uploadSourcemaps drops staged chunks from the glob result (build-plugin-manager.ts line 720), so assets and ignore never apply to them.
Reproduced with no sourcemap setting (the new default path):
ignore: ["**/entry2.js"]:entry2.jsis uploaded anyway, and its map is removed from the output.assets: ["<out>/entry1.js"]: all three chunks are uploaded.
Users set these options on purpose, for example to keep vendor or third-party chunks out of Sentry. Now they are silently ignored as soon as the user upgrades and has no sourcemap setting.
Suggestion: before staging a chunk, test its final path (path.join(destination, output.fileName)) against assets and ignore with the same matcher that globFiles uses. For a chunk that does not match, either keep the map in the bundle (if the plugin did not turn maps on, which cannot happen on this path) or drop it without uploading. Dropping it is probably right when the plugin turned on hidden maps: the user did not ask for maps, and they asked not to upload that chunk. Whichever you pick, write it down in the option docs. #25916 adds integration fixtures for these options, but they turn on source maps explicitly and never reach staging. Add staged variants (no sourcemap setting) there or here.
EDIT: Oh, I see Bugbot flagged this also.
| ); | ||
| decisions.set(getOutputDestination(rawOutputOptions), decision); | ||
|
|
||
| return decision.enableHidden ? { ...rawOutputOptions, sourcemap: 'hidden' } : null; |
There was a problem hiding this comment.
This is gonna return map assets that hte user didn't ask for, and log a false message.
outputOptions runs for generate() too, so the output switches to hidden maps. stageSourceMaps then returns early for isWrite === false (line 93), so the maps stay in the result.
Reproduced: bundle.generate({ dir }) with no sourcemap setting returns ['entry1.js', 'entry1.js.map'] and logs "Enabling hidden source maps. They are uploaded to Sentry and removed from the build output." Neither half is true: nothing is uploaded, and nothing is removed. Tools that call generate() and write the result themselves would now ship maps.
The outputOptions hook can't see whether the call is write or generate. One option: in stageSourceMaps, when isWrite === false and the plugin turned on hidden maps (decision kind unset), still delete the map assets without staging them, so generate() returns what the user configured. At minimum, change the log so it does not promise an upload or removal for generate().
| const result = await staging.stage({ | ||
| code: output.code, | ||
| mapSource: mapAsset.source, | ||
| mapDir: path.join(destination, path.dirname(output.fileName)), |
There was a problem hiding this comment.
This is the chunk's directory, not the map's. The disk path passes path.dirname(sourceMapPath) as mapDir (debug-id-upload.ts line 66). Map sources are relative to the map file. With output.sourcemapFileNames set to put maps in another directory (for example maps/[name].js.map), staging passes the chunk's directory instead. A custom rewriteSources hook that resolves sources against mapDir then gets a different value depending on which path ran. The default hook does not use mapDir, so most users will not notice.
stagedChunkPaths is built from path.resolve(input.mapDir, basename(fileName)) (build-plugin-manager.ts line 890), so changing mapDir alone would break that de-dupe. The two meanings are tangled.
Suggestion: pass the chunk path and the map directory to stage() as separate fields: mapDir: path.join(destination, path.dirname(mapFileName)) for the hook, and the chunk's absolute path for stagedChunkPaths. Verify with a test that uses sourcemapFileNames.
| "sentry-cli-mock.json": "["release","create","CURRENT_SHA","--project","fake-project"], | ||
| ["release","set-commits","CURRENT_SHA","--auto"], | ||
| ["release","finalize","CURRENT_SHA"], | ||
| ["sourcemap","upload","-p","fake-project","--release","CURRENT_SHA","sentry-bundler-plugin-upload-path"], |
There was a problem hiding this comment.
The only integration changes are an extra sourcemap upload line in the basic, basic-cjs and release-disabled snapshots. Unit tests cover the staging in depth, but none uses the Rollup CLI or several outputs, which is how concern 1 shows up in practice.
Suggestion: add one integration fixture with an array of outputs into one directory and mixed sourcemap settings. It runs through the real CLI path and would catch the note on packages/bundler-plugins/src/rollup/source-map-handling.ts line 82
56f613c to
8d0bfe5
Compare
7e4a6af to
a471e8c
Compare
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 a471e8c. Configure here.
11b221f to
d0c124d
Compare
…r upload in the Rollup plugin When the Rollup or Rolldown output has no `sourcemap` setting and an upload can run, the plugin switches the output to hidden source maps, copies each chunk and its map into the upload staging folder from `generateBundle`, and drops the map asset so it is never written. `deleteSourcemapsAfterUpload: true` does the same for maps the user enabled. SvelteKit and Nuxt pass `deleteSourcemapsAfterUpload: false` because they need the map files on disk after the Vite build. The other framework SDKs set the bundler's source map option themselves, which the plugin never deletes, so their builds do not change.
d0c124d to
39b3cb3
Compare
Part of the stack for #25609. When the Rollup or Rolldown output has no
sourcemapsetting and an upload can run, the plugin switches the output to hidden source maps, copies each chunk and its map into the upload staging folder from agenerateBundlehook that runs last, and drops the map asset so it is never written.deleteSourcemapsAfterUpload: truedoes the same for maps the user enabled; a chunk the plugin cannot prepare keeps its map, with a warning.generate()drops the maps the plugin turned on, so callers that write the result themselves get what they configured.Decisions and stagings are recorded per output, told apart by
format,entryFileNames,chunkFileNamesandfile, because several outputs can write into one directory at once with different settings. Staging appliessourcemaps.assetsandsourcemaps.ignorethrough the matcher thatglobexports for its ownignoreoption, so it selects the same chunks as the upload from disk; an excluded chunk is not staged and keeps its map, because the plugin only removes maps it uploaded. Only JavaScript chunk maps are removed.SvelteKit and Nuxt pass
deleteSourcemapsAfterUpload: falsebecause they need the map files on disk after the Vite build; the other framework SDKs set the bundler's source map option themselves, which the plugin never deletes, so their builds do not change until each one migrates in its own PR. Integration fixtures coverignoreandassetson the staged path and mixed outputs into one directory; the CLI stub change comes from #25916 and drops out when that merges. The parser-unavailable tests get a 30 second timeout, because importing core took longer than five seconds on slow runners.