Added a check for spotlight v1.x.x to not load sentry sdk >= v8 #401
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Good idea! heads-up, I'm gonna work soon on the v2 stable version. Just need to adjust the window.__SENTRY__ code a bit to get it working with getsentry/sentry-javascript#12206
Anyway, this isn't relevant for this PR as we'll merge it into v1
| notificationCount: { | ||
| count: errorsCount, | ||
| severe: errorsCount > 0, | ||
| if (checkBrowserSDKVersion()) { |
There was a problem hiding this comment.
Can we extract the two integrations here so that not the entire code is indented by checkBrowserSDKVersion?
There was a problem hiding this comment.
So basically, do something like:
if (checkBrowserSDKVersion()) {
return _sentryIntegration();
} else {
return _sdkNotCompatibleSentryIntegration()
}
No particular strong feelings about naming or details just wanna keep this somewhat readable :)
| import { WindowWithSentry } from '../types'; | ||
|
|
||
| export function checkBrowserSDKVersion() { | ||
| const spotlightV1SupportedSentryRegex = new RegExp(SDK_VERSION_SUPPORT_REGEX); |
There was a problem hiding this comment.
l: I'd rather just inline the regex here instead of initializing a new RegExp with the constant. Makes things a bit simpler and as far as I can tell, we're not reusing the regex.
| @@ -7,17 +7,17 @@ import { Tabs, TabItem } from '@astrojs/starlight/components'; | |||
| <Tabs> | |||
| <TabItem label="npm"> | |||
| ```sh | |||
| npm add @spotlightjs/spotlight | |||
| npm i @spotlightjs/spotlight --save-dev | |||
and PR comments resolved
|
hey @Lms24
|
|
@Shubhdeep12 thanks a lot for making the changes! I think this looks good but I just had a revelation: The plan for the v2 release is to merge With this in mind, we don't necessarily want this commit in the history for v2, but only for v1.So I suggest we wait with merging until v2 is stable and only then merge it into the I'm targeting Monday for v2 stable by the way so the wait shouldn't be too long 🤞 |
|
Sounds great! @Lms24 The main reason for this patch was for the users who were getting confused and were trying to use the spotlight with sentry v8. |
|
Yup, that makes sense, as I said, I think the change is a good idea! Also, waiting for stable gives us the additional bonus that people will feel less sceptical about updating to a stable version than to an alpha version. |
Shubhdeep12
changed the title
Shubhdeep12
changed the title
|
Is this still relevant @Lms24 and @Shubhdeep12? |
|
Yup, still relevant. Should go into |
|
Is there a strong reason for us to support 1.x branch and do hotfixes for that? |
All new fixes and features will be added to 2.x(main branch) only. This specific fix is only added for the users using spotlight v 1.x with sentry sdk v >=8.x because there was a breaking change in sentry js sdk v8.x, which spotlight 1.x doesn't support. |
Understand this part. Asking if there's anyone out there relying on v1.x or a good reason for us to maintain v1.x (otherwise I'll just close this PR and do not support v1) |
|
@BYK Seems like v1 is still used by a lot of people. If it's not too much work, I'd vote we merge this and cut a final v1 release to let people know about the need to update to v2 if they use a v8 JS SDK. No strong feelings though, I'll let you make the call. |
Co-authored-by: Lukas Stracke <[email protected]>
Before opening this PR:
pnpm changeset:addAdded a check for sentry integration in Spotlight, for v1.x.x to load only if -