feat(remix): Delete source maps through the bundler plugin instead of a glob

dev-packages/e2e-tests/test-applications/remix-sourcemaps/assert-build.ts

@@ -1,7 +1,7 @@
 import * as assert from 'assert/strict';
 import * as fs from 'fs';
 import * as path from 'path';
-import { assertSourceMapUpload } from '@sentry-internal/test-utils';
+import { assertSourceMapUpload, findSourceMapFiles, findSourceMappingUrlComments } from '@sentry-internal/test-utils';
 
 const BUILD_DIR = 'build';
 const CLIENT_OUTPUT = path.join(BUILD_DIR, 'client');
@@ -39,13 +39,21 @@ for (const chunk of chunks) {
 }
 console.log(`no client chunk carries more than one debug ID (${chunks.length} chunks)\n`);
 
-// The plugin defaults `filesToDeleteAfterUpload` when the app configures no source map setting. Remix
-// runs a client and an SSR pass with different `outDir`s, while the deletion glob is held in a promise
-// that settles once, so a leftover `.map` means the second pass was left behind.
-assertSourceMapUpload({
-  outputDirs: [CLIENT_OUTPUT, SERVER_OUTPUT],
-  strippedDirs: [BUILD_DIR],
-  markers: { 'client source': 'SOURCEMAP_MARKER_CLIENT', 'server source': 'SOURCEMAP_MARKER_SERVER' },
-});
+// The SDK turns on hidden source maps when the app sets none, and asks the bundler plugin to
+// remove them from both the client and the SSR output, also when no upload runs.
+if (process.env.E2E_NO_UPLOAD) {
+  assert.deepEqual(findSourceMapFiles({ outputDir: BUILD_DIR }), [], `Expected no source maps in ${BUILD_DIR}`);
+  assert.deepEqual(
+    findSourceMappingUrlComments({ outputDir: BUILD_DIR }),
+    [],
+    `Expected no sourceMappingURL comments in ${BUILD_DIR}`,
+  );
+} else {
+  assertSourceMapUpload({
+    outputDirs: [CLIENT_OUTPUT, SERVER_OUTPUT],
+    strippedDirs: [BUILD_DIR],
+    markers: { 'client source': 'SOURCEMAP_MARKER_CLIENT', 'server source': 'SOURCEMAP_MARKER_SERVER' },
+  });
+}
 
 console.log('All sourcemap assertions passed!');

dev-packages/e2e-tests/test-applications/remix-sourcemaps/package.json

@@ -6,7 +6,9 @@
     "build": "node start-mock-sentry-server.mjs & remix vite:build > .tmp_build_stdout 2> .tmp_build_stderr; BUILD_EXIT=$?; kill %1 2>/dev/null; cat .tmp_build_stdout .tmp_build_stderr; exit $BUILD_EXIT",
     "clean": "npx rimraf node_modules pnpm-lock.yaml",
     "test:build": "pnpm install && pnpm build",
-    "test:assert": "pnpm tsx assert-build.ts"
+    "test:build:no-upload": "pnpm install && E2E_NO_UPLOAD=1 pnpm build",
+    "test:assert": "pnpm tsx assert-build.ts",
+    "test:assert:no-upload": "E2E_NO_UPLOAD=1 pnpm test:assert"
   },
   "dependencies": {
     "@sentry/remix": "file:../../packed/sentry-remix-packed.tgz",
@@ -26,5 +28,14 @@
     "tsx": "^4.23.0",
     "typescript": "^5.1.6",
     "vite": "^5.4.11"
+  },
+  "sentryTest": {
+    "variants": [
+      {
+        "build-command": "pnpm test:build:no-upload",
+        "assert-command": "pnpm test:assert:no-upload",
+        "label": "remix-sourcemaps (no upload)"
+      }
+    ]
   }
 }

dev-packages/e2e-tests/test-applications/remix-sourcemaps/vite.config.ts

