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

add eventlisterner in useEffect in CustomPlyrInstance not working with empty dependency array #732

Closed
bradrice opened this issue Jan 25, 2022 · 4 comments

Comments

@bradrice
Copy link

If I leave off the dependency array, the eventlisteners get attached, but mulitple times due to rerenders of the AudioPlayer component I have created. I would like to prevent adding additional event listeners.

Here is my code in the useEffect

React.useEffect(() => {
      /**
       * Fool react for using forward ref as normal ref
       * NOTE: in a case you don't need the forward mechanism and handle everything via props
       * you can create the ref inside the component by yourself
       */
      const { current } = ref as React.MutableRefObject<APITypes>;
      if (current.plyr.source === null) return;

      const api = current as { plyr: PlyrInstance };
      api.plyr.on("ready", onReady);
      api.plyr.on("canplay", onCanPlay);
      api.plyr.on("ended", onEnded);
      eventBus.on("imagesLoaded", () => {
        console.log("images have loaded, time to play");
        // api.plyr.play();
      });

      return () => {
        api.plyr.off("ended", onEnded);
        api.plyr.off("ready", onReady);
        api.plyr.off("canplay", onCanPlay);
      };
    }, []);

Why can't I pass an empty array to minimize the calls to add the event listeners?

@realamirhe
Copy link
Collaborator

That's because in that cause you don't have access to the latest ref.

You can optimize your workflow by one of the following

  1. passing the ref related dependencies array
useEffect(yourEffect, [ref.current && ref.current.plyr.source])
  1. Set inner state triggering that plyr has been set with some value or not
const [hasPlyr, setHasPlyr] = useState(false)
useEffect(() => {
  if (current.plyr.source === null) setHasPlyr(true)
})

useEffect(() => {
     /* Your API bounding useEffect */
}, [hasPlyr]);

Note: These might not work on the first try and these are the idea to show how it might be possible.
In case you make a code sandbox for your example I might be able to suggest more practical snippets

@bradrice
Copy link
Author

bradrice commented Feb 3, 2022

Thanks for the suggestions. I've tried nearly everything and can't seem to get the plyr instance to play with event handlers. I think this is a weakness of plyr-react. In plain vanilla javascript the ability to call plyr.play() or other methods is so easy. Even the Angular version of plyr I managed to use on a project and it was pretty easy.

The original method you showed me works using api.plyr.on, but it calls it multiple times. I'm having trouble limiting it, because when a rerender happens, the audio starts over.

@realamirhe
Copy link
Collaborator

Thanks for your feedback, @bradrice. The only reason a code improves is through the feedback we get from day-to-day usage.
This is why we maid alpha release and add people to use it.

When you do it on js, you have control over the plyr instance you've created yourself, so it makes it easy to interact with.
In react world, it is somehow different cause it won't trigger updates in every scenario.
Although we didn't face any performance issues, in using react-plyr, I understand lack of documentation makes it hard to come up with an efficient update mechanism.

The usePlyr was a POC hook that we've added to the codebase and will be re-calculated every important action, on source or options change, It can be managed by custom dependency array.

The goal of plyr-react is to make it totally separated from the plyr package, this is why we don't re-export every method that plyr gives through hard bounded class methods and send you the latest instance of plyr.

I guess the other issue which makes it a little bit harder to follow is the Proxy we are using to eliminate future development null checks.

Here is a comparison of API calls without first layer proxy

function noProxyGetAPI(plyr) {
   return () => ({
     plyr,
   })
}

Now you should always check in your code that whether the plyr instance exist or not

const onPlayHandler = () => { ref.current.plyr?.play() }

If it is possible to make a code sandbox, we can help to eliminate attaching unwanted event listeners.
Also feel free to book a meeting for (Monday, Wednesday) if you think it can be helpful.

react plyr architecture + basic modules

@bradrice
Copy link
Author

bradrice commented Feb 9, 2022

thanks for the detailed explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants