jwplayer / jwplayer-react

ISC License
28 stars 13 forks source link

Event listeners are using a stale state in functional components #3

Closed ChristiaanScheermeijer closed 1 year ago

ChristiaanScheermeijer commented 2 years ago

Nice library, I've been waiting for this for a while! 🎉

I ran into a problem while trying to port this library in the OTT Webapp project:

When using the player component in a functional component with callback functions, the event listeners don't reflect the current state but a stale state. This is due to the event listeners being set once after initialization, but when dealing with functional components this callback function is updated when it uses a state variable that updates.

You can reproduce this with the following code:

const Player = () => {
  const [position, setPosition] = useState(0);

  return (
    <JWPlayer 
      library="https://path-to-my-jwplayer-library.js"
      playlist="https://cdn.jwplayer.com/v2/playlists/playlist_media_id"
      onPlay={() => console.log(`Play at ${position}s`)}
      onPause={() => console.log(`Pause at ${position}s`)}
      onTime={({ position }) => {
        setPosition(position);
        console.log(`Updating position to: ${position}`);
      }}
    />
  );
};

In OTT Webapp we're using an effect that binds and unbinds all event listeners when changing, but this has some side effects. The most annoying one is that not all event callbacks are called.

Since this is made using a class component, it might be easier to wrap all event listeners and call them by looking up the props.

  createEventListener = (prop) => {
    return event => typeof this.props[prop] === 'function' && this.props[prop](event);
  }

  createEventListeners() {
    Object.keys(this.props).forEach((prop) => {
      const matchedOnce = prop.match('(?<=^once).*') || [''];
      const onceHandlerName = matchedOnce[0].charAt(0).toLowerCase() + matchedOnce[0].slice(1);

      if (onceHandlerName) {
        this.player.once(onceHandlerName, this.createEventListener(prop));
        return;
      }

      const matchedOn = prop.match('(?<=^on).*') || [''];
      const eventHandlerName = matchedOn[0].charAt(0).toLowerCase() + matchedOn[0].slice(1);

      if (eventHandlerName) {
        this.player.on(eventHandlerName, this.createEventListener(prop));
      }
    });
  }
m4olivei commented 2 years ago

I ran into this issue as well. I believe that they are trying to solve a more general case in https://github.com/jwplayer/jwplayer-react/pull/5

If I'm following, this particular case of a changing event handler closure would be handled, as well as other use cases like adding / removing handlers via props.

m4olivei commented 2 years ago

In OTT Webapp we're using an effect that binds and unbinds all event listeners when changing, but this has some side effects. The most annoying one is that not all event callbacks are called.

I found a similar workaround, here's what my effect looks like in case it's helpful for others:

useEffect(() => {
  const jwPlayer = jwPlayerRef.current;
  if (!jwPlayer) {
    return;
  }
  jwPlayer.off('playlistItem');
  jwPlayer.on('playlistItem', handlePlaylistItem);
}, [guid, handlePlaylistItem]);

handlePlaylistItem uses state in it's closure (guid), so the event handler needs to be refreshed whenever it changes, otherwise we get the same stale state problem as noted.

That's going to get annoying as I anticipate needing many more event handlers when I get around to ads and analyitcs integration.

ChristiaanScheermeijer commented 2 years ago

You can also use an event callback hook which prevents the callback from updating when a dependency changes.

Add this custom hook:

const useEventCallback = (callback) => {
  const fnRef = useRef(() => {
    throw new Error('Callback called in render');
  });

  useLayoutEffect(() => {
    fnRef.current = callback;
  }, [callback]);

  return useCallback((...args) => {
    if (typeof fnRef.current === 'function') {
      return fnRef.current(...args);
    }
  }, []);
};

Then wrap your callbacks with the useEventCallback hook:

const onPlaylistItemCallback = useEventCallback(handlePlaylistItem);

You can use this callback in the Jwplayer component:

return (
  <JWPlayer 
    library="https://path-to-my-jwplayer-library.js"
    playlist="https://cdn.jwplayer.com/v2/playlists/playlist_media_id"
    onPlaylistItem={onPlaylistItemCallback}
  />
);
m4olivei commented 2 years ago

Ahhh, thats nice, thanks @ChristiaanScheermeijer . That also does the trick. I admit to being new-ish to React, so that's really helpful. I gather that comes from the React docs FAQ, which I stumbled on Googling and trying to understand your hook:

https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback

mamaddox commented 2 years ago

@ChristiaanScheermeijer We just released a new version of the react component v1.1.0. This release allows for dynamic on events which sound like the issue you're facing. When you get a chance, could you see if this is still an issue in the new version?