Improve resize listener with the loadedmetadata event in DailyVideo
#11
Conversation
The current `onResize` callback may not fire on initial load. Since it relies on React’s render timing (using a combination of `useEffect` and `requestAnimationFrame`) instead of events on the video, the video stream may not be loaded in time for the initial `handleResize()` call. React has built-in support for `onResize` and `onLoadedMetadata` on `video` elements. This commit ditches the custom resize listener defined in `useEffect` in favor of these supported properties.
|
Thank you, @rileyjshaw, for the PR! While the change itself makes a lot of sense, and I'm supportive of doing this, it also requires the libraries' dependencies on I'm a little hesitant to apply this change now, because of the required dependency update for customers. Have you considered alternative solutions to the issue you're facing or do you think |
That makes sense! Listening for the |
|
That sounds like a plan, @rileyjshaw 👍 |
This preserves support for React versions below v18.
| @@ -167,13 +167,13 @@ export const DailyVideo = forwardRef<HTMLVideoElement, Props>( | |||
| if (!onResize || !video) return; | |||
|
|
|||
| let frame: ReturnType<typeof requestAnimationFrame>; | |||
| const handleResize = () => { | |||
| if (!video) return; | |||
There was a problem hiding this comment.
I believe video would have always been defined at this point. Instead, I added another check for videoEl.current further down.
|
|
||
| return () => video?.removeEventListener('resize', handleResize); | ||
| return () => { | ||
| if (frame) cancelAnimationFrame(frame); |
There was a problem hiding this comment.
I’m not sure why all of these request/cancelAnimationFrames are needed, but I assume it’s related to a weird video edge case that I don’t have context on (eg. a video loading in a background tab).
If these are only intended to reorder the React lifecycle, I think moving from a useEffect to a callback ref would let you delete these and simplify the code.
In any case, I think adding a cancelAnimationFrame here prevents a callback firing on an already unmounted component.
| }, | ||
| [onResize, videoTrack] |
There was a problem hiding this comment.
I removed videoTrack because I think the loadedmetadata and resize listeners should handle this without re-running the effect.
|
This change has been released with 0.7.2! |
|
Awesome, thanks for the shout-out! |
The current
onResizecallback may not fire on initial load. Since it relies on React’s render timing (using a combination ofuseEffectandrequestAnimationFrame) instead of events on the video, the video stream may not be loaded in time for the initialhandleResize()call.React has built-in support for
onResizeandonLoadedMetadataonvideoelements. This commit ditches the custom resize listener defined inuseEffectin favor of these supported properties.