spodlecki / videojs-event-tracking

Track events with VideoJS and keep an eye on performance metrics
MIT License
71 stars 16 forks source link

Add failsafe to get duration during `timeupdate` #20

Closed IllyaMoskvin closed 3 years ago

IllyaMoskvin commented 3 years ago

We noticed that sometimes, unpredictably, the 25-50-75 percentile events would not fire. We could not figure out a reliable way to reproduce the error, but if you add a breakpoint around line 55 here:

https://github.com/spodlecki/videojs-event-tracking/blob/204dba2ac76da072cd6bf8745c5bb2d8658068e5/src/tracking/percentile.js#L47-L71

...and repeatedly refresh the page while trying to play a video.js player with these events bound, eventually, you may run into a situation where duration is equal to 0.

It seems that either the durationchange event never fired, or it fired before the watcher got bound. In either case, for whatever reason, the durationchange callback here:

https://github.com/spodlecki/videojs-event-tracking/blob/204dba2ac76da072cd6bf8745c5bb2d8658068e5/src/tracking/percentile.js#L83-L93

...never ran in those bugged-out instances, so the first, second, and third variables never got set.

(I'm guessing this could be a browser bug? We are using Chrome 92 on macOS.)

This pull request aims to fix that issue by adding an additional check for duration during the timeupdate event.

I confirmed that this fix works by adding a breakpoint on the getDuration() call inside timeupdate. By doing the same refresh-page-and-hit-play, I was able to confirm that the code at the breakpoint does occasionally get called (i.e. when it would have broken before), and the percentile events do fire as expected.

spodlecki commented 3 years ago

awesome catch! I'm not sure how this actually happens, but I love the solution

IllyaMoskvin commented 3 years ago

Sweet! Thank you for the merge! Would you be down to publish a new version to the npm registry?

spodlecki commented 3 years ago

published 1.0.2 - all set. Thank you for the contribution