Skip to content
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.

Sign up for GitHub

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

Improve resize listener with the loadedmetadata event in DailyVideo #11

Closed
wants to merge 2 commits into from
Closed

Improve resize listener with the loadedmetadata event in DailyVideo #11

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a975c6a
Improve resize listener with the `loadedmetadata` event in `DailyVideo`
rileyjshaw Jan 10, 2023
12177ca
Use custom event handlers rather than React synthetic events
rileyjshaw Jan 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions src/components/DailyVideo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,13 @@ export const DailyVideo = forwardRef<HTMLVideoElement, Props>(
if (!onResize || !video) return;

let frame: ReturnType<typeof requestAnimationFrame>;
const handleResize = () => {
if (!video) return;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe video would have always been defined at this point. Instead, I added another check for videoEl.current further down.

function handleResize() {
if (frame) cancelAnimationFrame(frame);
frame = requestAnimationFrame(() => {
if (document.hidden) return;
const videoWidth = video?.videoWidth;
const videoHeight = video?.videoHeight;
const video = videoEl.current;
if (!video || document.hidden) return;
const videoWidth = video.videoWidth;
const videoHeight = video.videoHeight;
if (videoWidth && videoHeight) {
onResize({
aspectRatio: videoWidth / videoHeight,
Expand All @@ -182,14 +182,19 @@ export const DailyVideo = forwardRef<HTMLVideoElement, Props>(
});
}
});
};
}

handleResize();
video?.addEventListener('resize', handleResize);
video.addEventListener('loadedmetadata', handleResize);
video.addEventListener('resize', handleResize);

return () => video?.removeEventListener('resize', handleResize);
return () => {
if (frame) cancelAnimationFrame(frame);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

video.removeEventListener('loadedmetadata', handleResize);
video.removeEventListener('resize', handleResize);
};
},
[onResize, videoTrack]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed videoTrack because I think the loadedmetadata and resize listeners should handle this without re-running the effect.

[onResize]
);

return (
Expand Down