Skip to content
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

fix(sveltekit): Avoid loading vite config to determine source maps setting #15440

Merged
merged 4 commits into from
Feb 20, 2025

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented Feb 18, 2025

This PR refactors how we determine the user-provided source map generation settings in our sentrySvelteKit vite plugins.

Previously, we'd call Vite's loadConfigFromFile function which sounded good but had a couple of drawbacks:

Since there's no reliably way to read the mode for Vite, and parsing process.argv also seemed sketchy, I decided to go down the Vite-recommended route and to read the config in the config hook. This required a change in our @sentry/vite-plugin which was released in 3.2.0, so this PR also bumps the Vite plugin version.

Concrete changes:

  • read sourcemap generation config in config hook of our source maps settings sub plugin
  • resolve the filesToDeleteAfterUpload promise whenever we know what to set it to (using a promise allows us to defer this decision to plugin hook runtime rather than plugin creation time)
  • adusted tests

closes #15414

@Lms24 Lms24 self-assigned this Feb 19, 2025
@Lms24 Lms24 requested review from s1gr1d and chargome February 19, 2025 11:46
@Lms24 Lms24 marked this pull request as ready for review February 19, 2025 11:46
@Lms24 Lms24 force-pushed the lms/fix-sveltekit-avoid-loadViteConfig branch from 9bb759d to efaf8c5 Compare February 19, 2025 11:48
Comment on lines +103 to +104
// resolving filesToDeleteAfterUpload here, because we return the original deletion plugin which awaits the promise
resolveFilesToDeleteAfterUpload(undefined);
Copy link
Member Author

@Lms24 Lms24 Feb 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the ugly part of this PR: We always need to resolve the promise we pass to the vite plugin. So everywhere where we early return, in addition to the expected places.

Copy link

codecov bot commented Feb 19, 2025

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
4591 1 4590 324
View the top 1 failed test(s) by shortest run time
sessions/initial-scope/test.ts should start a new session with navigation.
Stack Traces | 0.496s run time
test.ts:19:11 should start a new session with navigation.

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@Lms24 Lms24 merged commit 6c69710 into develop Feb 20, 2025
147 checks passed
@Lms24 Lms24 deleted the lms/fix-sveltekit-avoid-loadViteConfig branch February 20, 2025 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sentrySvelteKit loads Vite config in the wrong mode
2 participants