[Bug] playerRef attribute is not a valid Ref #575

[SK1965]

Description

This PR fixes a bug where the playerRef attribute in CldVideoPlayer was not a valid React Ref. Previously, playerRef could not be reliably accessed inside useEffect or parent components, causing playerRef.current to always be null.

This update:

Refactors CldVideoPlayer to use forwardRef.

Uses useImperativeHandle to expose the Cloudinary player instance via the parent’s ref.

Ensures event listeners can be attached reliably.

Confirms the player object is accessible and events like percentsplayed work as expected.

This addresses issue #575.

Issue Ticket Number

Fixes #575

Type of change

Bug fix (non-breaking change which fixes an issue)

Checklist

I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md

I have created an issue ticket for this PR

I have checked to ensure there aren't other open Pull Requests for the same update/change

I have performed a self-review of my own code

I have run tests locally to ensure they all pass

I have commented my code, particularly in hard-to-understand areas

I have made corresponding changes needed to the documentation

[vercel[bot]]

@SK1965 is attempting to deploy a commit to the Cloudinary DevX Team on Vercel.

A member of the Team first needs to authorize it.

[eportis-cloudinary]

Hi @SK1965!

Thank you so much for this PR and for the clearly-commented code. I am asking for some help internally to help review this but I wanted to first ask you about the use of forwardRef here. IIUC, it is no longer necessary in React 19, and will soon be deprecated. I assume it is necessary for React < 19, though, and is therefore probably the best way to maintain backwards compatibility?

[SK1965]

Hi! @eportis-cloudinary

Thanks for taking a look at this PR and for the great question.

Your assumption is exactly right. The decision to use forwardRef here is purely for backward compatibility.

The library's peerDependencies support React 17 and 18, and for those versions, forwardRef is the only way to correctly pass a ref to a function component. Using the new React 19 method would break the component for anyone on those older, but still supported, versions.

Since forwardRef still works perfectly in React 19, this approach ensures the fix works for all users across the library's supported range.

[eportis-cloudinary]

Hi @SK1965,

First of all I'd like to apologize for the extended delay in feedback.

After a thurough review, I think that because of how this PR affects refs and triggers additional renders, it will need to be considered a breaking change.

I'm also not entirely clear on the cases when you would need a ref for the player before it is instantiated in the DOM. If you could, could you provide an example in response to my question on the issue? https://github.com/cloudinary-community/next-cloudinary/issues/575#issuecomment-3383539754

If there are compelling cases, I am happy to merge this clear, well-authored change. But I will have to ask you to re-submit it against the beta branch, because it is breaking.

In the meantime, because I was not able to give you this feedback earlier, my colleage @devpatocld will reach out about Hacktoberfest swag, as if this were an accepted change.

To review

1. This does solve the issue, but is a breaking change
2. Please re-submit the PR against the beta branch
3. Please provide any use cases for when a ref is needed before the element exists to the discussion in issue #575 .

Thank you!

[devpatocld]

@SK1965 Can you please provide us with your email address? I need to send you an email with the insturctions to claim your swag

[SK1965]

@devpatocld email address : [email protected]

[devpatocld]

@SK1965 we have sent you an email

[SK1965]

Hi @eportis-cloudinary, sincere apologies for the delay in getting this back to you! I have re-submitted this PR against the beta branch as requested.

Here is the concrete use case you requested, along with a technical explanation of the race condition this PR resolves.

The Concrete Use Case (The Failure)

The issue arises when someone tries to interact with the player (e.g., adding an event listener) immediately after the component mounts.

Currently (Without this PR):

Because the player initializes asynchronously (waiting for the external script), ref.current is null inside a standard useEffect. The developer must implement a "polling" workaround to wait for the object to exist.

// ❌ FAILS / REQUIRES WORKAROUND
useEffect(() => {
  // We are forced to write a dirty interval loop to "catch" the player
  const interval = setInterval(() => {
    if (playerRef.current) {
      playerRef.current.on('play', () => console.log('Playing!'));
      clearInterval(interval);
    }
  }, 500);

  return () => clearInterval(interval);
}, []);

✅ The Fix (With this PR)

By using forwardRef and useImperativeHandle (triggered by an internal useState update), we tie the player instance directly to the React lifecycle. The ref is correctly managed, allowing standard React code to work without hacks.

// ✅ PASSES / STANDARD REACT
// The ref works as expected immediately without polling

useEffect(() => {
  if (playerRef.current) {
    playerRef.current.on('play', () => console.log('Playing!'));
  }
}, []);

Technical Deep Dive: The Race Condition

For clarity, here is exactly why the race condition occurs in the current version and how this PR resolves it:

The "Race" (Current Behavior)

• Render/Mount (t=0ms): The parent component mounts. The useEffect runs immediately.
• Check: The effect checks playerRef.current. Since the Cloudinary script is still downloading in the background, the player does not exist yet. The value is null.
• Async Load (t=200ms): The script finishes loading and populates the ref.
• Result: It is too late. The useEffect has already finished execution and "missed" the player.

The Fix

This PR introduces useState to track the player instance internally.

1. When the asynchronous script loads, we call setPlayer.
2. This triggers a React Re-Render.
3. During this render phase, useImperativeHandle executes and correctly populates the forwarded ref, ensuring it is available to the parent context in a reliable way.

Note on React 19

React 19 actually automates this behavior by treating ref as a standard prop (making forwardRef obsolete). By implementing this change now, we provide React 17/18 users with the same reliable, clean API that React 19 users get natively.

Thanks again for the review!