fix failed render during page navigation #317

JoshuaRotimi · 2023-10-18T19:23:55Z

Description

This PR fixes the issue #279 where some players fail to load when rendering multiple CldVideoPlayer on one page.

Also fixes the issue where the CldVideoPlayer component fails to render properly after navigating between pages
Also adds a cleanup function that removes the widget instance when a CldUploadWidget component unmount

Issue Ticket Number

#279

Fixes #<ISSUE_NUMBER>

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

vercel · 2023-10-18T19:23:59Z

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

A member of the Team first needs to authorize it.

vercel · 2023-10-19T12:45:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
next-cloudinary	✅ Ready (Inspect)	Visit Preview	Nov 9, 2023 4:17pm

colbyfayock · 2023-10-19T12:51:01Z

next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.tsx

@@ -125,6 +125,15 @@ const CldUploadWidget = ({
    });
  }

+  useEffect(() => {
+    handleOnLoad();


@JoshuaRotimi we actually don't need this here - Next.js works a little bit differently than vanilla React in that it has the Script component, so this function gets invoked as soon as the script is ready that handles initialization

that means that this function could actually possibly create an error (might be happening on the other work you're doing) in that it could potentially try to initialize without the Script being ready

Oh alright. After trying multiple things, I found out that calling the handleOnLoad function in the useEffect fixes the rendering issue when you navigate between pages with the CldVideoPlayer component. I would further check to see if that is what causes the error on the examples repo that you mentioned.

i tried commenting out that line and it seemed to work without it, just with the <Script tag natively reloading between the pages - where are you seeing an issue?

Oh sorry. Not on this one. The comment should have been on the other component CldVideoPlayer component not the CldUploadWidget. You are correct. It should work without that on the CldUploadWidget.

got it okay - i would expect the same for the video player though, ill test locally too

colbyfayock · 2023-10-19T12:51:56Z

next-cloudinary/src/components/CldVideoPlayer/CldVideoPlayer.tsx

@@ -161,6 +161,16 @@ const CldVideoPlayer = (props: CldVideoPlayerProps) => {
    }
  }

+  useEffect(() => {
+    handleOnLoad();


similar thing going on here, this shouldn't happen here

next-cloudinary/src/components/CldVideoPlayer/CldVideoPlayer.tsx

colbyfayock · 2023-10-19T12:54:15Z

nice work @JoshuaRotimi it seems to clear the issues we were seeing!! just a few small tweaks needed

JoshuaRotimi · 2023-10-25T19:21:35Z

@colbyfayock I'm currently having some issues calling the dispose function when unmounting the component. I think the types of the CloudinaryVideoPlayer might need to be updated. It currently has only one type of on which is a function.

colbyfayock · 2023-10-30T20:36:29Z

@colbyfayock I'm currently having some issues calling the dispose function when unmounting the component. I think the types of the CloudinaryVideoPlayer might need to be updated. It currently has only one type of on which is a function.

can you clarify if it's a Type issue or if it's an issue with the instance not being available at the time of unmounting? just in case there's a race condition there

if it's a Type issue, let's at a @ts-ignore for now and i can open a ticket to get this addressed on the widget. let me know!

JoshuaRotimi · 2023-10-30T21:30:19Z

@colbyfayock I'm currently having some issues calling the dispose function when unmounting the component. I think the types of the CloudinaryVideoPlayer might need to be updated. It currently has only one type of on which is a function.

can you clarify if it's a Type issue or if it's an issue with the instance not being available at the time of unmounting? just in case there's a race condition there

if it's a Type issue, let's at a @ts-ignore for now and i can open a ticket to get this addressed on the widget. let me know!

Oh yes it is a type issue, @ts-ignore is a valid temporary fix. Also, are you still experiencing the issue of one or more components failing to load? Not sure if you saw my previous comment about using the id prop for each component.

colbyfayock · 2023-10-31T01:29:02Z

i need to test it again

i just found out however that there's another onReady callback for the Script tag, it may be worth considering playing around with that to see if its a bit more reliable

onLoad: Execute code after the script has finished loading.
onReady: Execute code after the script has finished loading and every time the component is mounted.

https://nextjs.org/docs/pages/building-your-application/optimizing/scripts#executing-additional-code

i would imagine that's actually what we would want, correct? it may not make the initial load more reliable, but it should make the page navigation more reliable and prevent us from having to include the additional initialization inside useEffect

h/t: https://twitter.com/stordahldotdev/status/1719098207651905820?s=61&t=ya091PlFoeyCWxWs2FVkIQ

JoshuaRotimi · 2023-10-31T10:55:31Z

Alright. You can pull the latest changes and test it again then let me know if it works. I will check out the onReady callback to see if it provides a better solution.

colbyfayock · 2023-10-31T12:59:09Z

It's better than it was before but it's still having issues loading completely reliably between page loads

https://d.pr/v/W1Oahw

Notice on the 3rd and 4th navigation as well as some others we're missing a video.

Let's see how onReady works here, it might resolve the issue

colbyfayock · 2023-11-06T15:58:58Z

@JoshuaRotimi any luck?

JoshuaRotimi · 2023-11-06T17:32:25Z

the onReady callback only runs once, so only the first component is loaded properly so I'm still unable to figure out the best solution so far. any ideas? @colbyfayock

colbyfayock · 2023-11-08T21:30:05Z

i see what you mean. i dont have any quick ideas ill need to spend more time thinking about this

i think there's enough value to move forward with this now though without waiting to help address some issues for people

can you remove the examples at the bottom of: docs/pages/cldvideoplayer/basic-usage.mdx

then i can merge it in and perhaps if you'er still interested in looking into this we can do it in another ticket, otherwise i can take over

thanks @JoshuaRotimi

JoshuaRotimi · 2023-11-09T09:21:21Z

Alright done.

colbyfayock · 2023-11-09T17:02:58Z

thanks @JoshuaRotimi - awesome job with this!!

merging as a minor release update since its a bit more than a simple fix

# [5.4.0](v5.3.0...v5.4.0) (2023-11-09) ### Features * dispose of upload widget and video player instance on unmount ([#317](#317)) ([a313b7a](a313b7a)), closes [#279](#279) [#279](#279)

github-actions · 2023-11-09T17:07:13Z

🎉 This PR is included in version 5.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [5.4.0](cloudinary-community/next-cloudinary@v5.3.0...v5.4.0) (2023-11-09) ### Features * dispose of upload widget and video player instance on unmount ([#317](cloudinary-community/next-cloudinary#317)) ([a313b7a](cloudinary-community/next-cloudinary@a313b7a)), closes [#279](cloudinary-community/next-cloudinary#279) [#279](cloudinary-community/next-cloudinary#279)

fix failed render during page navigation

7aedf8d

vercel bot deployed to Preview October 19, 2023 12:45 View deployment

colbyfayock reviewed Oct 19, 2023

View reviewed changes

next-cloudinary/src/components/CldVideoPlayer/CldVideoPlayer.tsx Show resolved Hide resolved

Merge branch 'main' into video-error

a3588dc

vercel bot deployed to Preview October 31, 2023 01:31 View deployment

JoshuaRotimi added 2 commits October 31, 2023 09:22

update cleanup

918cc05

update player version

2717f1c

vercel bot deployed to Preview October 31, 2023 12:55 View deployment

update basic usage page

b4eb91b

Merge branch 'main' into video-error

392eea0

vercel bot deployed to Preview November 9, 2023 16:17 View deployment

colbyfayock merged commit a313b7a into cloudinary-community:main Nov 9, 2023
4 checks passed

github-actions bot added the released label Nov 9, 2023

colbyfayock mentioned this pull request Nov 10, 2023

[Bug] CldUploadWidget Cannot read properties of undefined (reading 'destroy') #344

Closed

colbyfayock mentioned this pull request Jan 25, 2024

[Bug] @ts-ignore in CldVideoPlayer.tsx #426

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix failed render during page navigation #317

fix failed render during page navigation #317

JoshuaRotimi commented Oct 18, 2023 •

edited

Loading

vercel bot commented Oct 18, 2023

vercel bot commented Oct 19, 2023 •

edited

Loading

colbyfayock Oct 19, 2023

JoshuaRotimi Oct 19, 2023

colbyfayock Oct 19, 2023

JoshuaRotimi Oct 19, 2023

colbyfayock Oct 19, 2023

colbyfayock Oct 19, 2023

colbyfayock commented Oct 19, 2023

JoshuaRotimi commented Oct 25, 2023

colbyfayock commented Oct 30, 2023

JoshuaRotimi commented Oct 30, 2023

colbyfayock commented Oct 31, 2023

JoshuaRotimi commented Oct 31, 2023

colbyfayock commented Oct 31, 2023

colbyfayock commented Nov 6, 2023

JoshuaRotimi commented Nov 6, 2023

colbyfayock commented Nov 8, 2023

JoshuaRotimi commented Nov 9, 2023

colbyfayock commented Nov 9, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023

fix failed render during page navigation #317

fix failed render during page navigation #317

Conversation

JoshuaRotimi commented Oct 18, 2023 • edited Loading

Description

Issue Ticket Number

Type of change

Checklist

vercel bot commented Oct 18, 2023

vercel bot commented Oct 19, 2023 • edited Loading

colbyfayock Oct 19, 2023

Choose a reason for hiding this comment

JoshuaRotimi Oct 19, 2023

Choose a reason for hiding this comment

colbyfayock Oct 19, 2023

Choose a reason for hiding this comment

JoshuaRotimi Oct 19, 2023

Choose a reason for hiding this comment

colbyfayock Oct 19, 2023

Choose a reason for hiding this comment

colbyfayock Oct 19, 2023

Choose a reason for hiding this comment

colbyfayock commented Oct 19, 2023

JoshuaRotimi commented Oct 25, 2023

colbyfayock commented Oct 30, 2023

JoshuaRotimi commented Oct 30, 2023

colbyfayock commented Oct 31, 2023

JoshuaRotimi commented Oct 31, 2023

colbyfayock commented Oct 31, 2023

colbyfayock commented Nov 6, 2023

JoshuaRotimi commented Nov 6, 2023

colbyfayock commented Nov 8, 2023

JoshuaRotimi commented Nov 9, 2023

colbyfayock commented Nov 9, 2023 • edited Loading

github-actions bot commented Nov 9, 2023

JoshuaRotimi commented Oct 18, 2023 •

edited

Loading

vercel bot commented Oct 19, 2023 •

edited

Loading

colbyfayock commented Nov 9, 2023 •

edited

Loading