⚡️ Process buffered performance entries in an idle callback #1337
Conversation
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
2 times, most recently
from
a06cd4e to
47a3d77
Compare
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
from
47a3d77 to
c6335ab
Compare
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
from
c6335ab to
2d3bb9a
Compare
| callback() | ||
| } | ||
| requestIdleCallback(callOnce) | ||
| lifeCycle.subscribe(LifeCycleEventType.BEFORE_UNLOAD, callOnce) |
There was a problem hiding this comment.
❓ question: what is the reasoning for calling it on unload?
There was a problem hiding this comment.
I don't know the detail behind the idle period detection but, FMU we can have cases where they are no idle periods before the unload. This is why the requestIdleCallback API provides a timeout option.
Btw the documentation states: "A timeout option is strongly recommended for required work, as otherwise it's possible multiple seconds will elapse before the callback is fired." but in our case BEFORE_UNLOAD seems more appropriate.
There was a problem hiding this comment.
IMO, these kind of behaviors should be explained in the code and tested.
I am wondering if this processing is not likely to be aborted by the unload anyway 🤔.
On the other hand, if we are worried about the requestIdleCallback not being called due to too much activity, let's just use a setTimeout, it should at least split this processing from the init.
wdyt?
There was a problem hiding this comment.
Other concern about the async call:
#1337 (comment)
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
2 times, most recently
from
2a52d2f to
b513656
Compare
| @@ -564,3 +564,23 @@ export function combine(...sources: any[]): unknown { | |||
| } | |||
|
|
|||
| export type TimeoutId = ReturnType<typeof setTimeout> | |||
|
|
|||
| export function requestIdleCallback(callback: () => void, opts?: { timeout?: number }) { | |||
There was a problem hiding this comment.
I think we could still let this function here as it could be valuable at some point in other packages but let me know.
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
from
b513656 to
238c980
Compare
amortemousque
force-pushed
the
aymeric/process-buffered-resources-in-an—idle-callback
branch
from
238c980 to
d94be5a
Compare
Codecov Report
@@ Coverage Diff @@
## main #1337 +/- ##
==========================================
- Coverage 91.07% 91.05% -0.03%
==========================================
Files 104 104
Lines 4268 4269 +1
Branches 950 950
==========================================
Hits 3887 3887
- Misses 381 382 +1
Continue to review full report at Codecov.
|
amortemousque
deleted the
aymeric/process-buffered-resources-in-an—idle-callback
branch
Motivation
Process buffered resources in an idle callback could prevent the SDK from blocking the main thread on page load too much.
Changes
Update performance collection
Testing
I have gone over the contributing documentation.