feat(nuxt): Base decision on source maps upload only on Nuxt source map settings #15859
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors how source map settings are handled in Nuxt so that decision-making is solely based on Nuxt’s root-level source map options, reducing complexity and preventing unintended overwrites.
- Unified source map configuration for Vite and Nitro using Nuxt settings.
- Refactored logging and validation functions for clearer, consolidated behavior.
- Updated tests to verify fallback logic for deleting files after upload.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/nuxt/test/vite/sourceMaps.test.ts | Updated tests to reflect new plugin options and fallback behavior for file deletion. |
| packages/nuxt/src/vite/sourceMaps.ts | Refactored source map settings handling to rely on Nuxt config, improved logging, and added validation helpers. |
Comments suppressed due to low confidence (2)
packages/nuxt/src/vite/sourceMaps.ts:144
- [nitpick] Consider renaming 'fallbackFilesToDelete' to 'defaultFilesToDeleteAfterUpload' to better reflect its purpose as the default deletion pattern for generated source maps.
const fallbackFilesToDelete = [
packages/nuxt/test/vite/sourceMaps.test.ts:155
- [nitpick] Consider adding additional test cases that validate behavior when both client and server fallback options are false, ensuring that no deletion patterns are set in those scenarios.
it('sets filesToDeleteAfterUpload correctly based on fallback options', () => {
There was a problem hiding this comment.
Thanks for the great PR explanation 🙏 and for digging through this mess of source maps options 😅
I think the approach to only change nuxt config source map settings and warn on divergences in lower level settings sounds very reasonable. My comments are mainly focused around avoiding user confusions with log messages and tests.
packages/nuxt/src/vite/sourceMaps.ts
Outdated
| @@ -275,24 +370,24 @@ function logKeepSourceMapSetting( | |||
| consoleSandbox(() => { | |||
| // eslint-disable-next-line no-console | |||
| console.log( | |||
| `[Sentry] We discovered \`${settingKey}\` is set to \`${settingValue}\`. Sentry will keep this source map setting. This will un-minify the code snippet on the Sentry Issue page.`, | |||
| `[Sentry] We discovered \`${settingKey}\` is set to \`${settingValue}\`. This will un-minify the code snippet on the Sentry Issue page. Be aware that there might be other source map settings in your config which could overwrite this setting.`, | |||
There was a problem hiding this comment.
l: should we keep the "be aware" sentence here? as far as I understand, we'd emit an explicit warning anyway via validateDifferentSourceMapSettings, right?
The reason why I'm thinking about this is that this should be a "success message", right? I just want to convey this and not risk users thinking they did something wrong or mistake the message with a warning. Especially with "hidden" I could see users being confused.
What about something like this?
| `[Sentry] We discovered \`${settingKey}\` is set to \`${settingValue}\`. This will un-minify the code snippet on the Sentry Issue page. Be aware that there might be other source map settings in your config which could overwrite this setting.`, | |
| `[Sentry] \`${settingKey}\` is already set to \`${settingValue}\`. This will correctly un-minify the code snippet on the Sentry Issue page.` |
Which brings me to an alternative question/idea: Do we even need this message at all? In case of users already having source maps enabled, there's nothing for us to do anyway, right? Not saying it has to go, just food for thought :)
There was a problem hiding this comment.
I'll change this to your suggestion but will keep it to show it when debug is enabled :)
| @@ -148,146 +151,187 @@ describe('getPluginOptions', () => { | |||
| }), | |||
| ); | |||
| }); | |||
|
|
|||
| it('sets filesToDeleteAfterUpload correctly based on fallback options', () => { | |||
There was a problem hiding this comment.
super-l: is there a reason to test multiple scenarios in one test? no strong feelings but I personally prefer sticking to one scenario per test (could be it.each or separate tests) because it makes it easier to see what specifically fails. But feel free to disregard and stick with this.
|
|
||
| beforeEach(() => { | ||
| cases.forEach(({ clientSourcemap, expectedSourcemap, expectedReturn }) => { |
There was a problem hiding this comment.
l: We can rewrite this as well as the cases below to it.each instead of iterating over the loop. My main concern is seeing what exactly failed which is a bit difficult in the loop (especially when just checking CI logs).
…ap settings (#15859) There is a total of 6 settings in Nuxt where you can disable/enable your source maps. This is not only difficult to understand when writing and maintaining the logic for this. It may also be hard to debug stuff for users.  There is this discussion around Nuxt source maps: #15028 And this issue: #15160 A problem with the current setup was not only the difficulty to understand all of this but also it sometimes just overwrote stuff you didn't want it to. Like in the default case of Nuxt, `sourcemap.server` is `true`, but as `nitro.rollupConfig.output.sourcemap` is always undefined, Sentry would overwrite this setting to `'hidden'`, even though the source maps were enabled already. --- The only two relevant options in Nuxt are the [root-level sourcemap options](https://nuxt.com/docs/guide/going-further/debugging#sourcemaps). Those settings will propagate to nitro, vite and rollup. As we overwrite the source maps setting if the setting is undefined, only the basic Nuxt source map settings (linked above) are taken into account for that from now on.
Improving tests for #15859 - added `it.each` instead of looping with `foreach` - added tests for console output
There is a total of 6 settings in Nuxt where you can disable/enable your source maps. This is not only difficult to understand when writing and maintaining the logic for this. It may also be hard to debug stuff for users.
There is this discussion around Nuxt source maps: #15028
And this issue: #15160
A problem with the current setup was not only the difficulty to understand all of this but also it sometimes just overwrote stuff you didn't want it to. Like in the default case of Nuxt,
sourcemap.serveristrue, but asnitro.rollupConfig.output.sourcemapis always undefined, Sentry would overwrite this setting to'hidden', even though the source maps were enabled already.The only two relevant options in Nuxt are the root-level sourcemap options. Those settings will propagate to nitro, vite and rollup.
As we overwrite the source maps setting if the setting is undefined, only the basic Nuxt source map settings (linked above) are taken into account for that from now on.