-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(core): Remove getCurrentHub
from AsyncContextStrategy
#11581
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
feat(core): Remove getCurrentHub
from AsyncContextStrategy
#11581
Conversation
size-limit report 📦
|
export function getCurrentHub(): HubInterface { | ||
// Get main carrier (global for every environment) | ||
const carrier = getMainCarrier(); | ||
|
||
const acs = getAsyncContextStrategy(carrier); | ||
return acs.getCurrentHub() || getGlobalHub(); | ||
} |
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.
It's no longer necessary to read from the carrier here since acs doesn't have the getCurrentHub
method anymore. This means that we can replace this entire implementation with the shim which in turn will again forward its respective calls to the globalHub
(to be renamed).
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.
I think we need to remove getGlobalHub
usage and make everything rely on the shim, though that does mean we need to refactor more of the return values in getHubStackAsyncContextStrategy
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, I talked about this with @mydea and that's basically what we should do. getHubStackAsyncContextStrategy
has to stay in one way or the other since our fallback (i.e. browser) async context strategy is still a stack-based scope hierarchy. getGlobalHub
currently returns a Hub
class instance which we can probably cut down a lot to only handle the scope stack. At this point we can then also rename this to something like globalScopeStackAsyncContextStrategy
.
oh god I just opened a whole box of circular dependency problems - these are not fun to resolve 😅 update: nvm, the circular dep was caused by me importing |
4e91097
to
b0ae26d
Compare
21ff82b
to
e658aff
Compare
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.
The imports in this file caused a (rather unrelated but still existing) circular dependency in the core package.
This PR removes the
getCurrentHub
property from theAsyncContextStrategy
interface and consequently also the property in the interface implementers.There's still some stuff left to clean up but it is only internal anymore.
Hopefully the last breaking change in connection to #11482