Uh oh!
There was an error while loading. Please reload this page.
feat(core): Deprecate session APIs on hub and add global replacements - #10054
Conversation
| } | ||
| const session = scope && scope.getSession(); | ||
| const session = (scope && scope.getSession()) || getIsolationScope().getSession(); |
There was a problem hiding this comment.
This makes me a bit uncomfortable tbh, not sure if we for now should simply still use hub.startSession in browser/node startSessionTracking. But this change seems to be necessary for browser, since scope.getSession() returned undefined
There was a problem hiding this comment.
hmm, is this correct in the new implementation, I wonder? Or should we simply always take the one from the isolation scope? 🤔 because the scope here would generally be the final scope being applied and can even be a temporary scope, like new Scope() 🤔
There was a problem hiding this comment.
thinking about this, I guess it doesn't hurt, if you pass a scope that actually has a session on it I guess it makes sense that we use this. But it will/should use the isolationScope in 99% of cases I guess?
There was a problem hiding this comment.
hmm, is this correct in the new implementation, I wonder?
You mean that we set it onto the isolation scope? I think that should be correct 🤔
EDIT: But not "complete" (see answer below)
Or should we simply always take the one from the isolation scope? 🤔
I guess it would be more correct if we're okay with not supporting people adding their own session to their current scope 🤔 Not sure if that's even a thing...
There was a problem hiding this comment.
Oh wait, for v7 we probably still need to set the session also on the current scope, right? So that getCurrentHub().getScope().getSession() still returns the session
There was a problem hiding this comment.
This probably makes calling getIsolationScope obsolete here
There was a problem hiding this comment.
Reworked the top level APIs to also modify the current scope's session. For now this will work and spare us mocking with event processing
size-limit report 📦
|
| const client = getClient(); | ||
| const isolationScope = getIsolationScope(); | ||
| const { release, environment = DEFAULT_ENVIRONMENT } = (client && client.getOptions()) || {}; |
There was a problem hiding this comment.
l: should we maybe just bail out if no client is found? Doesn't matter probably, but for clarity...
There was a problem hiding this comment.
Hmm thinking about this, if we do so, we have to return undefined which I'm not sure we want to do. Also this means the signature of the replacement would slightly differ from the deprecated hub method. Since we're only taking env and release from the client I think it's okay to continue. Obviously, no
session will be sent but to me this aligns more with the concept of no-op spans for example.
| }); | ||
| // End existing session if there's one | ||
| const currentSession = isolationScope.getSession && isolationScope.getSession(); |
There was a problem hiding this comment.
l: IMHO no need to check for getSession existance, if you have code without this everything will fail here anyhow I guess xD
7c8f841 to
f392633CompareUh oh!
There was an error while loading. Please reload this page.
24181c7 to
566016dCompare…#10054) Deprecates and adds top-level replacements for * `hub.startSession` -> `Sentry.startSession` * `hub.endSession` -> `Sentry.endSession` * `hub.captureException` -> `Sentry.captureSession` These APIs were mostly used in browser and node `startSessionTracking` functions which were called on `Sentry.init`. I updated these usages with the global replacements and it seems integration tests are happy with it. For now, we not only put the session onto the isolation scope but also onto the current scope to avoid changing event processing and stuff like the ANR worker, as well as avoid "soft-breaking" users who use `hub.(get|set)Session`.
Deprecates and adds top-level replacements for
hub.startSession->Sentry.startSessionhub.endSession->Sentry.endSessionhub.captureException->Sentry.captureSessionThese APIs were mostly used in browser and node
startSessionTrackingfunctions which were called onSentry.init. I updated these usages with the global replacements and it seems integration tests are happy with it.For now, we not only put the session onto the isolation scope but also onto the current scope to avoid changing event processing and stuff like the ANR worker, as well as avoid "soft-breaking" users who use
hub.(get|set)Session.Also, it looks like we didn't have unit tests for session APIs before (but we did have integration tests). Therefore
hub.startSessionAPIref #10033