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(rrweb): Use unpatched requestAnimationFrame when possible #150

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Jan 4, 2024

As explained here: getsentry/sentry-javascript#6946 (comment) the usage of requestAnimationFrame can lead to issues when working with Zone.js.

With this fix, where possible we should now use the requestAnimationFrame implementation from an iframe, if possible, and else fall back on just using window.requestAnimationFrame as before.

We already do something similar in sentry-javascript to get an unpatched fetch: https://github.com/getsentry/sentry-javascript/blob/23ef22b115c8868861896cc9003bd4bb6afb0690/packages/browser/src/transports/utils.ts#L65-L71 so this should hopefully work fine!

mydea and others added 2 commits January 4, 2024 12:46
As explained here: getsentry/sentry-javascript#6946 (comment) the usage of `requestAnimationFrame` can lead to issues when working with Zone.js.
Copy link

github-actions bot commented Jan 4, 2024

size-limit report 📦

Path Size
rrweb - record only (gzipped) 16.67 KB (+0.64% 🔺)
rrweb - record & getCanvasManager only (gzipped) 19.23 KB (+0.49% 🔺)
rrweb - record only (min) 56.87 KB (+0.6% 🔺)
rrweb - record with treeshaking flags (gzipped) 15.46 KB (+0.73% 🔺)

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.

I like this solution! Thinking about it, we might run into some kind of trouble for no longer using a patched version of raf but let's worry about it if anyone complains 😅

Curious how much this is gonna improve perf on Angular!

Copy link
Member

@billyvg billyvg left a comment

Choose a reason for hiding this comment

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

Browser JS is wild

@mydea mydea merged commit fd9dc42 into sentry-v2 Jan 4, 2024
10 checks passed
@mydea mydea deleted the fn/unpatchedRequestAnimationFrame branch January 4, 2024 15:13
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Jan 5, 2024
- fix(rrweb): Use unpatched requestAnimationFrame when possible [#150](getsentry/rrweb#150)
- ref: Avoid async in canvas (#143)
- feat: Bundle canvas worker manually (#144)
- build: Build for ES2020 (#142)
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Jan 8, 2024
- fix(rrweb): Use unpatched requestAnimationFrame when possible [#150](getsentry/rrweb#150)
- ref: Avoid async in canvas (#143)
- feat: Bundle canvas worker manually (#144)
- build: Build for ES2020 (#142)
mydea added a commit to getsentry/sentry-javascript that referenced this pull request Jan 10, 2024
This bump contains the following changes:

- fix(rrweb): Use unpatched requestAnimationFrame when possible
[#150](getsentry/rrweb#150)
- ref: Avoid async in canvas
[#143](getsentry/rrweb#143)
- feat: Bundle canvas worker manually
[#144](getsentry/rrweb#144)
- build: Build for ES2020
[#142](getsentry/rrweb#142)

Extracted out from
#9826

Closes #6946
billyvg pushed a commit that referenced this pull request Apr 26, 2024
As explained here:
getsentry/sentry-javascript#6946 (comment)
the usage of `requestAnimationFrame` can lead to issues when working
with Zone.js.

With this fix, where possible we should now use the
`requestAnimationFrame` implementation from an iframe, if possible, and
else fall back on just using `window.requestAnimationFrame` as before.

We already do something similar in sentry-javascript to get an unpatched
`fetch`:
https://github.com/getsentry/sentry-javascript/blob/23ef22b115c8868861896cc9003bd4bb6afb0690/packages/browser/src/transports/utils.ts#L65-L71
so this should hopefully work fine!
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.

None yet

3 participants