Using activationStart if available #73
Conversation
There was a problem hiding this comment.
Thanks for taking a look at this @Jonathan1600!
I think I have a couple ideas where to go next. Left a comment below.
package-lock.json
Outdated
There was a problem hiding this comment.
Reminder: We're using yarn instead of npm in this package. Please remove this file!
src/index.ts
Outdated
| @@ -12,6 +12,9 @@ export type {Metric, MetricSubscriber} from './visuallyCompleteCalculator'; | |||
|
|
|||
| let calculator: VisuallyCompleteCalculator; | |||
|
|
|||
| // @ts-ignore | |||
| const startTime = performance?.getEntriesByType?.('navigation')[0]?.activationStart > 0 ? performance?.getEntriesByType?.('navigation')[0]?.activationStart : performance.now() | |||
There was a problem hiding this comment.
Okay, I think I spotted a couple issues here:
-
This line of code is declared at the top level so it will be evaluated immediately when the module is loaded, instead of when
start()is actually invoked. That means the value ofperformance.now()is probably not what you expect. -
You are intercepting the public
start()method. In that case, the user is telling us that a navigation is starting right now, and we always will want to override any default start time values. I would suggest trying to intercept the default value of thestart()method of the calculator class instead:
https://github.com/dropbox/ttvc/blob/main/src/visuallyCompleteCalculator.ts#L98
src/visuallyCompleteCalculator.ts
Outdated
| if (activationStart > 0) { | ||
| start = activationStart; | ||
| } |
There was a problem hiding this comment.
I think this should probably check if activationStart > start. The reason being, if someone does an SPA navigation on a page that was prerendered, start should still be the manually supplied value, correct?
I think it would be nice if we can add one or two puppeteer test cases for this. We can pair on that sometime if you like.
No description provided.