feat(sveltekit): Respect user-provided source map generation settings

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this

[s1gr1d]

Enables hidden source maps if source maps are unset. In case they are explicitly disabled or enabled the setting is kept as is.

Still need to figure out how to set filesToDeleteAfterUpload as this setting needs to be passed when "calling" the sentry Vite plugin. However, filesToDeleteAfterUpload is dependent on the Vite options and we only have access to the Vite options when creating the custom plugin.

I thought about moving this parts of this logic to the sentryVitePlugin in general.

closes#14885

[Lms24]

I thought about moving this parts of this logic to the sentryVitePlugin in general.

This sounds reasonable to me. The base plugin's deletion behaviour is also problematic on its own as reported in #14131. I'm looking into this right now but maybe the answer really is to just handle deletion within our sentrySvelteKit Vite plugin

[Lms24]

I see that we don't use this return value other than in tests. We'll need the return value for when we can automatically configure filesToDeleteAfterUpload, right?

[Lms24]

Thanks for taking care of this! Just a concern about testing. Implementation looks great to me other than two small remarks

[chargome]

Looks good, I would just second what Lukas said about the export for mocking sourceMapSetting!

[Lms24]

Okay, I made several changes:

• we now read the config only once and store it on globalThis._sentry_sourceMapSettings
• this seems a bit safer to me than to store it in module scope because I'm a bit scared about recursive calls. While we can't repro them, I have a repro for a simple test plugin where an infinite recursion occurs.
• I'm still scared of actually re-reading the vite config file we're in but let's roll with it for now. If we get reports about this causing problems, we can also change the strategy to set a flag on the global object that specifies the glob pattern and use this in the bundler plugin instead of filesToDeleteAfterUpload. Let's cross that bridge when we get to it though.
• adjusted the tests to mock the loadConfigFromFile config and reset the global object per test
• added the output folders which can be configured dynamically. Luckliy we're already loading the necessary config file, so it's just a matter of setting these things correctly.

I'll open a small follow-up PR to unify logging a bit more but this has a larger area than source maps, so it should be its own PR.

[codecov]

❌ 1 Tests Failed:

Tests completed	Failed	Passed	Skipped
695	1	694	300

View the top 1 failed tests by shortest run time

client-app-routing-instrumentation.test.tsCreatesanavigationtransactionforapprouterroutes

Stack Traces | 30s run time

client-app-routing-instrumentation.test.ts:19:5Createsanavigationtransactionforapprouterroutes

To view more test analytics, go to the Test Analytics Dashboard
📢 Thoughts on this report? Let us know!

[Lms24]

let's merge this