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
feat(replay): Use unwrapped setTimeout
to avoid e.g. angular change detection
#11924
Conversation
size-limit report 📦
|
0c5bfa2
to
6940018
Compare
|
c4cf4ec
to
188f306
Compare
This moves some code around in `browser-utils` so we can-reuse the logic for getting the unwrapped fetch implementation to also get the unwrapped `setTimeout` implementation. E.g. Angular wraps this for change detection, which can lead to performance degration.
This reverts commit 78221f5.
…rted as early as possible
188f306
to
8cd00ba
Compare
function isNative(func: Function): boolean { | ||
return func && /^function\s+\w+\(\)\s+\{\s+\[native code\]\s+\}$/.test(func.toString()); | ||
} |
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 added this, which is a generic version of this: https://github.com/getsentry/sentry-javascript/blob/develop/packages/browser/src/transports/utils.ts#L56-L58
Without this tracing/metrics/web-vitals-fid
was failing consistently. Is the sandboxed version just too slow for our browser tests? cc @mydea
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 this looks reasonable. TBH the web-vitals tests are a bit of a black box to me, they appear/are brittle overall 😬 if that makes it pass, all good from my POV!
This makes the `isNativeFetch` check a bit more generic/naive. It no longer checks that the function name matches `fetch` explicitly, but I think thats ok. Follow-up to #11924
This PR makes sure we use the native, unwrapped
setTimeout
implementation of the browser. Some environments, e.g. Angular, monkey patch this for their change detection, leading to performance issues (possibly). We have already changed this in rrweb, but we also have some usage of this in replay itself.This PR should work fine, however all test fail today because we heavily use
jest.useFakeTimers()
, which basically monkey patches fetch too. So with this change, we do not use the patched timers, leading to everything blowing up 🤯Based on #11864
Depends on #11899