-
Notifications
You must be signed in to change notification settings - Fork 144
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
🐛 [RUM-8353] Throttle view context update #3314
Conversation
/to-staging |
Devflow running:
|
Integrated commit sha: a5e9c23 Co-authored-by: zcy <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3314 +/- ##
=======================================
Coverage 93.58% 93.59%
=======================================
Files 291 291
Lines 7691 7703 +12
Branches 1751 1751
=======================================
+ Hits 7198 7210 +12
Misses 493 493 ☔ View full report in Codecov by Sentry. |
094d837
to
c883c7e
Compare
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
|
||
// View context update should always be throttled | ||
contextManager.changeObservable.subscribe(() => { | ||
lifeCycle.notify(LifeCycleEventType.BEFORE_VIEW_UPDATED, { |
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.
💬 suggestion: I would expect BEFORE_VIEW_UPDATE to be called each time scheduleViewUpdate
or triggerViewUpdate
are called. Something like
const { throttled, cancel: cancelScheduleViewUpdate } = throttle(triggerViewUpdate, THROTTLE_VIEW_UPDATE_PERIOD, {
leading: false,
})
const scheduleViewUpdate = () => {
triggerBeforeViewUpdate()
throttled()
}
function triggerViewUpdate() {
triggerBeforeViewUpdate()
...
@@ -67,6 +67,13 @@ export interface ViewCreatedEvent { | |||
startClocks: ClocksState | |||
} | |||
|
|||
export interface ViewContextEvent { |
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.
💬 suggestion:
export interface ViewContextEvent { | |
export interface BeforeViewUpdateEvent { |
2a4ad53
to
030c7d1
Compare
@@ -924,6 +924,8 @@ describe('view event count', () => { | |||
const { getViewUpdate, setViewContext } = viewTest | |||
|
|||
setViewContext({ foo: 'bar' }) | |||
clock.tick(3000) |
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.
💬 suggestion: Use THROTTLE_VIEW_UPDATE_PERIOD
so we know where this value comes from and why we need it
030c7d1
to
c883c7e
Compare
Motivation
Updating of view context automatically triggers view updates, which could trigger excessive requests in edge cases. We should throttle view updates while maintaining current view context update behavior.
Changes
Added a new LifeCycleEvent type so we can update the view context for in viewHistory immediately
Use
scheduleViewUpdate
to throttle the view updatesTesting
I have gone over the contributing documentation.