cloudinary-community / cloudinary-examples

Easy Cloudinary integration examples for boosting web performance.
MIT License
88 stars 79 forks source link

Cleaning up the React Upload Widget instance on unload #64

Closed koren3210 closed 1 year ago

koren3210 commented 1 year ago

hey :)

i use the react upload widget in my app. i found that if i navigate from the upload page(in my app it /addnew) to the home page / and the back the /addnew the iframe is still exist as expected . but when i try to updateUrl state something related to the iframe is going wrong and the url state doesn't update.

i have a temp fix for that. a clean function that destroy the widget when the location of the page is changing.

Screenshot 2023-10-04 at 0 49 37
colbyfayock commented 1 year ago

hey @koren3210 thanks for sharing - what router are you using? i would suspect that part of the solution would be router dependent right? does the unlisten functionality properly work well for your use case? it likely makes sense that we should be cleaning up the instance in the examples as well

if you feel this is something we should add to our examples, would you want to create a pull request for all of the react-centric upload widget examples to include that?

There are also the Next.js examples, however those work slightly different since they take advantageof Next.js APIs for the loading aspect:

JoshuaRotimi commented 1 year ago

Hello @colbyfayock I'd like to work on this issue. If @koren3210 is not already on it. But before that, I would like to test out the issue and see if I can replicate it. Is there an npm package I can install to an existing project to test the issue?

colbyfayock commented 1 year ago

hey @JoshuaRotimi yup they havent answered in over a week so its all yours!

there's no npm package, this repository is a bunch of independent projects within the examples directory

the instructions should generally be in each folder's README but if you go clone this repo and navigate to that example, run npm install, and im thinking it's Create React App so run npm run start, you should be able to get it up and running

you'll likely need to add your cloudinary credentials as environment variables as well

JoshuaRotimi commented 1 year ago

Oh alright. I'll get on it then.

colbyfayock commented 1 year ago

awesome thank you!

JoshuaRotimi commented 1 year ago

Quick question @colbyfayock , Is there a reason the widget variable is being stored outside the UploadWidget component? I think it causes an issue.

The Issue - If you have two instances of the UploadWidget component on the page, with two different handleUpload functions that change a piece of state when the upload is successful, only the first upload function will be used every time giving a strange behaviour. Might be a reason why @koren3210 had that issue.

This should explain it better - https://www.loom.com/share/16351971c85c460ea85f85ad7c1887ec

koren3210 commented 1 year ago

Hey @JoshuaRotimi , I opened issue in the past. I tried to tag you in the closed issue so you can see why he placed the variables outside the function.

If the variables are inside the function so there was a problem of recreating the iframe instance when I navigate between pages in my app.

Tell me if you can see the tag in the closed issue

JoshuaRotimi commented 1 year ago

Yes I have seen the tag, thanks @koren3210. But maybe the solution you suggested with the cleanup function in the useEffect may solve that problem?

koren3210 commented 1 year ago

@JoshuaRotimi
I tried now with the variables inside the uploadWidget and it keep the issue of creating instances every time I navigate back to the page. Although the cleanup function

When the variables are lifted out of the function it works properly.

JoshuaRotimi commented 1 year ago

That's strange @koren3210. The cleanup function should remove the instance when unmounted and create a new one when you navigate back to the page when the widget variable is inside the uploadWidget component. Is the issue creating a new instance or creating multiple instances ?

colbyfayock commented 1 year ago

hey before i dig into this, any updates? thanks Koren for filling in on the history of the changes. it's been tough finding a perfect solution for how to manage the instance of the widget

JoshuaRotimi commented 1 year ago

From my tests, removing the widget with the cleanup function will solve the problem of creating new instances which (from my understanding) was the reason for putting the widget variable outside the uploadWidget component in the first place. With the widget variable inside the component, the problem I described earlier/in the video is resolved. Of course, I can make the PR and we can test it and see if all works well.

colbyfayock commented 1 year ago

yes! if you dont mind, let's get this in a PR and i'll spin that up locally as well. thanks @JoshuaRotimi

koren3210 commented 1 year ago
Screenshot 2023-10-11 at 22 53 24

i tried again with the instance variables inside the function. and it does work properly.