Skip to content
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): Add withIsolationScope #10141

Merged
merged 6 commits into from
Jan 12, 2024
Merged

Conversation

lforst
Copy link
Member

@lforst lforst commented Jan 10, 2024

This adds a global API withIsolationScope<T>((isolationScope: Scope) => T): T.

withIsolationScope() will fork the current isolationScope and the current scope. The callback is passed the isolation scope that is active within the callback.

withIsolationScope() is mostly intended for developing SDKs and is generally not recommended to be used by SDK consumers directly.

This function makes use of the provided async context strategy in the SDK. In the browser, this means it does effectively nothing as of now.


For reviewers:

This PR looks a bit goofy because it is literally wrapping runWithAsyncContext, however, this is probably exactly what we want this function to do until we have fully deprecated the hub.

We cannot make assertions in the core package that withIsolationScope() will fork the current isolation scope as this requires an async context strategy, which core by itself doesn't set. This is why the tests in the core package look a bit slim.

Copy link
Contributor

github-actions bot commented Jan 10, 2024

size-limit report 📦

Path Size
@sentry/browser (incl. Tracing, Replay, Feedback) - Webpack (gzipped) 76.99 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack (gzipped) 68.37 KB (0%)
@sentry/browser (incl. Tracing, Replay) - Webpack with treeshaking flags (gzipped) 62.02 KB (0%)
@sentry/browser (incl. Tracing) - Webpack (gzipped) 32.39 KB (0%)
@sentry/browser (incl. Feedback) - Webpack (gzipped) 30.99 KB (0%)
@sentry/browser - Webpack (gzipped) 22.33 KB (0%)
@sentry/browser (incl. Tracing, Replay, Feedback) - ES6 CDN Bundle (gzipped) 74.68 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (gzipped) 66.33 KB (+0.09% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (gzipped) 32.15 KB (+0.18% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped) 23.96 KB (+0.25% 🔺)
@sentry/browser (incl. Tracing, Replay) - ES6 CDN Bundle (minified & uncompressed) 208.93 KB (+0.08% 🔺)
@sentry/browser (incl. Tracing) - ES6 CDN Bundle (minified & uncompressed) 96.98 KB (+0.18% 🔺)
@sentry/browser - ES6 CDN Bundle (minified & uncompressed) 71.59 KB (+0.24% 🔺)
@sentry/browser (incl. Tracing) - ES5 CDN Bundle (gzipped) 35.17 KB (+0.17% 🔺)
@sentry/react (incl. Tracing, Replay) - Webpack (gzipped) 68.74 KB (0%)
@sentry/react - Webpack (gzipped) 22.37 KB (0%)
@sentry/nextjs Client (incl. Tracing, Replay) - Webpack (gzipped) 85.41 KB (0%)
@sentry/nextjs Client - Webpack (gzipped) 49.51 KB (0%)
@sentry-internal/feedback - Webpack (gzipped) 17 KB (0%)

@lforst lforst marked this pull request as ready for review January 11, 2024 11:23
@lforst lforst requested review from Lms24 and AbhiPrasad January 11, 2024 11:23
@@ -208,6 +209,15 @@ export function withScope<T>(
return getCurrentHub().withScope(rest[0]);
}

/**
* TODO
Copy link
Member

Choose a reason for hiding this comment

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

I think there's still something missing 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!

*/
export function withIsolationScope<T>(callback: (isolationScope: Scope) => T): T {
return runWithAsyncContext(() => {
return callback(getIsolationScope());
Copy link
Member

Choose a reason for hiding this comment

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

to sanity check: runWithAsyncContext forks the hub for the new async local storage and hence isolation and current scope, correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically the answer to your question is no. runWithAsyncContext only forks the hub when there is an async context strategy set. In the browser this is not the case so the isolation scope will always be the same. This may seem odd, but as a side-effect will prevent users, who think async context keeping works in the browser, from running into race conditions.

(Side note: This is also the reason why I didn't add any forking tests in the core package, simply because we cannot make this assumption.)

Copy link
Member

Choose a reason for hiding this comment

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

Right sorry, it's the async context strategy that takes care of it.

@lforst lforst merged commit e74dac7 into develop Jan 12, 2024
96 checks passed
@lforst lforst deleted the lforst-add-with-isolation-scope branch January 12, 2024 10:50
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.

2 participants