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(core): Version carrier rather than use SDK version #15132

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

timfish
Copy link
Collaborator

@timfish timfish commented Jan 22, 2025

The carrier was originally versioned in this PR:

We chose to use the SDK version as the key for the carrier. This means that if any SDK versions don't match, weird behaviour occurs where things just don't work:

While debugging the above issue myself, I also managed to install incompatible versions and it took me forever to work out what was going on.

The issues linked in #12188 were all caused by changes in the carrier interface between v7 and v8 and users upgrading. We don't need to update the carrier version on every release, only when the interface changes. This does add some extra things to consider in PR's and reviews but we're often thinking about semver everywhere anyway. I think this is preferable to what we have now where all versions have to match exactly.

This PR keeps __SENTRY__.version and how the versioning works so I don't think this would require changes in Spotlight or the loader.

Alternative Approaches

If we end up with multiple versions of @sentry/core, we could log warnings to the console. The downside here is it's not always easy to guide users in fixing this problem.

Copy link
Contributor

github-actions bot commented Jan 22, 2025

size-limit report 📦

Path Size % Change Change
@sentry/browser 23.05 KB +0.14% +31 B 🔺
@sentry/browser - with treeshaking flags 22.94 KB +0.14% +31 B 🔺
@sentry/browser (incl. Tracing) 35.78 KB +0.08% +27 B 🔺
@sentry/browser (incl. Tracing, Replay) 72.56 KB +0.04% +24 B 🔺
@sentry/browser (incl. Tracing, Replay) - with treeshaking flags 66.1 KB +0.03% +16 B 🔺
@sentry/browser (incl. Tracing, Replay with Canvas) 76.81 KB +0.03% +19 B 🔺
@sentry/browser (incl. Tracing, Replay, Feedback) 89.32 KB +0.05% +40 B 🔺
@sentry/browser (incl. Feedback) 39.74 KB +0.1% +38 B 🔺
@sentry/browser (incl. sendFeedback) 27.67 KB +0.08% +22 B 🔺
@sentry/browser (incl. FeedbackAsync) 32.45 KB +0.04% +13 B 🔺
@sentry/react 25.7 KB +0.02% +3 B 🔺
@sentry/react (incl. Tracing) 38.52 KB - -
@sentry/vue 27.13 KB +0.02% +5 B 🔺
@sentry/vue (incl. Tracing) 37.47 KB -0.01% -1 B 🔽
@sentry/svelte 23.17 KB +0.14% +32 B 🔺
CDN Bundle 24.26 KB +0.06% +14 B 🔺
CDN Bundle (incl. Tracing) 35.89 KB +0.03% +10 B 🔺
CDN Bundle (incl. Tracing, Replay) 70.52 KB +0.04% +24 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) 75.66 KB +0.02% +12 B 🔺
CDN Bundle - uncompressed 70.88 KB +0.02% +11 B 🔺
CDN Bundle (incl. Tracing) - uncompressed 106.51 KB +0.02% +17 B 🔺
CDN Bundle (incl. Tracing, Replay) - uncompressed 217.37 KB +0.01% +17 B 🔺
CDN Bundle (incl. Tracing, Replay, Feedback) - uncompressed 229.93 KB +0.01% +17 B 🔺
@sentry/nextjs (client) 38.64 KB +0.05% +17 B 🔺
@sentry/sveltekit (client) 36.26 KB -0.01% -2 B 🔽
@sentry/node 156.31 KB -0.03% -45 B 🔽
@sentry/node - without tracing 97.4 KB -0.04% -35 B 🔽
@sentry/aws-serverless 106.86 KB -0.04% -33 B 🔽

View base workflow run

@timfish timfish force-pushed the timfish/feat/carrier-version branch from 2ac5869 to dc47eb9 Compare January 22, 2025 14:02
@timfish timfish marked this pull request as ready for review January 22, 2025 14:19
@mydea
Copy link
Member

mydea commented Jan 22, 2025

Hmm I am not so sure. It is kind of desired (??) that this behaves this way, as even if the carrier structure does not change, stuff may still be incompatible - e.g. if anything on the scope or client changes, for example 🤔

Generally speaking, I think it is desired that all versions are aligned, and I think I'd rather work on fixing the problems that prevent us from ensuring this 🤔

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