Skip to content

feat(browser): Add __SENTRY_RELEASE__ magic string #6322

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Nov 30, 2022
Merged

feat(browser): Add __SENTRY_RELEASE__ magic string #6322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd1aa68
feat(browser): Add `__SENTRY_RELEASE__` magic string
Nov 29, 2022
9c193e0
Fix test
Nov 29, 2022
20fe284
Move
Nov 29, 2022
5d9ea17
Fix test
Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/browser/src/sdk.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,11 @@ export const defaultIntegrations = [
new HttpContext(),
];

/**
* A magic string that build tooling can leverage in order to inject a release value into the SDK.
*/
declare const __SENTRY_RELEASE__: string | undefined;

/**
* The Sentry Browser SDK Client.
*
Expand Down Expand Up @@ -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__;
Copy link

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.

Copy link
Contributor Author

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?

Copy link

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

Copy link
Contributor Author

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.

Copy link

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 supports the variable that sentry-webpack-plugin injects
if (WINDOW.SENTRY_RELEASE && WINDOW.SENTRY_RELEASE.id) {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

@AbhiPrasad AbhiPrasad Nov 29, 2022 •
edited
Loading

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.

options.release = WINDOW.SENTRY_RELEASE.id;
Expand Down
2 changes: 0 additions & 2 deletions packages/gatsby/gatsby-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ exports.onClientEntry = function (_, pluginParams) {
}

Sentry.init({
// eslint-disable-next-line no-undef
release: __SENTRY_RELEASE__,
Comment on lines -28 to -29
Copy link
Member

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

Copy link
Contributor Author

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!

// eslint-disable-next-line no-undef
dsn: __SENTRY_DSN__,
...pluginParams,
Expand Down
4 changes: 2 additions & 2 deletions packages/gatsby/test/gatsby-browser.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,8 @@ describe('onClientEntry', () => {
});

it.each([
[{}, ['dsn', 'release']],
[{ key: 'value' }, ['dsn', 'release', 'key']],
[{}, ['dsn']],
[{ key: 'value' }, ['dsn', 'key']],
])('inits Sentry by default', (pluginParams, expectedKeys) => {
onClientEntry(undefined, pluginParams);
expect(sentryInit).toHaveBeenCalledTimes(1);
Expand Down