Skip to content

Conversation

@Lms24
Copy link
Member

@Lms24 Lms24 commented Nov 9, 2022

In our initial implementation of release injection, we did not inject the SENTRY_RELEASES global variable which was added to the webpack plugin in getsentry/sentry-webpack-plugin#307.

Although it is unlikely that people are actually using this feature, it doesn't hurt us a lot if we implement it to prevent breakage for anyone who might manually be relying on this variable (e.g. MFE projects).

EDIT: We decided to make this behaviour opt-in by introducing a new injectReleasesMap option.


A little more context:

The idea of this variable is to have a map of projects that point to the same release. This seems to be a request for module federation and MFEs where it seems that multiple projects want to access the same release.

This original PR was merged, however it seems like our SDK never reads SENTRY_RELEASES. The PR author added this in a comment:

If this is accepted, we can maybe open another PR on @sentry/browser so that the default release is read from SENTRY_RELEASES and retire SENTRY_RELEASE

So what I'm thinking is that this just never happened. This however still means that people could actually be using this feature by manually accessing global.SENTRY_RELEASES and setting the version based on that.

@Lms24 Lms24 marked this pull request as draft November 9, 2022 13:15
@Lms24 Lms24 force-pushed the lms-inject-releases branch from 707d345 to b463d17 Compare November 9, 2022 13:16
@Lms24 Lms24 marked this pull request as ready for review November 9, 2022 13:17
@Lms24 Lms24 requested review from lobsterkatie and mydea November 9, 2022 13:17
@Lms24 Lms24 changed the title feat(core): Add SENTRY_RELEASES` variable during release injection feat(core): Add SENTRY_RELEASES variable during release injection Nov 9, 2022
const key = org ? `${project}@${org}` : project;
code += `
_global.SENTRY_RELEASES=_global.SENTRY_RELEASES || {};
_global.SENTRY_RELEASES["${key}"]={id:"${release}"};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: can we make this hidden behind a flag? I'd rather not inject things unnecessarily. I recognize this is a breaking change, but given this is undocumented I'm fine making the change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's fair and it would not bloat the bundle unnecessarily for all 99% of users. Will do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, is this something that people could have to opt-in to? If that's easy to do, I would say let people opt in (then, in a hypothetical world where we have a better understanding of which options are set by users, we can take better decisions in the future about this functionality). Seems like it is something pretty specific to want.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Lms24 Lms24 enabled auto-merge (squash) November 9, 2022 13:48
@Lms24 Lms24 force-pushed the lms-inject-releases branch from 1b63663 to ecac791 Compare November 9, 2022 13:51
@Lms24 Lms24 merged commit 3ecb9ff into main Nov 9, 2022
@Lms24 Lms24 deleted the lms-inject-releases branch November 9, 2022 13:58
@Lms24 Lms24 self-assigned this Nov 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants