diff --git a/packages/react-router/src/vite/buildEnd/handleOnBuildEnd.ts b/packages/react-router/src/vite/buildEnd/handleOnBuildEnd.ts index 2cdd2c5cd09b..59e32fe1918c 100644 --- a/packages/react-router/src/vite/buildEnd/handleOnBuildEnd.ts +++ b/packages/react-router/src/vite/buildEnd/handleOnBuildEnd.ts @@ -16,6 +16,20 @@ function getSentryConfig(viteConfig: unknown): SentryReactRouterBuildOptions { return (viteConfig as { sentryConfig: SentryReactRouterBuildOptions }).sentryConfig; } +/** + * This hook is the only place that injects debug IDs and uploads source maps for React + * Router, so `disable` has to be honoured wherever the user set it. Reading it from the + * top-level config only would silently ignore `unstable_sentryVitePluginOptions`. + */ +function resolveSourceMapsDisable(sentryConfig: SentryReactRouterBuildOptions): boolean | 'disable-upload' | undefined { + // eslint-disable-next-line typescript/no-deprecated + if (sentryConfig.sourceMapsUploadOptions?.enabled === false) { + return true; + } + + return sentryConfig.sourcemaps?.disable ?? sentryConfig.unstable_sentryVitePluginOptions?.sourcemaps?.disable; +} + /** * A build end hook that handles Sentry release creation and source map uploads. * It creates a new Sentry release if configured, uploads source maps to Sentry, @@ -48,8 +62,7 @@ export const sentryOnBuildEnd: BuildEndHook = async ({ reactRouterConfig, viteCo ...unstableSentryVitePluginOptions?.sourcemaps, ...sentryConfig.sourcemaps, ...sourceMapsUploadOptions, - // eslint-disable-next-line typescript/no-deprecated - disable: sourceMapsUploadOptions?.enabled === false ? true : sentryConfig.sourcemaps?.disable, + disable: resolveSourceMapsDisable(sentryConfig), }, release: { ...unstableSentryVitePluginOptions?.release, @@ -80,7 +93,12 @@ export const sentryOnBuildEnd: BuildEndHook = async ({ reactRouterConfig, viteCo } } - if (!sourcemaps?.disable && viteConfig.build.sourcemap !== false) { + // `disable: 'disable-upload'` still injects debug IDs, so that source maps can be + // uploaded manually at a later point - only `true` turns source maps off entirely. + const sourceMapsFullyDisabled = sourcemaps?.disable === true; + const uploadDisabled = sourceMapsFullyDisabled || sourcemaps?.disable === 'disable-upload'; + + if (!sourceMapsFullyDisabled && viteConfig.build.sourcemap !== false) { // inject debugIds try { await cliInstance.execute( @@ -92,21 +110,30 @@ export const sentryOnBuildEnd: BuildEndHook = async ({ reactRouterConfig, viteCo console.error('[Sentry] Could not inject debug ids', error); } - // upload sourcemaps - try { - await cliInstance.releases.uploadSourceMaps(release?.name || 'undefined', { - include: [ - { - paths: [reactRouterConfig.buildDirectory], - }, - ], - live: 'rejectOnError', - }); - } catch (error) { - // eslint-disable-next-line no-console - console.error('[Sentry] Could not upload sourcemaps', error); + if (!uploadDisabled) { + // upload sourcemaps + try { + await cliInstance.releases.uploadSourceMaps(release?.name || 'undefined', { + include: [ + { + paths: [reactRouterConfig.buildDirectory], + }, + ], + live: 'rejectOnError', + }); + } catch (error) { + // eslint-disable-next-line no-console + console.error('[Sentry] Could not upload sourcemaps', error); + } } } + + // Only clean up source maps that were actually uploaded. Deleting them after skipping + // the upload would leave the user with neither, breaking a manual upload. + if (uploadDisabled) { + return; + } + // delete sourcemaps after upload let updatedFilesToDeleteAfterUpload = await sourcemaps?.filesToDeleteAfterUpload; diff --git a/packages/react-router/src/vite/makeCustomSentryVitePlugins.ts b/packages/react-router/src/vite/makeCustomSentryVitePlugins.ts index b2d41378db33..9d8c0a8a7ceb 100644 --- a/packages/react-router/src/vite/makeCustomSentryVitePlugins.ts +++ b/packages/react-router/src/vite/makeCustomSentryVitePlugins.ts @@ -19,6 +19,20 @@ export async function makeCustomSentryVitePlugins(options: SentryReactRouterBuil release, } = options; + const unstableSourcemapsDisable = unstable_sentryVitePluginOptions?.sourcemaps?.disable; + + // Anything other than `true` would have the Vite plugin inject debug IDs on top of the + // ones `sentryOnBuildEnd` injects, which breaks source map resolution. The value still + // applies to the buildEnd hook - only the Vite plugin ignores it. + if (unstableSourcemapsDisable !== undefined && unstableSourcemapsDisable !== true) { + // eslint-disable-next-line no-console + console.warn( + `[Sentry] \`unstable_sentryVitePluginOptions.sourcemaps.disable: ${JSON.stringify( + unstableSourcemapsDisable, + )}\` does not apply to the Vite plugin. Debug ID injection and source map upload are handled by the \`sentryOnBuildEnd\` hook for React Router, so letting the Vite plugin do it as well would inject a second debug ID per chunk. The option still applies to \`sentryOnBuildEnd\`.`, + ); + } + const sentryVitePlugins = sentryVitePlugin({ applicationKey, authToken: authToken ?? process.env.SENTRY_AUTH_TOKEN, @@ -27,27 +41,38 @@ export async function makeCustomSentryVitePlugins(options: SentryReactRouterBuil org: org ?? process.env.SENTRY_ORG, project: project ?? process.env.SENTRY_PROJECT, telemetry: telemetry ?? true, + // Spread here so it can override the plain options above, but not the objects + // merged below - object spread replaces whole keys rather than deep-merging. + ...unstable_sentryVitePluginOptions, _metaOptions: { + ...unstable_sentryVitePluginOptions?._metaOptions, telemetry: { + ...unstable_sentryVitePluginOptions?._metaOptions?.telemetry, metaFramework: 'react-router', }, - ...unstable_sentryVitePluginOptions?._metaOptions, }, reactComponentAnnotation: { - enabled: reactComponentAnnotation?.enabled ?? undefined, - ignoredComponents: reactComponentAnnotation?.ignoredComponents ?? undefined, + // Only assign when set, as an explicit `undefined` would erase the unstable value + ...(reactComponentAnnotation?.enabled !== undefined && { enabled: reactComponentAnnotation.enabled }), + ...(reactComponentAnnotation?.ignoredComponents !== undefined && { + ignoredComponents: reactComponentAnnotation.ignoredComponents, + }), ...unstable_sentryVitePluginOptions?.reactComponentAnnotation, }, release: { ...unstable_sentryVitePluginOptions?.release, ...release, }, - // will be handled in buildEnd hook sourcemaps: { - disable: true, ...unstable_sentryVitePluginOptions?.sourcemaps, + // Injection and upload are handled in the buildEnd hook, so the Vite plugin must + // never do it too. This is deliberately not overridable - see the warning above. + disable: true, + // The plugin deletes these in a `finally` block that runs regardless of `disable`, + // which would remove the maps before `sentryOnBuildEnd` gets to upload them. + // Deletion is handled there instead, from the same option. + filesToDeleteAfterUpload: undefined, }, - ...unstable_sentryVitePluginOptions, }) as Plugin[]; return sentryVitePlugins; diff --git a/packages/react-router/test/vite/buildEnd/handleOnBuildEnd.test.ts b/packages/react-router/test/vite/buildEnd/handleOnBuildEnd.test.ts index a607ff3ccfc6..e2b0985d57d2 100644 --- a/packages/react-router/test/vite/buildEnd/handleOnBuildEnd.test.ts +++ b/packages/react-router/test/vite/buildEnd/handleOnBuildEnd.test.ts @@ -178,6 +178,176 @@ describe('sentryOnBuildEnd', () => { expect(mockSentryCliInstance.releases.uploadSourceMaps).not.toHaveBeenCalled(); }); + it('should not upload source maps when disabled via top-level sourcemaps.disable', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + sourcemaps: { disable: true }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.execute).not.toHaveBeenCalled(); + expect(mockSentryCliInstance.releases.uploadSourceMaps).not.toHaveBeenCalled(); + }); + + // `disable` used to be read from the top-level config only, so this opt-out was + // silently ignored while the Vite plugin honoured it - see #22929. + it('should not upload source maps when disabled via unstable_sentryVitePluginOptions', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + unstable_sentryVitePluginOptions: { + sourcemaps: { disable: true }, + }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.execute).not.toHaveBeenCalled(); + expect(mockSentryCliInstance.releases.uploadSourceMaps).not.toHaveBeenCalled(); + }); + + it('should let top-level sourcemaps.disable override unstable_sentryVitePluginOptions', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + sourcemaps: { disable: false }, + unstable_sentryVitePluginOptions: { + sourcemaps: { disable: true }, + }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.releases.uploadSourceMaps).toHaveBeenCalled(); + }); + + it('should still upload source maps when unstable_sentryVitePluginOptions only sets unrelated sourcemaps keys', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + unstable_sentryVitePluginOptions: { + sourcemaps: { filesToDeleteAfterUpload: ['./build/**/*.map'] }, + }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.execute).toHaveBeenCalledWith(['sourcemaps', 'inject', '/build'], false); + expect(mockSentryCliInstance.releases.uploadSourceMaps).toHaveBeenCalled(); + expect(glob).toHaveBeenCalledWith(['./build/**/*.map'], { + absolute: true, + nodir: true, + }); + }); + + // `'disable-upload'` means "inject debug IDs, but let me upload the maps myself", so + // injection must still run and the maps must survive. + it('should inject debug IDs but skip upload and deletion when disable is "disable-upload"', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + sourcemaps: { disable: 'disable-upload' }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.execute).toHaveBeenCalledWith(['sourcemaps', 'inject', '/build'], false); + expect(mockSentryCliInstance.releases.uploadSourceMaps).not.toHaveBeenCalled(); + expect(glob).not.toHaveBeenCalled(); + }); + + it('should honour "disable-upload" set via unstable_sentryVitePluginOptions', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + unstable_sentryVitePluginOptions: { + sourcemaps: { disable: 'disable-upload' }, + }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(mockSentryCliInstance.execute).toHaveBeenCalledWith(['sourcemaps', 'inject', '/build'], false); + expect(mockSentryCliInstance.releases.uploadSourceMaps).not.toHaveBeenCalled(); + expect(glob).not.toHaveBeenCalled(); + }); + + // Deleting maps that were never uploaded would leave the user with neither. + it('should not delete source maps when upload is disabled', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + sourcemaps: { disable: true }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(glob).not.toHaveBeenCalled(); + expect(fs.promises.rm).not.toHaveBeenCalled(); + }); + + it('should not delete source maps when disabled via the deprecated sourceMapsUploadOptions', async () => { + const config = { + ...defaultConfig, + viteConfig: { + ...defaultConfig.viteConfig, + sentryConfig: { + ...defaultConfig.viteConfig.sentryConfig, + sourceMapsUploadOptions: { enabled: false }, + }, + } as unknown as TestConfig, + }; + + // @ts-expect-error - mocking the React config + await sentryOnBuildEnd(config); + + expect(glob).not.toHaveBeenCalled(); + }); + it('should delete source maps after upload with default pattern', async () => { // @ts-expect-error - mocking the React config await sentryOnBuildEnd(defaultConfig); diff --git a/packages/react-router/test/vite/makeCustomSentryVitePlugins.test.ts b/packages/react-router/test/vite/makeCustomSentryVitePlugins.test.ts index 2434d7592c5e..34893960b522 100644 --- a/packages/react-router/test/vite/makeCustomSentryVitePlugins.test.ts +++ b/packages/react-router/test/vite/makeCustomSentryVitePlugins.test.ts @@ -1,5 +1,5 @@ import { sentryVitePlugin } from '@sentry/bundler-plugins/vite'; -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { makeCustomSentryVitePlugins } from '../../src/vite/makeCustomSentryVitePlugins'; vi.mock('@sentry/bundler-plugins/vite', () => ({ @@ -7,6 +7,12 @@ vi.mock('@sentry/bundler-plugins/vite', () => ({ })); describe('makeCustomSentryVitePlugins', () => { + beforeEach(() => { + // Without this, `toHaveBeenCalledWith` can match a call made by an earlier test, + // so assertions pass against stale arguments instead of their own. + vi.clearAllMocks(); + }); + it('should pass release configuration to sentryVitePlugin', async () => { const options = { release: { @@ -33,16 +39,19 @@ describe('makeCustomSentryVitePlugins', () => { unstable_sentryVitePluginOptions: { release: { name: 'unstable-release', + setCommits: { auto: true as const }, }, }, }; await makeCustomSentryVitePlugins(options); + // Top-level `release` wins field-wise, but unstable-only fields are preserved expect(sentryVitePlugin).toHaveBeenCalledWith( expect.objectContaining({ release: { name: 'test-release', + setCommits: { auto: true }, }, }), ); @@ -78,7 +87,7 @@ describe('makeCustomSentryVitePlugins', () => { ); }); - it('should allow overriding sourcemaps via unstable_sentryVitePluginOptions', async () => { + it('should merge sourcemaps options from unstable_sentryVitePluginOptions while keeping disable', async () => { await makeCustomSentryVitePlugins({ unstable_sentryVitePluginOptions: { sourcemaps: { @@ -87,13 +96,223 @@ describe('makeCustomSentryVitePlugins', () => { }, }); - // unstable_sentryVitePluginOptions is spread last, so it fully overrides sourcemaps expect(sentryVitePlugin).toHaveBeenCalledWith( expect.objectContaining({ sourcemaps: { assets: ['dist/**'], + disable: true, }, }), ); }); + + // Regression test for https://github.com/getsentry/sentry-javascript/issues/22929: + // any `sourcemaps` key used to drop `disable: true`, re-enabling debug ID injection + // in the Vite plugin on top of the one done by `sentryOnBuildEnd`. + it('should keep sourcemaps disabled when unstable_sentryVitePluginOptions sets an unrelated sourcemaps key', async () => { + await makeCustomSentryVitePlugins({ + authToken: 'token', + org: 'org', + project: 'project', + unstable_sentryVitePluginOptions: { + release: { name: 'commit-sha', setCommits: { auto: true } }, + sourcemaps: { + filesToDeleteAfterUpload: ['./build/**/*.map'], + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + sourcemaps: { + filesToDeleteAfterUpload: undefined, + disable: true, + }, + }), + ); + }); + + // The plugin's `writeBundle` deletes these in a `finally` block that runs even when + // `sourcemaps.disable` is set, which would remove the maps before `sentryOnBuildEnd` + // uploads them. `sentryOnBuildEnd` performs the deletion instead. + it('should not forward filesToDeleteAfterUpload to the Vite plugin', async () => { + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + sourcemaps: { + assets: ['dist/**'], + filesToDeleteAfterUpload: ['./build/**/*.map'], + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + sourcemaps: { + assets: ['dist/**'], + disable: true, + filesToDeleteAfterUpload: undefined, + }, + }), + ); + }); + + it('should not let unstable_sentryVitePluginOptions re-enable sourcemaps via disable: false', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + sourcemaps: { + disable: false, + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + sourcemaps: expect.objectContaining({ disable: true }), + }), + ); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('sourcemaps.disable: false')); + + warnSpy.mockRestore(); + }); + + it('should not let unstable_sentryVitePluginOptions re-enable sourcemaps via disable: "disable-upload"', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + sourcemaps: { + disable: 'disable-upload', + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + sourcemaps: expect.objectContaining({ disable: true }), + }), + ); + expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining('disable-upload')); + + warnSpy.mockRestore(); + }); + + it('should not warn when unstable_sentryVitePluginOptions sets sourcemaps.disable: true', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { sourcemaps: { disable: true } }, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + + warnSpy.mockRestore(); + }); + + it('should not warn when unstable_sentryVitePluginOptions does not set sourcemaps.disable', async () => { + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { sourcemaps: { assets: ['dist/**'] } }, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + + warnSpy.mockRestore(); + }); + + // metaFramework identifies the SDK to Sentry telemetry, so it stays pinned even + // though unstable_sentryVitePluginOptions can override other options. + it('should keep metaFramework when unstable_sentryVitePluginOptions sets _metaOptions.telemetry', async () => { + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + _metaOptions: { + telemetry: { + metaFramework: 'something-else', + }, + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + _metaOptions: { + telemetry: { + metaFramework: 'react-router', + }, + }, + }), + ); + }); + + it('should keep reactComponentAnnotation from unstable_sentryVitePluginOptions when top-level is unset', async () => { + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + reactComponentAnnotation: { + enabled: true, + ignoredComponents: ['Foo'], + }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + reactComponentAnnotation: { + enabled: true, + ignoredComponents: ['Foo'], + }, + }), + ); + }); + + it('should merge reactComponentAnnotation field-wise with unstable_sentryVitePluginOptions', async () => { + await makeCustomSentryVitePlugins({ + reactComponentAnnotation: { enabled: true }, + unstable_sentryVitePluginOptions: { + reactComponentAnnotation: { ignoredComponents: ['Foo'] }, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + reactComponentAnnotation: { + enabled: true, + ignoredComponents: ['Foo'], + }, + }), + ); + }); + + // `unstable_sentryVitePluginOptions` is documented as being able to override any + // option the SDK passes to the Vite plugin, so plain top-level keys stay overridable. + it('should let unstable_sentryVitePluginOptions override plain top-level options', async () => { + await makeCustomSentryVitePlugins({ + org: 'top-level-org', + project: 'top-level-project', + telemetry: false, + unstable_sentryVitePluginOptions: { + org: 'unstable-org', + project: 'unstable-project', + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith( + expect.objectContaining({ + org: 'unstable-org', + project: 'unstable-project', + telemetry: false, + }), + ); + }); + + it('should pass through unstable_sentryVitePluginOptions keys that have no top-level equivalent', async () => { + await makeCustomSentryVitePlugins({ + unstable_sentryVitePluginOptions: { + silent: true, + }, + }); + + expect(sentryVitePlugin).toHaveBeenCalledWith(expect.objectContaining({ silent: true })); + }); });