search

benwiley4000 / cassette

📼 A flexible media player component library for React that requires no up-front config
https://benwiley4000.github.io/cassette/styleguide
MIT License
185 stars 28 forks source link

Prevent errors on componentWillUnmount #343

Closed danielr18 closed 5 years ago

danielr18 commented 5 years ago

Similar to #342, but for the rest of components that use componentWillUnmount, it makes sure that the variables exist before doing their cleaning.

benwiley4000 commented 5 years ago

I understand the checks for MaybeMarquee and VideoDisplay since they rely on MutationObserver, but I'm wondering why we have checks in ProgressBar and VolumeControl. According to MDN appendChild and addEventListener are each supported in IE9, which is the minimum browser version that React 16 supports. Do you have a case where this helps you?

For the checks we do include, I would like comments similar to those for the last PR. It would be sufficient to reference the componentWillUnmount of PlayerContextProvider (like so: https://github.com/benwiley4000/cassette/blob/next/packages/core/src/PlayerContextProvider.js#L1043).

danielr18 commented 5 years ago

I actually made this PR because I got at error in ProgressBar, caused by this.noselectStyleElement not being defined, my guess is that because it's set on the setTimeout, when another component's componentDidMount fails, the boundary unmounts all component, and the callback that defines this.noselectStyleElement is not called.

danielr18 commented 5 years ago

The one in VideoDisplay was just in case to be honest, but it should always be set by the time componentWillUnmount is called, I'll remove it.

benwiley4000 commented 5 years ago

Ah, that makes sense. I'd guess the timeout callback actually does get called, but it happens after unmount if you unmount it synchronously following the error boundary lifecycle hook.

benwiley4000 commented 5 years ago

Could you include a note about what we discussed above in the comment for ProgressBar's check? I just want to make sure it's clear why we think the style element could be missing.

danielr18 commented 5 years ago

I've just added the comments

benwiley4000 commented 5 years ago

I think there's a couple things I should reword slightly but I'll do that after merging. Thank you for this!!

benwiley4000 commented 5 years ago

@danielr18 did you experience problems with MaybeMarquee or VideoDisplay specifically? I just looked at the readme for the ResizeObserver polyfill and it says it falls back to mutation events if the MutationObserver isn't supported. https://github.com/que-etc/resize-observer-polyfill So if there are issues I'm wondering what would cause them. The polyfill is supposed to support IE9 and up.

benwiley4000 commented 5 years ago

Published in 2.0.0-alpha.19 ☺️

danielr18 commented 5 years ago

My bad regarding ResizeObserver, I forgot that it was using the polyfill. I haven't had any issues logged yet regarding ResizeObserver.

benwiley4000 commented 5 years ago

No worries, thank you for clarifying. I missed it too. :slightly_smiling_face: And the checks didn't hurt anything.

I just reverted that part of the change and released a new version (alpha.20). Of course let me know if you run into other issues.