-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(tanstackstart-react): Add sentryTanstackStart vite plugin to manage automatic source map uploads #18712
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
Conversation
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| /** | ||
| * Build options for the Sentry plugin. These options are used during build-time by the Sentry SDK. | ||
| */ | ||
| export type SentryTanstackStartReactPluginOptions = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can/should extend from this type, so we can make sure that all build-time options are aligned:
| export interface BuildTimeOptionsBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah nice thanks I did not know that existed, updated
| if (process.env.NODE_ENV !== 'development') { | ||
| // Check if source maps upload is enabled | ||
| // Default to enabled | ||
| const sourceMapsEnabled = options.sourceMapsUploadOptions?.enabled ?? true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably want to use options.sourcemaps.disable here. Also make sure people can still create a release, even if they don't want to upload source maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to use disable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also always adding the sentry vite plugin down passing down the source maps disable option so that release creation works also if people do not use source maps
|
A general thought I had on this: Are the sources included in Nitro source maps? In the Nuxt SDK, we have to disable this setting:
|
Lms24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naive question for now: What's the advantage over this wrapper that we can't achieve with just exporting a plugin? If we need to modify the vite config, Vite offers plugin hooks to do so from within a plugin.
I'd personally prefer the plugin approach because this is a pattern that users are more used to than wrapping their config. If you or anyone else has different opinions, let's hear it :D
| sourcemaps: { disable: true }, | ||
| }); | ||
|
|
||
| expect(plugins).toHaveLength(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-l: we can use expect(plugins).toEqual([..., ..., ...]) here (+ in the tests below) instead of the length and find operations. I would argue the plugin order should be deterministic and we should fail the test if the order changes. Sometimes even plugins depend on each other, so the order matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
dev-packages/e2e-tests/test-applications/tanstackstart-react/vite.config.ts
Show resolved
Hide resolved
|
|
||
| // Default to auto-deleting source maps from hidden directories after upload | ||
| // Users can override this by explicitly setting sourcemaps.filesToDeleteAfterUpload | ||
| const defaultFilesToDelete = ['.*/**/*.map']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to confirm: TSS' build output is in a hidden directory? (as opposed to something like /dist)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using nitro, it's .output but maybe it might be worth setting this to ./**/*.map as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using ./**/*.map now to cover all cases
| const plugins: Plugin[] = []; | ||
|
|
||
| // Only add plugins in production builds | ||
| if (process.env.NODE_ENV !== 'development') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super-l: If we don't do anything in development, we could invert the if condition here and early return. Feel free to ignore, this is mostly just me preferring early returns and keeping the mainline logic outside of conditions 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jap fair enough I usually prefer this too, thanks for the comment I flipped it
| const configPlugin: Plugin = { | ||
| name: 'sentry-tanstackstart-source-maps-config', | ||
| apply: 'build', | ||
| enforce: 'pre', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m: we probably want to enforce that configPlugin plugin runs 'post' so that we get the most likely final config object
actually now that I think about it, does this plugin do anything besides logging? Looks like we unconditionally set filesToDeleteAfterUpload when we create the @sentry/vite-plugin plugins (?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- changed it to
post - true, had a subtle bug here. I moved the setting of
filesToDeleteAfterUploadinto the if/else so warning and setting actually align, so the plugin now determines the correct setting and also optionally warns now. also now use a promise as we do in SvelteKit for example that gets resolved in the plugin and is put into thesentry/vite-pluginsince otherwise we cannot really ensure that we get the updated setting in the Vite-plugin
| export default defineConfig({ | ||
| plugins: [ | ||
| sentryTanstackStart({ | ||
| org: 'your-org', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
l: Nice changelog entry! Just for completeness I suggest we mention auth token here as well. Alternatively, we can just mention it in text (smthg like "Make sure to set the SENTRY_AUTH_TOKEN env var") since the env var is picked up automatically.
| org: 'your-org', | |
| authToken: process.env.SENTRY_AUTH_TOKEN, | |
| org: 'your-org', |
Adds sentryTanstackStart Vite plugin to enable automatic source map uploads in TanStack Start applications.
What it does
I tried to align with what we do in other SDKs (e.g. SvelteKit). On a high-level it:
Usage
Tests
Closes #18664