feat(nuxt): Setup source maps with vite config by s1gr1d · Pull Request #13018 · getsentry/sentry-javascript

s1gr1d · 2024-07-23T13:15:31Z

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

s1gr1d · 2024-07-23T13:15:31Z

Closes#13017

mydea · 2024-07-23T13:38:54Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


hmm, no strong feelings, but it feels a bit confusing that this only turns on debug mode for the vite plugin, right? It seems easy to think that this also enables debug mode for sentry itself 🤔 would it make sense to name this in a different, more specific way, e.g. viteDebug or debugVitePlugin, something along these lines? Not sure how this works in other SDKs though, if we do the same there it's probably fine.

I would keep a top-level debug option like this. Makes it very easy to ask users to enable debug mode to debug all issues related to build time stuff.

I was thinking about the same actually before creating this PR. But this debug option is actually quite nice for the build-time debug. The debug flag in init only works during runtime. To make it more explicit, this could be renamed to buildDebug?

I would keep it at debug tbh

Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

lforst · 2024-07-23T13:50:29Z

+ * The SDK options are mostly handled inside the `init` function in separate files (see type `SentryNuxtOptions`).
+ * Other options, such as the source maps options are added inside the `nuxt.config.ts` to be able to access those options during build time and modify the Vite config.
+ */
+export type SentryNuxtModuleOptions = Pick<Options, 'debug'> & SourceMapsOptions;


Any reason you picked debug off the SDK options? I would just add a new debug field and document it separately, because it has nothing to do with the SDK init debug option.
I would phrase the JS doc for this type differently. Imagine you are a user hovering over the type. I would start with something like "Build options used by the Sentry Nuxt SDK. ...". In general I would steer away from describing how things are not and describe what they are.
I would restructure this type and the SourceMapsOptions type so that it is not just an interface with one property.

lforst · 2024-07-23T13:52:27Z

+ });
+
+ viteInlineConfig.plugins = viteInlineConfig.plugins || [];
+ viteInlineConfig.plugins.push(...sentryPlugins);


I think you should be able to do
Suggested change
viteInlineConfig.plugins.push(...sentryPlugins);
viteInlineConfig.plugins.push(sentryPlugins);
and I highly recommend because you theoretically can not assume that the vite plugin is an array.

Lms24

Looks great so far! Most things were already addressed in previews reviews. Feel free to defer the source maps deletion part to a follow up PR but we should think about it before going stable with the SDK.

Lms24 · 2024-07-23T14:14:08Z

