Uh oh!
There was an error while loading. Please reload this page.
feat(react-router): Add build-time config - #15406
Conversation
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
152b618 to
dca9d06CompareUh 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.
Co-authored-by: Sigrid Huemer <32902192+s1gr1d@users.noreply.github.com>
Lms24
left a comment
There was a problem hiding this comment.
The approach generally looks good to me (as we already discussed offline). I think it's good to try to get debugId injection into the plugin but it's totally fine to go with this solution for the alpha. Likewise, I had some comments for things to address after the initial alpha but IMHO they don't block a first alpha release.
| export default defineConfig(config => { | ||
| return { | ||
| plugins: [reactRouter(), sentryReactRouter(sentryConfig, config)], | ||
| sentryConfig, |
There was a problem hiding this comment.
l: just to confirm does this not throw a type error? or is defineConfig lenient enough to allow adding arbitrary keys to the config object?
There was a problem hiding this comment.
Also something to explore in the future: Maybe we can add a plugin in sentryReactRouter that adds the sentryConfig in the config hook to the vite config, so that users don't have to do it explicitly. Not sure if you already tried this, or which instance of the vite config is passed into buildEnd but maybe it's worth a shot. we could even try to write it onto the global object to pass it over 😅
There was a problem hiding this comment.
Didn't throw a type error, but you are right the best solution for this would be to define this in a plugin that we add! Will do that in a follow up task
| let updatedFilesToDeleteAfterUpload = sourceMapsUploadOptions?.filesToDeleteAfterUpload; | ||
| // set a default value no option was set | ||
| if (typeof sourceMapsUploadOptions?.filesToDeleteAfterUpload === 'undefined') { | ||
| updatedFilesToDeleteAfterUpload = [`${reactRouterConfig.buildDirectory}/**/*.map`]; |
There was a problem hiding this comment.
m/something to follow up on: We only want to delete source maps by default if it was us who turned on source map generation in the first place. This was the reason why I had to pass the promise for filesToDeleteAfterUpload in SvelteKit, because I only knew that once makeEnableSourceMapsPlugin's config hook was invoked. But since we have the vite config here, and we don't rely on the original file deletion plugin, maybe we can solve this simpler 🤔
There was a problem hiding this comment.
Yeah my issue was that I only get the final vite config here and not the initial one, BUT I guess we can maybe just write that into the config as well with a custom plugin (like the sentryConfig from your comment above)
| export default { | ||
| ssr: true, | ||
| buildEnd: sentryOnBuildEnd, |
There was a problem hiding this comment.
l/Q: is there some kind of sequence() helper or so in case people have more than one buildEnd callback?
There was a problem hiding this comment.
in this case they could just
buildEnd: ({ viteConfig, reactRouterConfig, buildManifest })=>{// do their stuffsentryOnBuildEnd({ viteConfig, reactRouterConfig, buildManifest });},
Currently the options passed to both the plugin and the hook partly overlap which is confusing, would be nice to just have one common options object at the end.Actually I can pass these options via vite and read them in the hookWe'll need to revisit this and move DebugId injection back to the vite plugin to avoid modifying the final build. For the alpha release we'll still rely on the SentryCli.
closes#15188