-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
feat(browser): Add __SENTRY_RELEASE__
magic string
#6322
Conversation
// eslint-disable-next-line no-undef | ||
release: __SENTRY_RELEASE__, |
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.
m: Let's just check that removing this doesn't change/break anything for Gatsby
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.
Tested it in a gatsby app and it still works!
size-limit report 📦
|
packages/browser/src/sdk.ts
Outdated
@@ -97,6 +102,10 @@ export function init(options: BrowserOptions = {}): void { | |||
if (WINDOW.SENTRY_RELEASE && WINDOW.SENTRY_RELEASE.id) { | |||
options.release = WINDOW.SENTRY_RELEASE.id; | |||
} | |||
|
|||
if (typeof __SENTRY_RELEASE__ === 'string') { |
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.
l: Should this take precedence over a release set on window
? just thinking, to make sure this is what we expect :)
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.
Yes I think so. It's either that or the other way around.
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.
Hmm, you are more "into" these things, but I imagine it could be weird if you somehow set window.SENTRY_RELEASE
to a specific value, just to have it be overwritten by SENTRY_RELEASE?
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.
I would put it like this: I would give precedence depending on how much work it is to set a value. To set window.SENTRY_RELEASE
you just have to do exactly that - assign a value. To set __SENTRY_RELEASE__
you have to instruct your bundler to replace that string, probably even by using some plugin. I would argue that this is a lot more effort to set this value.
I guess there is the argument that we want to have the value be configurable via window even if the bundler injects something. I can change it.
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.
I guess there is the argument that we want to have the value be configurable via window even if the bundler injects something.
Yeah, that's basically my thought - if someone overwrites this at runtime or whenever, I think it may make sense to have this take precendence. But I don't have strong feelings on this, so feel free to also keep it as it is if you think that makes more sense 👍
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.
No I def like it. Will change!
@@ -97,6 +102,10 @@ export function init(options: BrowserOptions = {}): void { | |||
if (WINDOW.SENTRY_RELEASE && WINDOW.SENTRY_RELEASE.id) { |
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.
m: Should we deprecate and remove in v8?
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.
That would kill old webpack plugin compatibility. We definitely could. But I would probably delay it by a major or two.
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.
it would be a great way to get people to migrate to newer webpack plugin version, and also we can then point people to this as a feature in the first place when highlighting it in the changelog.
Well, this change make my build crash, as I was using |
Damn, sorry about that. Wasn't expecting to run into collisions. We will probably leave it at |
@@ -93,6 +98,11 @@ export function init(options: BrowserOptions = {}): void { | |||
options.defaultIntegrations = defaultIntegrations; | |||
} | |||
if (options.release === undefined) { | |||
// This allows build tooling to find-and-replace __SENTRY_RELEASE__ to inject a release value | |||
if (typeof __SENTRY_RELEASE__ === 'string') { | |||
options.release = __SENTRY_RELEASE__; |
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.
Potential pitfall of this approach is that any new release on user side will be changing vendor module source even if sentry version does not change. This will lead to new hash of vendor chunk of user app (which is usually the heaviest) and cache busting for end-users, need to download it again even if release contains only app changes.
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.
Hi, good point! This isn't used yet so we can change it at any point. Do you have suggestions on how to solve it differently?
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.
Simplest way is to provide release name as parameter at runtime.
Another way is to provide separate entry point that user need to include into build pipeline manually (or via bundler plugin, sentry webpack plugin does something similar). This entry file can have inline release name but user can choose chunk for this entry, for example inline chunk that will be inlined into index.html
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.
The SDK already has a way of configuring a release name during runtime: Sentry.init({ release: 'your-release-name'})
As for the separate entry point - this is something users can pretty easily do themselves. The SDK will currently already read the release name from window.SENTRY_RELEASE.id
or global.SENTRY_RELEASE.id
if it is set. This is the global variable that the webpack plugin and our other bundler plugins leverage to inject a release https://github.com/getsentry/sentry-javascript-bundler-plugins. If you want to disable the release injection done by the plugins you can use the releaseInjectionTargets
option.
Considering the above, I think we can confidently leave everything as-is. I like the idea of providing a separate entry point though. We will definitely consider it in the future.
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.
Thanks for mentioning releaseInjectionTargets
, was not aware of this option.
So considering this option and ability to provide release name at runtime, changes in PR can be mitigated with careful setup for those who care about long term caching.
Thank you for thoughtful reply.
This adds a magic string in the browser code that can be used by build tooling to inject a release value into the SDK during build time.
This is not used right now but it makes sense to introduce something like it as early as possible because, in the future, we want to move our bundler plugins to use this approach instead of injecting a variable on
global
.