feat(browser): Enable Spotlight via SENTRY_SPOTLIGHT env var #18050
Conversation
BYK
commented
Oct 29, 2025
BYK
commented
- Move envToBool utility from node-core to core for code sharing
- Add spotlight option to BrowserSpecificOptions type
- Support SENTRY_SPOTLIGHT env var in browser SDK with graceful fallback
- Auto-inject spotlightBrowserIntegration when enabled via env var
- Match Node.js behavior: truthy values use default URL, custom strings use that URL
- Move envToBool utility from node-core to core for code sharing - Add spotlight option to BrowserSpecificOptions type - Support SENTRY_SPOTLIGHT env var in browser SDK with graceful fallback - Auto-inject spotlightBrowserIntegration when enabled via env var - Match Node.js behavior: truthy values use default URL, custom strings use that URL
size-limit report 📦
|
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.
|
There was a problem hiding this comment.
Preliminary review
It seems like this PR does two things:
- add
spotlightBrowserIntegrationbased on thespotlightinit option (i.e. remove the need for users to add it manually and thereby destroy all tree-shaking abilities) - if not set, enable it if the
SENTRY_SPOTLIGHTenv variable is set at build/dev-build time
Can we split this PR up? The reason I'm asking for this is because I'm concerned with 2 (see my review comment), so I want to be able to easily revert either part of it without removing the other one, in case our changes break users.
| // Read spotlight config from env var with graceful fallback | ||
| // This will be replaced at build time by bundlers like webpack/vite | ||
| let spotlightFromEnv: boolean | string | undefined; | ||
| if (typeof process !== 'undefined' && process.env && process.env.SENTRY_SPOTLIGHT !== undefined) { | ||
| const envValue = process.env.SENTRY_SPOTLIGHT; | ||
| const parsedEnvValue = envToBool(envValue, { strict: true }); | ||
| spotlightFromEnv = parsedEnvValue ?? envValue; | ||
| } |
There was a problem hiding this comment.
So, process.env will almost never* be replaced by bundlers, which means that this code path will almost never work. The only bundler that straight up adds support for process.env.X seems to be Parcel.
Webpack requires users to additionally add a replacement plugin that hard-replaces strings at build time. Rollup requires the same treatment (just with its own plugin architecture, or something like rollup-plugin-replace). In either case, this current code would fail because while process might be undefined, process.env.SENTRY_SPOTLIGHT could be "defined" (in the sense of the whole thing being hard-replaced at build time). See my code suggestion for a tentative workaround.
Vite (the one I care most about, next to webpack), can dynamically replace any env variable that is prefixed with VITE_. This is a security measure to avoid users leaking keys into their public browser bundles (actually a good thing IMHO). So users would have to define VITE_SENTRY_SPOTLIGHT instead of SENTRY_SPOTLIGHT. As I understand it, this again requires a "special treatment" for Spotlight in browser which I think you want to avoid.
Now the delightful JS ecosystem can't even agree on this VITE_ prefix, so Vite allows users to customize their public prefix via envPrefix.
In a similar manner, some frameworks built their own abstractions around env variables, for example NextJS but it requires a NEXT_PUBLIC_ prefix. The good news is that process.env.NEXT_PUBLIC_SENTRY_SPOTLIGHT would work. SvelteKit allows env variables prefixed with PUBLIC_ to be replaced in its browser bundles but they must be explicitly imported. Nuxt seems to prefer defining env variables explicitly in a runtime config file.
Older frameworks like Angular or Ember don't have env variable support at all but instead recommend using environment.(prod|dev|...).ts files with the respective variables defined in an environment object.
So, as you can see, there's no universal solution that will make this "just work". So we have two choices:
a) apply a fail-safe best-effort check and document that setting SENTRY_SPOTLIGHT might sometimes work depending on framework and bundler but other times it won't in which case users need to fall back to setting the option themselves.
b) not bother with this and the bytes this costs us but simply make it explicit for users to set spotlight: true|false on their terms, with the check they prefer.
In case you want to go with a (which I don't recommend), here's an idea how this could be done, trying to keep the bundle size waste as low as possible:
// this will need a bunch of ts-ignores
// wherever we need to check this
const enableSpotlight = options.spotlight ?? getSpotlightFromEnv();
// ...
function getSpotlightFromEnv() {
try {
const processEnvVar = process.env.SENTRY_SPOTLIGHT || process.env.NEXT_PUBLIC_SENTRY_SPOTLIGHT;
return envToBool(processEnv , { strict: true });
} catch { /* ignore */ }
try {
// this still feels scary to add to the SDK because I don't know how bundlers other than Vite react to `import.meta` mentioned in the code. They could throw a build error. We should at least test this with webpack.
return envToBool(import.meta.env.VITE_SENTRY_SPOTLIGHT, { strict: true });
} catch { /* ignore */ }
return false;
}The most important aspects of this function are:
- we must never throw at runtime or cause a build error
- we must always default to
falserather than true because otherwise, we risk sending stuff to spotlight in a live prod build.
Overall, I'd still recommend going with b.
