fix: widget isn't destroyed on unmount

[yawuxi]

Description

Since the component is mounted, unmounted, and then mounted again, the onLoad event on the <Script /> component from next/script fires multiple times. As a result, two iframes are created instead of one.

I noticed that you implemented a useEffect() to remove the old widget, but it does not work as intended. Below, I will explain why it fails and provide possible solution.

Ensure useEffect() Executes Properly. Currently, the code contains the following useEffect() for cleanup:

useEffect(() => {
  return () => {
    widget.current?.destroy();
    widget.current = undefined;        
  }
}, [])

This should clean up the previous widget, but the issue is that it does not track updates to the widget.current variable. How to Fix It?
Instead of using an empty dependency array [], update it to track widget.current:

useEffect(() => {
  return () => {
    widget.current?.destroy();
    widget.current = undefined;        
  }
}, [widget.current])

This ensures the cleanup runs correctly when widget.current changes, preventing multiple iframe creations.

Issue Ticket Number

Fixes #587
(#587)

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Fix or improve the documentation
• This change requires a documentation update

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 (I think it's unnecessary)

[vercel]

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

A member of the Team first needs to authorize it.

[yawuxi]

If you're not satisfied with the approach of using a ref in the dependency array, I have a few other possible solutions to the problem.

[yawuxi]

I spent some more time on this and realized that the cleanup actually works—it works if the unmount doesn't happen immediately. However, it doesn't work if the asynchronous operation inside useEffect() runs during the mount, which causes two instances to be created. Well, it seems that to make this work, I really need to remove this part of the code.

triggerOnIdle(() => {
  if ( !widget.current ) {
    widget.current = createWidget();
  }
});

I’d appreciate any feedback, thanks!

[yawuxi]

In the latest commit, I found a solution to keep the automatic initialization of the iframe while removing its duplicate. I suggest simply tracking the component's state using a ref. I look forward to your feedback. Thank you!

[yawuxi]

Hello! I am pinging the pull request since there was no response, and the error is critical for me as it causes my E2E tests to fail.

[eportis-cloudinary]

@yawuxi Thank you so much for both the PR, and the ping. Your description of the problem/solution makes sense but I have not made the time to do an actual review. I hope to early next week.

[yawuxi]

@eportis-cloudinary Thank you for your response, alright, I’ll wait for the feedback, thanks!

[eportis-cloudinary]

@yawuxi I finally (!) had some time to try to wrap my brain around this today. TL;DR I am still not sure what the best course of action is and am asking for a second opinion.

Adding the isMounted ref makes total sense, but I am trying to wrap my mind around whether or not there are other race conditions that can be triggered in between the time handleOnLoad is called and requestIdelCallback runs and we actually create the widget. If there are, the best course of action might be removing the triggerOnIdle wrapper.

Thank you once again for all of the time you put into understanding and clearly communicating about the problem, and exploring a range of potential remedies. And again sincere apologies for the long wait.

[yawuxi]

@eportis-cloudinary Hi! Yes, I believe removing triggerOnIdle is the best solution. I can include this change in the current pull request. What do you think?

[pawelphilipczyk-cloudinary]

Hey! I've looked at the problem and I propose a bit different approach to keep the original intent of triggerOnIdle but make sure it is both cleaned up correctly on unmount as well as on subsequent triggers.

Please see and test my branch.