feat(browser): Add environment variable support for Spotlight configuration #18198
Conversation
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.
|
…ration - Add support for multiple framework/bundler-specific environment variables with proper precedence - SENTRY_SPOTLIGHT (highest priority - base name, supported natively by many bundlers) - PUBLIC_SENTRY_SPOTLIGHT (SvelteKit, Astro, Qwik) - NEXT_PUBLIC_SENTRY_SPOTLIGHT (Next.js) - VITE_SENTRY_SPOTLIGHT (Vite) - NUXT_PUBLIC_SENTRY_SPOTLIGHT (Nuxt.js) - REACT_APP_SENTRY_SPOTLIGHT (Create React App) - VUE_APP_SENTRY_SPOTLIGHT (Vue CLI) - GATSBY_SENTRY_SPOTLIGHT (Gatsby) - Add defensive environment variable access via process.env (transformed by all major bundlers) - Move envToBool utility from node-core to core for shared usage - Add resolveSpotlightOptions utility for consistent precedence rules - Update node-core and aws-serverless to use shared utilities - Add comprehensive tests for all new utilities and SDK integration Note: import.meta.env is intentionally not checked because bundlers only replace static references (e.g., import.meta.env.VITE_VAR) at build time, not dynamic access. All major bundlers transform process.env references, making it the universal solution.
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
92ffd33 to
6cb5513
Compare
BYK
force-pushed
the
feat/spotlight-environment-variable-support
branch
from
4f6dcd0 to
fda3d61
Compare
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). Fixes a test failure where `getSpotlightConfig` was returning empty or whitespace-only strings for `SENTRY_SPOTLIGHT` environment variables, instead of `undefined`. This change explicitly filters out such values, aligning the function's behavior with test expectations and preventing invalid Spotlight configurations. --- <a href="https://cursor.com/background-agent?bcId=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-88bd0365-3796-47b2-bb84-29c93d0430e8"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Lms24
left a comment
Lms24
left a comment
There was a problem hiding this comment.
First pass review. I still want to figure out why size bot reports a slight increase for the CDN bundles.
packages/browser/src/utils/env.ts
Outdated
| * Checks process.env which is transformed by most bundlers (Webpack, Vite, Rollup, Rspack, Parcel, etc.) | ||
| * at build time. | ||
| * | ||
| * Note: We don't check import.meta.env because: | ||
| * 1. Bundlers only replace static references like `import.meta.env.VITE_VAR`, not dynamic access | ||
| * 2. Dynamic access causes syntax errors in unsupported environments | ||
| * 3. Most bundlers transform process.env references anyway |
There was a problem hiding this comment.
m: My impression while doing some research for #18050 (comment) was what very few bundlers inject process.env into browser bundles. For example, this code will not work in Angular.
Vite instructs checking on import.meta.env, so not sure if it double-writes to process.env (my gut feeling is no).
Did you test this in the respective frameworks? Maybe I'm also misinformed and this works just fine 😅 Ideally we can add an e2e test for at least NextJS and some Vite-based framework app to be sure.
|
|
||
| // No Spotlight configuration found in environment | ||
| return undefined; |
There was a problem hiding this comment.
l: neat size trick: You can let the function return void and simply omit the return undefined here. Saves a couple of bytes and JS returns undefined anyway. 😅 (but feel free to ignore since this is shaken out for prod builds anyway)
Before submitting a pull request, please take a look at our [Contributing](https://github.com/getsentry/sentry-javascript/blob/master/CONTRIBUTING.md) guidelines and verify: - [x] If you've added code that should be tested, please add tests. - [x] Ensure your code lints and the test suite passes (`yarn lint`) & (`yarn test`). --- This PR addresses critical feedback regarding environment variable detection, particularly for Vite-based frameworks. **Key Changes:** * **`packages/browser/src/utils/env.ts`**: The `getEnvValue` function now checks both `process.env` (for Webpack, Next.js, CRA) and `import.meta.env` (for Vite, Astro, SvelteKit). This ensures that environment variables (like those for Spotlight) are correctly detected across a wider range of bundlers and frameworks, fixing a significant compatibility issue. * **`packages/browser/test/utils/env.test.ts`**: Updated unit tests to focus on `process.env` scenarios. Added a note explaining that `import.meta.env` cannot be unit tested due to its read-only, compile-time nature and is covered by e2e tests. * **`packages/browser/src/utils/spotlightConfig.ts`**: Added a comment to clarify the explicit `return undefined` for readability, noting its optimization in production builds. --- <a href="https://cursor.com/background-agent?bcId=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-cursor-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-cursor-light.svg"><img alt="Open in Cursor" src="https://cursor.com/open-in-cursor.svg"></picture></a> <a href="https://cursor.com/agents?id=bc-d6001dc9-59fc-42eb-8354-a573e3ae71a9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/open-in-web-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/open-in-web-light.svg"><img alt="Open in Web" src="https://cursor.com/open-in-web.svg"></picture></a> Co-authored-by: Cursor Agent <cursoragent@cursor.com>
timfish
left a comment
timfish
left a comment
There was a problem hiding this comment.
Looks good but needs some e2e tests of some kind otherwise we could inadvertently break this in the future.
It should be quite easy to test Vite by adding some additional tests to dev-packages/bundler-tests with the specific variables set and then check they make it into the bundle.
Then as @Lms24 says, a Nextjs test would probably be a good idea too!
There was a problem hiding this comment.
Bug: Breaking change: `envToBool` export removed from `@sentry/node-core` (Bugbot Rules)
The envToBool function was previously exported from @sentry/node-core but has been removed in this change (now exported from @sentry/core instead). This is a breaking change for any external consumers who were importing envToBool directly from @sentry/node-core. The function is still available from @sentry/core, but existing code using import { envToBool } from '@sentry/node-core' will break. Flagging this per the review rules about "Removal of publicly exported functions, classes, or types."
packages/node-core/src/index.ts#L44-L45
sentry-javascript/packages/node-core/src/index.ts
Lines 44 to 45 in b6fda2b
packages/nextjs/src/client/index.ts
Outdated
| const spotlightEnvValue = process.env.NEXT_PUBLIC_SENTRY_SPOTLIGHT; | ||
| if (spotlightEnvValue !== undefined && options.spotlight === undefined) { | ||
| const boolValue = envToBool(spotlightEnvValue, { strict: true }); | ||
| const spotlightConfig = boolValue !== null ? boolValue : spotlightEnvValue; | ||
| const spotlightValue = resolveSpotlightOptions(undefined, spotlightConfig); | ||
|
|
||
| if (spotlightValue) { | ||
| const spotlightArgs = typeof spotlightValue === 'string' ? { sidecarUrl: spotlightValue } : undefined; | ||
| customDefaultIntegrations.push(spotlightBrowserIntegration(spotlightArgs)); | ||
| } | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Module field removed from browser package.json
The "module" field was removed from packages/browser/package.json. This field was previously set to "build/npm/esm/prod/index.js" and is used by older bundlers (e.g., Webpack 4, older Rollup versions) that don't support the "exports" field for ESM module resolution. Without it, these bundlers will fall back to the "main" field (CJS), potentially causing issues in ESM-expecting environments. This change appears unrelated to the PR's goal of adding Spotlight environment variable support and may be accidental.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 0d69896
There was a problem hiding this comment.
Bug: Removed `module` field breaks older bundler compatibility
The module field ("module": "build/npm/esm/prod/index.js") was removed from @sentry/browser's package.json. While modern bundlers support the exports field, older bundlers (like Webpack 4 and older Rollup versions) rely on the module field to resolve ES module entry points. This is inconsistent with other SDK packages (@sentry/core, @sentry/react, @sentry/vue) which still have this field, and could cause older bundlers to incorrectly use the CJS build instead of ESM.
packages/browser/package.json#L14-L16
sentry-javascript/packages/browser/package.json
Lines 14 to 16 in 2346b4c
| // Applying turbopack config when webpack is used is safe (webpack ignores it). | ||
| if (isTurbopack || isTurbopackSupported) { | ||
| turboPackConfig = constructTurbopackConfig({ | ||
| userNextConfig: incomingUserNextConfigObject, | ||
| userSentryOptions, | ||
| routeManifest, | ||
| nextJsVersion, | ||
| spotlightConfig, | ||
| }); | ||
| } | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| if (spotlightValue) { | ||
| const spotlightArgs = typeof spotlightValue === 'string' ? { sidecarUrl: spotlightValue } : undefined; | ||
| customDefaultIntegrations.push(spotlightBrowserIntegration(spotlightArgs)); | ||
| // eslint-disable-next-line no-console | ||
| console.log('[Sentry Next.js DEBUG] Spotlight integration ADDED'); | ||
| } | ||
| } | ||
|
|
||
| return customDefaultIntegrations; | ||
| } | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| optionsSpotlight: options.spotlight, | ||
| GLOBAL_OBJ_keys: Object.keys(GLOBAL_OBJ), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Bug: Debug data exposed on globalThis in production builds
Debug information is being exposed on globalThis._sentrySpotlightDebug in the getDefaultIntegrations function. This object contains internal debugging data like processEnvSpotlight, GLOBAL_OBJ_keys, and other internal values that leak implementation details. This code is not wrapped in development-only markers and will be included in production bundles, potentially exposing internal state to end users.
| "preview": "vite preview --port 3030", | ||
| "test": "playwright test", | ||
| "test:build": "pnpm install && pnpm build", | ||
| "test:build": "pnpm install", |
There was a problem hiding this comment.
Bug: E2E test skips production build by removing pnpm build
The test:build script was changed from pnpm install && pnpm build to just pnpm install, and the playwright config now uses pnpm dev instead of pnpm preview. This means tests no longer run against the production build, potentially masking production-specific bugs like the new ESM/CJS stripping behavior. The tests removed debug_meta assertions which are only generated in production builds.
Additional Locations (1)
| expect(errorEvent.exception?.values?.[0]?.value).toBe('Uncaught Error: Uncaught error in worker'); | ||
| expect(errorEvent.exception?.values?.[0]?.stacktrace?.frames).toHaveLength(1); | ||
| expect(errorEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.filename).toMatch(/worker-.+\.js$/); | ||
| expect(errorEvent.exception?.values?.[0]?.stacktrace?.frames?.[0]?.filename).toMatch(WORKER_FILENAME_PATTERN); |
There was a problem hiding this comment.
Bug: Test no longer verifies debug IDs despite test name
The test is named "captures an error with debug ids and pageload trace context" but the debug_meta assertion that verified debug IDs has been removed. The test name claims to verify debug ID functionality, but now only checks trace context. Debug IDs could break without the test failing. This represents a test coverage regression where the actual test behavior no longer matches its documented purpose.
![https://cursor.com/open-in-cursor-dark.svg"><source](https://cursor.com/open-in-cursor-dark.svg"><source){kind=link}
![https://cursor.com/open-in-cursor-light.svg"><img](https://cursor.com/open-in-cursor-light.svg"><img){kind=link}
![https://cursor.com/open-in-cursor.svg"></picture></a> <a](https://cursor.com/open-in-cursor.svg"></picture></a> <a){kind=link}
![https://cursor.com/open-in-web-dark.svg"><source](https://cursor.com/open-in-web-dark.svg"><source){kind=link}
![https://cursor.com/open-in-web-light.svg"><img](https://cursor.com/open-in-web-light.svg"><img){kind=link}
![https://cursor.com/open-in-web.svg"></picture></a](https://cursor.com/open-in-web.svg"></picture></a){kind=link}
Implements full Spotlight spec with support for multiple framework-specific
environment variable prefixes. Adds defensive environment variable access
for both process.env and import.meta.env to support various bundlers.
Supported environment variables (in priority order):
PUBLIC_SENTRY_SPOTLIGHT(SvelteKit, Astro, Qwik)NEXT_PUBLIC_SENTRY_SPOTLIGHT(Next.js)VITE_SENTRY_SPOTLIGHT(Vite)NUXT_PUBLIC_SENTRY_SPOTLIGHT(Nuxt)REACT_APP_SENTRY_SPOTLIGHT(Create React App)VUE_APP_SENTRY_SPOTLIGHT(Vue CLI)GATSBY_SENTRY_SPOTLIGHT(Gatsby)SENTRY_SPOTLIGHT(base/official)SENTRY_SPOTLIGHTis last as in environments like Docker Compose, we actually make the front-end env variable different than the baseSENTRY_SPOTLIGHTone -- the backends need to reachdocker.host.internalwhereas front-ends always needlocalhostas we assume the browser runs on the same host with Spotlight.Refactors envToBool utility from node-core to core package for shared usage.
Adds resolveSpotlightOptions utility to ensure consistent precedence rules
across Browser and Node SDKs.
Includes comprehensive test coverage for all new utilities and integration
tests for environment variable precedence behavior.
Closes #18404