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.

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

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

Closed
goffioul opened this issue May 5, 2022 · 13 comments
Closed

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

goffioul opened this issue May 5, 2022 · 13 comments
Labels
enhancement needs: discussion Needs a broader discussion

Comments

@goffioul
Copy link

goffioul commented May 5, 2022

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

  • videojs@7.18.1
  • react@18.1.0
@mister-ben
Copy link
Contributor

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.

@goffioul
Copy link
Author

goffioul commented May 5, 2022

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.

@mister-ben
Copy link
Contributor

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

@gkatsev
Copy link
Member

gkatsev commented May 10, 2022

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.

@jimmycallin
Copy link

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/

@thijstriemstra
Copy link
Contributor

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.

azimut added a commit to azimut/reddit-gallery that referenced this issue Aug 1, 2022
@mister-ben
Copy link
Contributor

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.

@thijstriemstra
Copy link
Contributor

thijstriemstra commented Aug 1, 2022

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

@lachieh
Copy link

lachieh commented Aug 2, 2022

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])

@skreutzberger
Copy link

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.

@danestves
Copy link

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

@JuanIrache
Copy link

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

@mister-ben
Copy link
Contributor

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement needs: discussion Needs a broader discussion
Projects
None yet
Development

No branches or pull requests

10 participants