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

Sample code does not work when interacting with component states [video.js with react] #7361

Closed
jomarquez21 opened this issue Aug 3, 2021 · 12 comments

Comments

@jomarquez21
Copy link
Contributor

jomarquez21 commented Aug 3, 2021

Description

When trying to update a state in any event related to the player, the video disappears. https://codesandbox.io/s/videojs-react-hooks-example-forked-17bx0?file=/src/VideoJsPlayer.js

Steps to reproduce

Explain in detail the exact steps necessary to reproduce the issue.

  1. Instantiate the videojs, ex: player = videjs(reference, options);
  2. add a listener, ex: player.on('play', onPlayerListener);
  3. In the listener trigger any state update.

Results

Expected

I expected to be able to interact with the component through the events of the player.

Actual

When the play button is clicked the player disappears.

Error output

No error is displayed on the screen

Additional Information

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

versions

videojs

7.14.3

browsers

Any

OSes

Any

plugins

n/a

react

17.0.2

react dom

17.0.2

@welcome
Copy link

welcome bot commented Aug 3, 2021

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

@gkatsev
Copy link
Member

gkatsev commented Aug 3, 2021

It's not the event that's the issue. It's whatever setSaludo is. Unfortunately, I'm not that familiar with react's useEffect to help out further. But commenting out line 23 makes it continue working.

@qiuyaya
Copy link

qiuyaya commented Aug 4, 2021

I separated the configuration
image

@jomarquez21
Copy link
Contributor Author

jomarquez21 commented Aug 4, 2021

@decision4fr could you add a snippet? I can't see you interact with the player events.

What does the getTargetElement () function do and where does it come from?

@jomarquez21
Copy link
Contributor Author

@gkatsev I understand that the error comes from line 23, and I don't think the useEffect is the problem, for me the error is more related to the reference, but this error happens regardless of whether the component is functional or class.

@qiuyaya
Copy link

qiuyaya commented Aug 5, 2021

@decision4fr could you add a snippet? I can't see you interact with the player events.

What does the getTargetElement () function do and where does it come from?

getTargetElement () is equivalent to videoRef.current

@FredZeng
Copy link
Contributor

FredZeng commented Aug 5, 2021

You should notice that <VideoJsPlayer /> is a Function Component. Rendering a FC is equal to executing a function, which means you will generate a new <VideoHtml /> FC once <VideoJsPlayer /> rendered. And since VideoHtml is a new function, the React will perform diff operation on <VideoHtml />.

To fix this problem, you can try these:

  1. add key on <VideoHtml />, to tell React there is no need to perform diff operation on <VideoHtml />
return (
    <div>
        <h3>{saludo}</h3>
        <VideoHtml key="video" />
    </div>
)
  1. wrap VideoHtml with React.useCallback, to ensure the <VideoHtml /> FC is always the same
const VideoHtml = React.useCallback((props) => (
    <div data-vjs-player>
      <video ref={videoRef} muted className="video-js vjs-big-play-centered" />
    </div>
  ), []);
  1. declare the <VideoHtml /> outside <VideoJsPlayer />[Recommended]
import React from "react";
import videojs from "video.js";

const VideoHtml = React.forwardRef((props, videoRef) => (
  <div data-vjs-player>
    <video ref={videoRef} muted className="video-js vjs-big-play-centered" />
  </div>
));

const VideoJsPlayer = (props) => {
  const videoRef = React.useRef(null);
  const [saludo, setSaludo] = React.useState("hola");

  React.useEffect(() => {
    const videoElement = videoRef.current;
    let player;
    if (videoElement) {
      player = videojs(videoElement, props, () => {
        console.log("player is ready 123123");
      });
      player.on("play", () => {
        console.log("desde play");
        setSaludo("Hola mundo");
      });
    }
    return () => {
      if (player) {
        player.dispose();
      }
    };
  }, [props]);
  return (
    <div>
      <h3>{saludo}</h3>
      <VideoHtml ref={videoRef} />
    </div>
  );
};

export default VideoJsPlayer;

@jomarquez21
Copy link
Contributor Author

@FredZeng Thanks for your answer, I had already tried to use React.forwardRef before but I found another error, When the values received by the VideoJsPlayer component are updated the player disappears after calling the dispose() method.

I add another snippet with the mentioned error. https://codesandbox.io/s/blazing-worker-jmtyy?file=/src/VideoPage.js

@FredZeng
Copy link
Contributor

FredZeng commented Aug 6, 2021

@jomarquez21 Well, I get your point now. If you dive into video.js Component.dipose(), you can find some code like this:
https://github.com/videojs/video.js/blob/main/src/js/component.js#L174-L181

dispose() {
  // ...

  // **What makes errors happened**
  if (this.el_) {
    // Remove element from DOM
    if (this.el_.parentNode) {
      this.el_.parentNode.removeChild(this.el_);
    }

    this.el_ = null;
  }

  // remove reference to the player after disposing of the element
  this.player_ = null;
}

The VjsPlayer inherits from Component, when calling player.dispose(), it will also call component.dispose(), which will remove your <div data-vjs-player>(generated by React) from the document. However, React dosen't aware <div data-vjs-player> has been removed, so in the useEffect, videoRef.current actually not exist in the document.

To fix this, you can try, or make sure <div data-vjs-player> exist before creating a new player:

import React from "react";
import videojs from "video.js";
import VideoHtml from "./VideoHtml";

const VideoJsPlayer = (props) => {
  const videoRef = React.useRef(null);
  const playerRef = React.useRef(null);
  const [saludo, setSaludo] = React.useState("hola");

  React.useEffect(() => {
    const videoElement = videoRef.current;
    if (videoElement) {
      if (!playerRef.current) {
        playerRef.current = videojs(videoElement, props, () => {
          console.log("player is ready 123123");
        });
        playerRef.current.on("play", () => {
          console.log("desde play");
          setSaludo("Hola mundo");
        });
      } else {
        // only update necessary props
        playerRef.current.width(props.width);
        playerRef.current.height(props.height);
        playerRef.current.autoplay(props.autoplay);
        playerRef.current.controls(props.controls);
        playerRef.current.src(props.sources);
      }
    }
  }, [props]);

  React.useEffect(() => {
    return () => {
      if (playerRef.current) {
        playerRef.current.dispose();
        playerRef.current = null;
      }
    };
  }, []);

  return (
    <div>
      <h3>{saludo}</h3>
      <VideoHtml ref={videoRef} />
    </div>
  );
};

export default VideoJsPlayer;

@jomarquez21
Copy link
Contributor Author

jomarquez21 commented Aug 9, 2021

Thank you very much @FredZeng for the answers now I understand more about the image of @gkatsev, you also helped me to understand a little more the use of references within the effect.

Do you think it is necessary to update the react? tutorial in the documentation?

@gkatsev
Copy link
Member

gkatsev commented Aug 9, 2021

Any help to improving the docs would be greatly appreciated!

@jomarquez21
Copy link
Contributor Author

jomarquez21 commented Sep 20, 2021

I think this issue can already be closed.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2022
edirub pushed a commit to edirub/video.js that referenced this issue Jun 8, 2023
Improve documentation on functional components to clean up reference if the component is unmounted.

Fixes videojs#7361
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants