video.js icon indicating copy to clipboard operation
video.js copied to clipboard

React 18 StrictMode does not work with typical usage of video.js

Open goffioul opened this issue 2 years ago • 10 comments

Description

React 18 StrictMode comes with additional behaviors to prepare for reusable state. In particular, when a component is first mounted, React simulates mount+unmount+mount [1]. A typical integration as described in the guides [2] does not work anymore and lead to the following warning (and no video DOM element present in the page):

VIDEOJS: WARN: The element supplied is not included in the DOM

The reason is that when React simulates unmount, the video.js dispose removes the video element from the DOM, which invalidates the ref that React held.

This is illustrated in the following sandbox: https://codesandbox.io/s/react-videojs-strictmode-kvkk4c

The easy route is for each project to blame the other: react should not simulate mount+unmount+mount, video.js should not remove the DOM element on dispose. But as an application developer, I'm creating this bug in the hope to trigger a discussion between the projects and offer a solution for developers (other than do not use StrictMode).

[1] https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-strict-mode [2] https://videojs.com/guides/react/

Steps to reproduce

https://codesandbox.io/s/react-videojs-strictmode-kvkk4c

Results

Expected

One should still be able to use StrictMode.

Actual

No video DOM element when using StrictMode.

Additional Information

Please include any additional information necessary here. Including the following:

versions

goffioul avatar May 05 '22 08:05 goffioul

Would the restoreEl option in #7722 work in this case? It will insert a clone of the original element in the disposed player's place.

mister-ben avatar May 05 '22 13:05 mister-ben

I don't think so, at least not without additional changes. Even if dispose inserts a fresh new clone inside the DOM, the react ref would still be invalid. The problem really boils down to manipulating the DOM outside of react, for DOM nodes that were rendered by react.

goffioul avatar May 05 '22 14:05 goffioul

I don't know React well, so don't necessarily understand the implications in React of this approach, but having React render a placeholder element and inserting a video-js el into that seems to work. That way Video.js is only manipulating the element inserted into React's remdered element.

https://codesandbox.io/s/react-videojs-strictmode-forked-gkk5eh?file=/src/VideoJS.js

mister-ben avatar May 10 '22 19:05 mister-ben

It's interesting to note that react says this behavior only happens in dev mode and doesn't happen in production mode (which makes sense), though, yeah, it'll be good to figure out what the new best practice is, since ideally, we'd want to support strict-mode long term, even if we say it isn't supported in the short term.

gkatsev avatar May 10 '22 19:05 gkatsev

Hi all, I have just published version 3.0.0 of my react-hook-videojs package that should support React 18 Strict Mode. It required a fair bit of DOM manipulation to undo the DOM destruction caused by videojs dispose. Let me know if it works for you! https://github.com/jimmycallin/react-hook-videojs/

jimmycallin avatar Jul 20 '22 13:07 jimmycallin

Not being able to use the latest React is a major issue for video.js, I'm surprised this is still open after 3 months.

thijstriemstra avatar Jul 30 '22 13:07 thijstriemstra

Any feedback on the suggestion above could have helped this along. If React's changes are complicating things enough that a package like @jimmycallin's serves better than a quick example, that could become the recommendation.

mister-ben avatar Aug 01 '22 20:08 mister-ben

I'm sorry if I sounded harsh; one of these days.

thijstriemstra avatar Aug 01 '22 22:08 thijstriemstra

I came to a similar solution as you, @mister-ben with a slight variation:

In the callback for unmount, I added a check to see if the player is disposed already in case it is destroyed elsewhere. This might be unnecessary to include in the updated react workflow example.

	useEffect(() => {
		const player = playerRef.current
		return () => {
			if (!player || player.isDisposed()) return
			player.dispose()
			playerRef.current = null
		}
	}, [playerRef])

lachieh avatar Aug 02 '22 13:08 lachieh

I can confirm that the solution by @mister-ben is solving the issue. It took me 3h to figure out why Video.js is not working in my Next.js project which uses React 18 Strict Mode.

skreutzberger avatar Sep 07 '22 14:09 skreutzberger

On Remix side this approach is not working 😅 so we have to switch to react@v17

danestves avatar Nov 01 '22 23:11 danestves

I don't know React well, so don't necessarily understand the implications in React of this approach, but having React render a placeholder element and inserting a video-js el into that seems to work. That way Video.js is only manipulating the element inserted into React's remdered element.

https://codesandbox.io/s/react-videojs-strictmode-forked-gkk5eh?file=/src/VideoJS.js

Thank you. I think this solution should be more prominent somewhere

JuanIrache avatar Nov 16 '22 10:11 JuanIrache

Realised this pull request on the docs wasn't merged. It is now, https://videojs.com/guides/react/ is updated now.

Any further doc PRs from those using React are of course always welcome.

mister-ben avatar Nov 16 '22 11:11 mister-ben