search

mderrick / react-html5video

A customizeable HTML5 Video React component with i18n and a11y.
http://mderrick.github.io/react-html5video/
MIT License
459 stars 123 forks source link

Prevents recreation of ref function on every render #65

Closed aarontam closed 7 years ago

aarontam commented 7 years ago

Issue

We are using the Video component within a larger component that includes some elements that need regular updating. As a result, we noticed that the ref to the <video> element would be assigned to null and then to the DOM node for each re-render. This caused some transitory issues when attempting to access the ref in things like event handlers.

Fix

By defining the ref function within the JSX, that function is being recreated on each render. As a result, the "old" function receives a null node ref, and the "new" function receives the actual node ref on each render (https://github.com/facebook/react/issues/4533#issuecomment-126807678). We've moved that function onto the component itself so that it is only created once.

P.S. My editor automatically removes trailing spaces - let me know if this is a problem and I can remove those changes.

mderrick commented 7 years ago

This is an interesting problem and seems like reasonable behaviour of refs I didn't fully consider. I think in general we should check this.videoEl exists before using it anyway (e.g the component has genuinely unmounted before a function using the reference has fired). Regardless, this looks like a better practice and saves us running that method repeatedly.

Thanks for the PR. Will look at merging this and releasing over the next week.

aarontam commented 7 years ago

Adding guards for the video element make sense - I can make that part of this PR, as well - let me know.

Looking forward to the new release - thanks for building a very useful component! 👍

aarontam commented 7 years ago

Happy New Year! 🎊 🎉

Curious if there was any news on when this PR would be taken and/or a new release was going to be made. Thanks!

mderrick commented 7 years ago

Hi @aarontam sorry I've been travelling for a few months so have been inundated with other commitments as well as having little to no internet connection. I have implemented this change in the new v2 branch which I hope to release in the coming months. If I can find a decent internet connection I will try and release this one to master and publish.

mderrick commented 7 years ago

I've release this fix in v2. Thanks!