-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: Make sure that getSession exists before calling it #3017
Conversation
size-limit report
|
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.
LGTM
👋 we upgraded to Wondering if this didn't quite fix (seems like it should have 🤷) Or maybe this fix hasn't been pushed everywhere yet? It doesn't seem to be in https://browser.sentry-cdn.com/5.27.2/bundle.min.js |
@@ -522,7 +522,7 @@ export abstract class BaseClient<B extends Backend, O extends Options> implement | |||
throw new SentryError('`beforeSend` returned `null`, will not send event.'); | |||
} | |||
|
|||
const session = scope && scope.getSession(); | |||
const session = scope && scope.getSession && scope.getSession(); |
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.
What about simply using ?.
? (same comment for the second file in this PR)
const session = scope && scope.getSession?.();
Is its compatibility enough for you?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Optional_chaining#Browser_compatibility
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, we could do that, as we're compiling it down to ES5 anyway. Good catch.
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.
For that matter, couldn't we just do
const session = scope?.getSession?.();
?
This can happen when malformed node_modules are using older version of
hub
, butcore
wants to call that method. Or when there are 2 versions of Sentry living next to each other for some unknown reason. Fix is straightforward enough that I'm fine to include it, and we already got 2 customers reporting this issue.