-
-
Notifications
You must be signed in to change notification settings - Fork 343
feat(replay): Allow using browserReplayIntegration without isWeb guard #4858
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
Conversation
|
iOS (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 1216.66 ms | 1212.06 ms | -4.60 ms |
9da5c4e+dirty | 1231.84 ms | 1235.49 ms | 3.65 ms |
8a54dff+dirty | 1226.17 ms | 1227.79 ms | 1.63 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 2.63 MiB | 3.77 MiB | 1.14 MiB |
9da5c4e+dirty | 2.63 MiB | 3.76 MiB | 1.13 MiB |
8a54dff+dirty | 2.63 MiB | 3.79 MiB | 1.16 MiB |
iOS (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 1215.22 ms | 1222.26 ms | 7.04 ms |
9da5c4e+dirty | 1215.41 ms | 1226.38 ms | 10.97 ms |
8a54dff+dirty | 1244.29 ms | 1254.71 ms | 10.43 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
9fbcfaf+dirty | 3.19 MiB | 4.34 MiB | 1.15 MiB |
9da5c4e+dirty | 3.19 MiB | 4.33 MiB | 1.14 MiB |
8a54dff+dirty | 3.19 MiB | 4.35 MiB | 1.17 MiB |
Android (legacy) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8a54dff | 482.57 ms | 467.16 ms | -15.41 ms |
9fbcfaf | 420.06 ms | 435.31 ms | 15.25 ms |
9da5c4e | 478.08 ms | 467.46 ms | -10.63 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8a54dff | 17.75 MiB | 19.54 MiB | 1.80 MiB |
9fbcfaf | 17.75 MiB | 19.54 MiB | 1.79 MiB |
9da5c4e | 17.75 MiB | 20.16 MiB | 2.41 MiB |
Android (new) Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8a54dff+dirty | 393.65 ms | 392.32 ms | -1.33 ms |
9fbcfaf+dirty | 417.92 ms | 431.81 ms | 13.90 ms |
9da5c4e+dirty | 399.70 ms | 407.34 ms | 7.64 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
8a54dff+dirty | 7.15 MiB | 8.30 MiB | 1.15 MiB |
9fbcfaf+dirty | 7.15 MiB | 8.29 MiB | 1.14 MiB |
9da5c4e+dirty | 7.15 MiB | 8.42 MiB | 1.27 MiB |
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.
LGTM 🚀
Thank you for the added test 🙇
Q: isn't this change going to increase the SDK size for users by always bundling the web replay? |
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Good point, but no, this is not changing the bundle size, unless excluded by the Sentry Metro Plugin the Browser Replay is always included in the bundle already. |
📢 Type of change
📜 Description
With this change users don't have to wrap
browserReplayIntegration
withisWeb
guard.And MobileReplay can use in the future the same interface for the manual controls.
The change of the interface is possible breaking change and the base of the PR is v7.
💚 How did you test it?
unit test, sample app
📝 Checklist
sendDefaultPII
is enabled