-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
build(replay): Provide full browser+tracing+replay bundle #6793
Conversation
size-limit report 📦
|
20272b7
to
7265349
Compare
7265349
to
77c2dc0
Compare
replace({ | ||
preventAssignment: true, | ||
values: { | ||
__SENTRY_REPLAY_VERSION__: JSON.stringify(pkg.version), |
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.
Reminder to ourselves: In the next version, we should get rid of this. By now all docs etc. point to importing replay directly from @sentry/browser, so this should always be in sync.
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.
Can we document this here #5194 or in a new issue?
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.
Oh wait you mean next version of the SDK, not next major version. Ignore me!
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 don't think this needs to be in a major release. We're tracking this in #6366
|
||
import * as Sentry from './index.bundle'; | ||
|
||
Sentry.Integrations.Replay = Replay; |
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 was the easiest/cleanest way I could find to handle this in a dedicated input file. Otherwise it gets quite messy when trying to do this inside of index.bundle.ts
😬
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.
Note: This means we only have a single default export here, and can't do our usual import * as Sentry
stuff. but since we only use this to generate the bundle, and there it works as expected, I'd say that's fine?
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 this is fine and with this change, the full CDN bundle usage is identical to the addon bundle (docs).
29d3018
to
f9bd33c
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.
Looks good! I wanna go over this tomorrow again to double check I didn't miss anything but great work!
src="https://browser.sentry-cdn.com/7.24.1/bundle.tracing.min.js" | ||
crossorigin="anonymous" | ||
></script> | ||
|
||
// Replay integration bundle | ||
<script | ||
src="https://browser.sentry-cdn.com/7.24.1/replay.min.js" |
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.
l: Hmm it probably makes sense to show the full CDN bundle here but I'd like to keep the addon bundle around and documented (at least in docs) as I think some people might prefer it 🤔 Especially, if we're considering using the loader to lazy load it in the future.
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.
Hmm yeah, I guess the reason to use it is if you want replay but not tracing - fair! I can add both somehow - probably leave the "full" bundle on top and add a section below on standalone usage.
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.
Added another section below 👍
|
||
import * as Sentry from './index.bundle'; | ||
|
||
Sentry.Integrations.Replay = Replay; |
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 this is fine and with this change, the full CDN bundle usage is identical to the addon bundle (docs).
f9bd33c
to
f5ec17a
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.
Ok, double checked and I think this looks good 🚀
Also checked that we're uploading this as an artifact on release branches ✅
// Replay integration bundle | ||
<script | ||
src="https://browser.sentry-cdn.com/7.24.1/replay.min.js" | ||
src="https://browser.sentry-cdn.com/7.31.0/bundle.tracing.replay.min.js" |
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.
Can't comment there directly but:
L95-96: Let's remove the text as this doesn't apply anymore if we tell them in this snippet to use the full bundle.
f5ec17a
to
53c298b
Compare
Adds another CDN bundle
bundle.tracing.replay.js
which includes both tracing and replay.