Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 1.8k
fix(react-router): Preserve sourcemaps.disable when unstable_sentryVitePluginOptions is set#22945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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\`.`, | ||
| ); | ||
chargome marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| 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, | ||
chargome marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| // 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; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 () => { | ||
Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm didn't we say that unstable options always have precedence? Tbh this is logaf-super-L for me since we can remove unstable options with v11 but was curious on your thoughts either way Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I misremembered and it's the other way around MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, you're right actually! MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was pre-existing, I'll streamline in a follow up | ||
| 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); | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.