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

fix(browser): Guard for this._flushOutcomes if old BaseClients are floating around #13085

Closed
wants to merge 2 commits into from

Conversation

lforst
Copy link
Member

@lforst lforst commented Jul 29, 2024

Fixes #13083

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Not sure how this happens given that recent versions shouldn't even look at the same global sentry carrier anymore but I have no objections to the fix 😅

packages/browser/src/client.ts Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jul 29, 2024

size-limit report 📦

Path Size
@sentry/browser 22.44 KB (+0.16% 🔺)
@sentry/browser (incl. Tracing) 34.22 KB (+1.1% 🔺)
@sentry/browser (incl. Tracing, Replay) 70.25 KB (+0.49% 🔺)
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 63.59 KB (+0.54% 🔺)
@sentry/browser (incl. Tracing, Replay with Canvas) 74.65 KB (+0.47% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback) 87.23 KB (+0.41% 🔺)
@sentry/browser (incl. Tracing, Replay, Feedback, metrics) 89.07 KB (+0.34% 🔺)
@sentry/browser (incl. metrics) 26.75 KB (+0.13% 🔺)
@sentry/browser (incl. Feedback) 39.36 KB (+0.11% 🔺)
@sentry/browser (incl. sendFeedback) 27.06 KB (+0.15% 🔺)
@sentry/browser (incl. FeedbackAsync) 31.69 KB (+0.12% 🔺)
@sentry/react 25.21 KB (+0.15% 🔺)
@sentry/react (incl. Tracing) 37.22 KB (+0.87% 🔺)
@sentry/vue 26.59 KB (+0.14% 🔺)
@sentry/vue (incl. Tracing) 36.05 KB (+0.99% 🔺)
@sentry/svelte 22.57 KB (+0.17% 🔺)
CDN Bundle 23.61 KB (+0.1% 🔺)
CDN Bundle (incl. Tracing) 35.87 KB (+0.83% 🔺)
CDN Bundle (incl. Tracing, Replay) 70.25 KB (+0.44% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) 75.52 KB (+0.41% 🔺)
CDN Bundle - uncompressed 69.33 KB (+0.02% 🔺)
CDN Bundle (incl. Tracing) - uncompressed 106.31 KB (+0.85% 🔺)
CDN Bundle (incl. Tracing, Replay) - uncompressed 217.95 KB (+0.41% 🔺)
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 230.77 KB (+0.39% 🔺)
@sentry/nextjs (client) 37.08 KB (+0.89% 🔺)
@sentry/sveltekit (client) 34.81 KB (+0.93% 🔺)
@sentry/node 111.9 KB (+0.01% 🔺)
@sentry/node - without tracing 89.33 KB (+0.01% 🔺)
@sentry/aws-serverless 98.49 KB (+0.01% 🔺)

Co-authored-by: Lukas Stracke <[email protected]>
@mydea
Copy link
Member

mydea commented Jul 29, 2024

Not sure how this happens given that recent versions shouldn't even look at the same global sentry carrier anymore but I have no objections to the fix 😅

In that case the error must be at build time, actually, I believe - as the browser client is extending the base client, and it seems (??) that the base client it is extending is not the v8 base client (I guess??) 🤔

@Lms24
Copy link
Member

Lms24 commented Jul 29, 2024

that the base client it is extending is not the v8 base client (I guess??) 🤔

This rather screams module resolution error then, no? 🤔

@mydea
Copy link
Member

mydea commented Jul 30, 2024

that the base client it is extending is not the v8 base client (I guess??) 🤔

This rather screams module resolution error then, no? 🤔

🤷 possibly? 😅 Always hard to be sure in such cases, but I think it has to be that, because we never call this on another ("random") client, but only on this 🤔

@Lms24
Copy link
Member

Lms24 commented Jul 30, 2024

Again, I think it's fine to merge this if it helps people. Just surprised that this is happening 😅

@lforst
Copy link
Member Author

lforst commented Jul 30, 2024

I kinda don't want to merge this because of reasons outlined in this comment: #13083 (comment)

@lforst
Copy link
Member Author

lforst commented Aug 28, 2024

Closing bc stale

@lforst lforst closed this Aug 28, 2024
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.

this._flushOutcomes is not a function
3 participants