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

feat: Ensure to use unwrapped versions of setTimeout / clearTimeout #176

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

mydea
Copy link
Member

@mydea mydea commented Apr 18, 2024

Let's see if that helps with Angular performance some more...!

Closes getsentry/sentry-javascript#11661 (hopefully...)

Let's see if that helps with Angular performance some more...!
@mydea mydea requested review from billyvg and c298lee April 18, 2024 07:32
@mydea mydea self-assigned this Apr 18, 2024
Copy link

size-limit report 📦

Path Size
rrweb - record only (gzipped) 16.8 KB (+0.2% 🔺)
rrweb - record & CanvasManager only (gzipped) 19.61 KB (+0.14% 🔺)
rrweb - record only (min) 57.24 KB (+0.07% 🔺)
rrweb - record with treeshaking flags (gzipped) 15.59 KB (+0.22% 🔺)

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.

Very interesting! It kinda makes sense after thinking about it. ZoneJS patches all of these global APIs so let's see 🍿

(long term this might phase out with Angular becoming zone-less in favor of signals but this will take a long time until it's used broadly)

@mydea
Copy link
Member Author

mydea commented Apr 18, 2024

Very interesting! It kinda makes sense after thinking about it. ZoneJS patches all of these global APIs so let's see 🍿

(long term this might phase out with Angular becoming zone-less in favor of signals but this will take a long time until it's used broadly)

Yeah, I'd say the bundle size hit is acceptable here, and we can revert it later if we want/need to!

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.

🙏

We will also want to do this for the setTimeouts in our replay package

@mydea mydea merged commit d2cdc6f into sentry-v2 Apr 19, 2024
14 checks passed
@mydea mydea deleted the fn/setTimeoutNative branch April 19, 2024 07:59
@mydea
Copy link
Member Author

mydea commented Apr 19, 2024

🙏

We will also want to do this for the setTimeouts in our replay package

Let's ship this, I'd say, and see if it already improves things, than we can also add this to replay itself, at least in hot paths related to rrweb events etc!

@billyvg
Copy link
Member

billyvg commented Apr 19, 2024

🙏
We will also want to do this for the setTimeouts in our replay package

Let's ship this, I'd say, and see if it already improves things, than we can also add this to replay itself, at least in hot paths related to rrweb events etc!

@mydea when I was debugging, I saw that it was being triggered by our click handler in the replay package

@mydea
Copy link
Member Author

mydea commented Apr 22, 2024

🙏
We will also want to do this for the setTimeouts in our replay package

Let's ship this, I'd say, and see if it already improves things, than we can also add this to replay itself, at least in hot paths related to rrweb events etc!

@mydea when I was debugging, I saw that it was being triggered by our click handler in the replay package

sad! I guess let's implement it there as well then, will add some bundle size but I guess it's worth it!

billyvg pushed a commit that referenced this pull request Apr 26, 2024
…t` (#176)

Let's see if that helps with Angular performance some more...!

Closes getsentry/sentry-javascript#11661
(hopefully...)
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.

Replay hinders performance on Angular apps
3 participants