@@ -8,7 +8,7 @@ export default defineConfig({
       ignoredRouteFiles: ['**/.*'],
     }),
     sentryRemixVitePlugin({
-      authToken: 'fake-auth-token',
+      authToken: process.env.E2E_NO_UPLOAD ? undefined : 'fake-auth-token',
       org: 'test-org',
       project: 'test-project',
       sentryUrl: 'http://localhost:3033',

packages/remix/src/vite/index.ts

@@ -1,6 +1,7 @@
 import type { Plugin } from 'vite';
 import { makeOrchestrionPlugin } from './orchestrionPlugin';
 import { makeRouteManifestPlugin } from './routeManifestPlugin';
+import type { SourceMapState } from './sourceMaps';
 import { makeAddSentryVitePlugin, makeEnableSourceMapsPlugin } from './sourceMaps';
 import type { SentryRemixVitePluginOptions } from './types';
 
@@ -42,18 +43,20 @@ export function sentryRemixVitePlugin(options: SentryRemixVitePluginOptions = {}
     return plugins;
   }
 
-  // Added even when source maps are disabled: the bundler plugin also applies bundle size
-  // optimizations, module metadata, the application key and release management, and it already
-  // skips the upload itself.
-  //
-  // Order matters: Vite passes the already-merged config to every `config` hook, so the deletion
-  // plugin has to read `build.sourcemap` before `makeEnableSourceMapsPlugin` sets it to 'hidden'.
-  plugins.push(...makeAddSentryVitePlugin(options));
+  const sourceMapState: SourceMapState = { sdkEnabledSourceMaps: false };
 
   // `'disable-upload'` still generates them - debug IDs are injected and the user uploads by hand.
+  //
+  // Order matters: the bundler plugin decides on the `build.sourcemap` it finds in its own
+  // `config` hook, so this plugin has to set it first.
   if (options.sourcemaps?.disable !== true) {
-    plugins.push(makeEnableSourceMapsPlugin(options));
+    plugins.push(makeEnableSourceMapsPlugin(options, sourceMapState));
   }
 
+  // Added even when source maps are disabled: the bundler plugin also applies bundle size
+  // optimizations, module metadata, the application key and release management, and it already
+  // skips the upload itself.
+  plugins.push(...makeAddSentryVitePlugin(options, sourceMapState));
+
   return plugins;
 }

packages/remix/src/vite/sourceMaps.ts

@@ -1,14 +1,20 @@
-import * as path from 'path';
 import { sentryVitePlugin } from '@sentry/bundler-plugins/vite';
 import type { Plugin, UserConfig } from 'vite';
 import type { SentryRemixVitePluginOptions } from './types';
 
-type FilesToDeleteAfterUpload = string | string[] | undefined;
+/** Shared by the two plugins of one build. */
+export type SourceMapState = {
+  /** The SDK turned on hidden source maps because the user did not set `build.sourcemap`. */
+  sdkEnabledSourceMaps: boolean;
+};
 
 /**
  * Adds the Sentry bundler plugin, which injects debug IDs and uploads source maps to Sentry.
  */
-export function makeAddSentryVitePlugin(options: SentryRemixVitePluginOptions): Plugin[] {
+export function makeAddSentryVitePlugin(
+  options: SentryRemixVitePluginOptions,
+  state: SourceMapState = { sdkEnabledSourceMaps: false },
+): Plugin[] {
   const {
     applicationKey,
     authToken,
@@ -26,59 +32,7 @@ export function makeAddSentryVitePlugin(options: SentryRemixVitePluginOptions):
     telemetry,
   } = options;
 
-  // The default depends on `build.sourcemap`, which is only known once Vite resolves the config.
-  let resolveFilesToDeleteAfterUpload: ((value: FilesToDeleteAfterUpload) => void) | undefined;
-  const filesToDeleteAfterUploadPromise = new Promise<FilesToDeleteAfterUpload>(resolve => {
-    resolveFilesToDeleteAfterUpload = resolve;
-  });
-
-  const configPlugin: Plugin = {
-    name: 'sentry-remix-files-to-delete-after-upload',
-    apply: 'build',
-    enforce: 'post',
-    config(config) {
-      const userFilesToDelete = sourcemaps?.filesToDeleteAfterUpload;
-
-      // Only clean up after ourselves: if the user asked for source maps, they are theirs to keep.
-      // Scoped to the build output rather than `./**/*.map`, which the bundler plugin globs without
-      // ignoring `node_modules` and then deletes with `force: true`.
-      //
-      // `disable: 'disable-upload'` injects debug IDs but leaves uploading to the user, and the
-      // bundler plugin deletes in a `finally` block even when it skipped the upload - so defaulting
-      // the deletion there would remove the very maps they still have to upload by hand.
-      if (
-        typeof userFilesToDelete === 'undefined' &&
-        typeof config.build?.sourcemap === 'undefined' &&
-        !sourcemaps?.disable
-      ) {
-        // Derived from the configured `outDir` rather than hardcoding `build`, so a custom Remix
-        // `buildDirectory` still gets cleaned up. Remix runs a client and an SSR build, each with
-        // its own `outDir`, so each pass deletes only what it emitted.
-        //
-        // Separators are normalised because `glob` reads a backslash as an escape, and both path
-        // flavours are checked because `path.isAbsolute` only recognises the host platform's form.
-        // Only a relative path may take the `./` prefix: `.//abs/path/**/*.map` matches nothing.
-        const outDir = (config.build?.outDir || 'build').replace(/\\/g, '/').replace(/\/+$/, '');
-        const isAbsoluteOutDir = path.posix.isAbsolute(outDir) || path.win32.isAbsolute(outDir);
-        const outDirGlob = isAbsoluteOutDir ? outDir : `./${outDir.replace(/^\.\//, '')}`;
-        const filesToDelete = [`${outDirGlob}/**/*.map`];
-
-        if (debug) {
-          // eslint-disable-next-line no-console
-          console.log(
-            `[Sentry] Automatically setting \`sourcemaps.filesToDeleteAfterUpload: ${JSON.stringify(
-              filesToDelete,
-            )}\` to delete generated source maps after they were uploaded to Sentry.`,
-          );
-        }
-        resolveFilesToDeleteAfterUpload?.(filesToDelete);
-      } else {
-        resolveFilesToDeleteAfterUpload?.(userFilesToDelete);
-      }
-    },
-  };
-
-  const sentryPlugins = sentryVitePlugin({
+  return sentryVitePlugin({
     applicationKey,
     authToken: authToken ?? process.env.SENTRY_AUTH_TOKEN,
     bundleSizeOptimizations,
@@ -96,7 +50,13 @@ export function makeAddSentryVitePlugin(options: SentryRemixVitePluginOptions):
       ignore: sourcemaps?.ignore,
       rewriteSources: sourcemaps?.rewriteSources,
       resolveSourceMap: sourcemaps?.resolveSourceMap,
-      filesToDeleteAfterUpload: filesToDeleteAfterUploadPromise,
+      filesToDeleteAfterUpload: sourcemaps?.filesToDeleteAfterUpload,
+      // Called when the bundler plugin decides, after `makeEnableSourceMapsPlugin` ran.
+      // With `disable: 'disable-upload'` the user uploads the maps by hand, so they stay.
+      deleteSourcemapsAfterUpload: () =>
+        sourcemaps?.deleteSourcemapsAfterUpload ??
+        (state.sdkEnabledSourceMaps && !sourcemaps?.disable ? true : undefined),
+      sdkEnabledSourceMaps: () => state.sdkEnabledSourceMaps,
     },
     telemetry: telemetry ?? true,
     url: sentryUrl,
@@ -106,25 +66,26 @@ export function makeAddSentryVitePlugin(options: SentryRemixVitePluginOptions):
       },
     },
   });
-
-  return [configPlugin, ...sentryPlugins];
 }
 
 /**
  * Enables "hidden" source maps if the user did not configure them.
  */
-export function makeEnableSourceMapsPlugin(options: SentryRemixVitePluginOptions): Plugin {
+export function makeEnableSourceMapsPlugin(
+  options: SentryRemixVitePluginOptions,
+  state: SourceMapState = { sdkEnabledSourceMaps: false },
+): Plugin {
   return {
     name: 'sentry-remix-update-source-map-setting',
     apply: 'build',
     enforce: 'post',
     // Returning only the changed key: Vite concatenates arrays when merging a `config` return
     // value, so echoing the whole config back duplicates `ssr.noExternal`, `resolve.alias` etc.
     config(viteConfig) {
+      const sourcemap = getUpdatedSourceMapSettings(viteConfig, options);
+      state.sdkEnabledSourceMaps = sourcemap !== viteConfig.build?.sourcemap;
       return {
-        build: {
-          sourcemap: getUpdatedSourceMapSettings(viteConfig, options),
-        },
+        build: { sourcemap },
       };
     },
   };
@@ -141,7 +102,7 @@ export function makeEnableSourceMapsPlugin(options: SentryRemixVitePluginOptions
  *
  *     3. Users didn't set source maps generation
  *       - we enable 'hidden' source maps generation
- *       - configure `filesToDeleteAfterUpload` to delete all .map files (we emit a log about this)
+ *       - pass `deleteSourcemapsAfterUpload: true`, so the maps never reach the build output
  *
  * --> only exported for testing
  */

packages/remix/test/vite/index.test.ts

@@ -31,23 +31,32 @@ vi.mock('@sentry/server-utils/orchestrion/vite', () => ({
 type CapturedSentryOptions = {
   applicationKey?: string;
   bundleSizeOptimizations?: { excludeTracing?: boolean };
-  sourcemaps?: { filesToDeleteAfterUpload?: Promise<unknown> };
+  sourcemaps?: { deleteSourcemapsAfterUpload?: boolean | (() => boolean | undefined) };
 };
 
 let capturedSentryOptions: CapturedSentryOptions | undefined;
+let deleteOptionInConfigHook: boolean | undefined | 'not called';
+
+function getDeleteOption(): boolean | undefined {
+  const value = capturedSentryOptions?.sourcemaps?.deleteSourcemapsAfterUpload;
+  return typeof value === 'function' ? value() : value;
+}
 
 vi.mock('@sentry/bundler-plugins/vite', () => ({
   sentryVitePlugin: (options: CapturedSentryOptions) => {
     capturedSentryOptions = options;
-    return [{ name: 'sentry-vite-plugin' }];
+    return [
+      {
+        name: 'sentry-vite-plugin',
+        config: () => {
+          deleteOptionInConfigHook = getDeleteOption();
+        },
+      },
+    ];
   },
 }));
 
-const SOURCE_MAP_PLUGINS = [
-  'sentry-remix-files-to-delete-after-upload',
-  'sentry-vite-plugin',
-  'sentry-remix-update-source-map-setting',
-];
+const SOURCE_MAP_PLUGINS = ['sentry-remix-update-source-map-setting', 'sentry-vite-plugin'];
 
 const NODE_CONFIG = { ssr: { target: 'node' } } as UserConfig;
 const WORKER_CONFIG = { ssr: { target: 'webworker' } } as UserConfig;
@@ -73,15 +82,10 @@ describe('sentryRemixVitePlugin', () => {
     vi.clearAllMocks();
     vi.unstubAllEnvs();
     capturedSentryOptions = undefined;
+    deleteOptionInConfigHook = 'not called';
   });
 
-  // Vite hands every `config` hook the already-merged config, so the deletion plugin has to read
-  // `build.sourcemap` before `makeEnableSourceMapsPlugin` sets it. Running the hooks in isolation
-  // hides that, which is why this drives them in plugin order over one shared config.
-  it('still deletes the generated source maps once the hooks run in plugin order', async () => {
-    const plugins = sentryRemixVitePlugin();
-    const config: UserConfig = {};
-
+  function runConfigHooks(plugins: ReturnType<typeof sentryRemixVitePlugin>, config: UserConfig): UserConfig {
     for (const plugin of plugins) {
       if (!plugin.config) {
         continue;
@@ -94,8 +98,22 @@ describe('sentryRemixVitePlugin', () => {
         config.build = { ...config.build, sourcemap };
       }
     }
+    return config;
+  }
+
+  // The bundler plugin reads the option when its own `config` hook runs, which comes after ours.
+  it('enables hidden source maps and asks the bundler plugin to delete them', () => {
+    const config = runConfigHooks(sentryRemixVitePlugin(), {});
+
+    expect(config.build?.sourcemap).toBe('hidden');
+    expect(deleteOptionInConfigHook).toBe(true);
+  });
+
+  it('keeps source maps the user enabled', () => {
+    const config = runConfigHooks(sentryRemixVitePlugin(), { build: { sourcemap: 'hidden' } });
 
-    await expect(capturedSentryOptions?.sourcemaps?.filesToDeleteAfterUpload).resolves.toEqual(['./build/**/*.map']);
+    expect(config.build?.sourcemap).toBe('hidden');
+    expect(deleteOptionInConfigHook).toBeUndefined();
   });
 
   it('returns the route manifest, orchestrion and source map plugins', () => {
@@ -127,7 +145,6 @@ describe('sentryRemixVitePlugin', () => {
     expect(plugins.map(plugin => plugin.name)).toEqual([
       'sentry-remix-route-manifest',
       'code-transformer',
-      'sentry-remix-files-to-delete-after-upload',
       'sentry-vite-plugin',
     ]);
   });