-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(browser): Enable Spotlight via SENTRY_SPOTLIGHT env var #18050
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
BYK
wants to merge
4
commits into
develop
Choose a base branch
from
feat/browser-spotlight-env-var
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
70513b7
feat(browser): Enable Spotlight via SENTRY_SPOTLIGHT env var
BYK 7e0d482
meta(changelog): Add entry for Spotlight env var support
BYK ea50a4a
meta(changelog): Clean up CHANGELOG entry
BYK 03877a9
meta(changelog): Document truthy/falsy values for SENTRY_SPOTLIGHT
BYK File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So,
process.envwill almost never* be replaced by bundlers, which means that this code path will almost never work. The only bundler that straight up adds support forprocess.env.Xseems 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 whileprocessmight be undefined,process.env.SENTRY_SPOTLIGHTcould 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 defineVITE_SENTRY_SPOTLIGHTinstead ofSENTRY_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 viaenvPrefix.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 thatprocess.env.NEXT_PUBLIC_SENTRY_SPOTLIGHTwould work. SvelteKit allows env variables prefixed withPUBLIC_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|...).tsfiles with the respective variables defined in anenvironmentobject.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_SPOTLIGHTmight 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|falseon 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:
The most important aspects of this function are:
falserather than true because otherwise, we risk sending stuff to spotlight in a live prod build.Overall, I'd still recommend going with b.