+export default defineNuxtConfig({
+ modules: ['@sentry/nuxt/module'],
+ sentry: {
+ debug: true,


Fwiw (no strong opinions but from having dealt with this before): We have this pattern of multiple debug properties in different files in SvelteKit and Astro and so far people haven't complained about it. If we point out well in the JSDoc what kind of logging this specific option enables, I think going simply with debug is fine.
Especially considering, this flag can be used everywhere within the nuxt module (so not just within the vite plugin I assume) I wouldn't give it a too specific name.

Lms24 · 2024-07-23T14:17:49Z

+
+ if ((sourceMapsUploadOptions.enabled ?? true) && viteInlineConfig.mode !== 'development') {
+ const sentryPlugins = sentryVitePlugin({
+ org: sourceMapsUploadOptions.org ?? process.env.SENTRY_ORG,


l: Do we need to pass in the env variable fallbacks explicitly? the plugin itself should pick them up, right?

Lms24 · 2024-07-23T14:25:17Z

+ viteInlineConfig.plugins.push(...sentryPlugins);
+
+ viteInlineConfig.build = viteInlineConfig.build || {};
+ viteInlineConfig.build.sourcemap = true;


m: If viteInlineConfig.build.sourcemap wasn't true before we set it, I'd recommend to log out a warning that we're changing their build config to enable emitting source maps. It's fine to do that but users are worried that their source maps then get deployed to prod.
I'd recommend we expose the filesToDeleteAfterUpload option from the plugin and hint them in the warning that they should set it if they want to delete source maps afterwards again.

Lms24

Thanks for addressing my feedback! Good to go from my end!

Lms24 · 2024-07-24T09:52:59Z

 config: config => {
- const sourceMapsPreviouslyEnabled = !config.build?.sourcemap;
- if (debug && sourceMapsPreviouslyEnabled) {
+ const sourceMapsPreviouslyNotEnabled = !config.build?.sourcemap;


thx for fixing! :)

s1gr1d · 2024-07-24T10:59:04Z

lock changes are from this PR: #12920

feat(nuxt): Setup source maps with vite config
b0523a2

s1gr1d requested review from Lms24 and lforst July 23, 2024 13:36

mydea reviewed Jul 23, 2024
View reviewed changes

lforst reviewed Jul 23, 2024
View reviewed changes

Lms24 reviewed Jul 23, 2024
View reviewed changes

s1gr1dand others added 4 commits July 24, 2024 09:59

Merge branch 'refs/heads/develop' into sig/sourcemaps-nuxt
2a9e671

review comments
ea78994

fix type error
74ee389

Merge branch 'develop' into sig/sourcemaps-nuxt
02a2f33

s1gr1d requested review from Lms24, lforst and mydea July 24, 2024 09:44

Lms24 approved these changes Jul 24, 2024
View reviewed changes

Merge branch 'develop' into sig/sourcemaps-nuxt
6bc94dd

s1gr1d merged commit ea07ec7 into developJul 24, 2024

s1gr1d deleted the sig/sourcemaps-nuxt branch July 24, 2024 10:58

s1gr1d commented Jul 24, 2024
View reviewed changes

Comment threadyarn.lock
s1gr1dJul 24, 2024
Copy link
Copy Markdown
MemberAuthor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock changes are from this PR: #12920

angeloreale mentioned this pull request Aug 15, 2024
[Snyk] Upgrade @sentry/nextjs from 8.17.0 to 8.20.0 dupipcom/hypnos#30
Merged

q1blue mentioned this pull request Aug 16, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.20.0 https-quantumblockchainai-atlassian-net/ossinsight#807
Open

wkylin mentioned this pull request Aug 19, 2024
[Snyk] Upgrade @sentry/react from 8.18.0 to 8.20.0 wkylin/pro-react-admin#383
Closed

sleidig mentioned this pull request Aug 21, 2024
[Snyk] Upgrade @sentry/angular from 8.20.0 to 8.21.0 Aam-Digital/ndb-core#2517
Closed

q1blue mentioned this pull request Aug 22, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.21.0 https-quantumblockchainai-atlassian-net/ossinsight#814
Open

sleidig mentioned this pull request Aug 22, 2024
[Snyk] Upgrade @sentry/angular from 8.20.0 to 8.22.0 Aam-Digital/ndb-core#2519
Closed

angeloreale mentioned this pull request Aug 22, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.22.0 dupipcom/hypnos#32
Merged

q1blue mentioned this pull request Aug 23, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.22.0 https-quantumblockchainai-atlassian-net/ossinsight#816
Open

gokulk16 mentioned this pull request Aug 23, 2024
[Snyk] Upgrade @sentry/browser from 8.20.0 to 8.22.0 gokulk16/type-to-calculate-web#33
Closed

FengShangWuQi mentioned this pull request Aug 25, 2024
[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.22.0 FengShangWuQi/fengshangwuqi.github.io#1916
Closed

angeloreale mentioned this pull request Aug 25, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.22.0 dupipcom/hypnos#46
Closed

q1blue mentioned this pull request Aug 27, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.23.0 https-quantumblockchainai-atlassian-net/ossinsight#820
Open

gokulk16 mentioned this pull request Aug 27, 2024
[Snyk] Upgrade @sentry/browser from 8.20.0 to 8.23.0 gokulk16/type-to-calculate-web#35
Closed

angeloreale mentioned this pull request Aug 27, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.23.0 dupipcom/hygieia#1
Merged

tomwwinter mentioned this pull request Aug 27, 2024
[Snyk] Upgrade @sentry/angular from 8.20.0 to 8.23.0 Aam-Digital/ndb-core#2537
Closed

gokulk16 mentioned this pull request Aug 28, 2024
[Snyk] Upgrade @sentry/browser from 8.20.0 to 8.24.0 gokulk16/type-to-calculate-web#36
Closed

angeloreale mentioned this pull request Aug 28, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.24.0 dupipcom/hygieia#3
Open

q1blue mentioned this pull request Aug 28, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.24.0 https-quantumblockchainai-atlassian-net/ossinsight#833
Open

angeloreale mentioned this pull request Aug 29, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.24.0 dupipcom/hygieia#4
Open

gokulk16 mentioned this pull request Aug 30, 2024
[Snyk] Upgrade @sentry/browser from 8.20.0 to 8.25.0 gokulk16/type-to-calculate-web#38
Merged

angeloreale mentioned this pull request Aug 30, 2024
[Snyk] Upgrade @sentry/nextjs from 8.20.0 to 8.25.0 dupipcom/hygieia#5
Open

q1blue mentioned this pull request Aug 30, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.25.0 https-quantumblockchainai-atlassian-net/ossinsight#835
Open

FengShangWuQi mentioned this pull request Sep 1, 2024
[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.25.0 FengShangWuQi/fengshangwuqi.github.io#1918
Closed

PierreGode mentioned this pull request Sep 4, 2024
[Snyk] Upgrade @sentry/node from 8.9.2 to 8.25.0 PierreGode/shields#1
Closed

q1blue mentioned this pull request Sep 7, 2024
[Snyk] Upgrade @sentry/react from 7.16.0 to 8.26.0 https-quantumblockchainai-atlassian-net/ossinsight#846
Open

FengShangWuQi mentioned this pull request Sep 8, 2024
[Snyk] Upgrade @sentry/gatsby from 7.20.0 to 8.26.0 FengShangWuQi/fengshangwuqi.github.io#1919
Closed