cloudinary-community / netlify-plugin-cloudinary

Supercharge images on your Netlify site with Cloudinary!
https://netlify.cloudinary.dev/
MIT License
42 stars 18 forks source link

:recycle: Refactor Cloudinary upload logic to add a upload retry #88

Closed kcoderhtml closed 1 year ago

kcoderhtml commented 1 year ago

Hopefully this works :)

Fixes #82

netlify[bot] commented 1 year ago

Deploy Preview for netlify-plugin-cloudinary ready!

Name Link
Latest commit 894b2dc005c1da6da8cf425d14178276c58f3583
Latest deploy log https://app.netlify.com/sites/netlify-plugin-cloudinary/deploys/654e893a26f4ca00082c55f8
Deploy Preview https://deploy-preview-88--netlify-plugin-cloudinary.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

colbyfayock commented 1 year ago

hey @kcoderhtml clever idea of using a loop like that, hadnt considered that

i think we're running into an issue though, where the retry logic doesn't seem to be running for me

to test, i replaced fullPath in both upload lines to something random and invalid like /

i think what's happening is that when the first error occurs, the catch re-throws the error, resulting in getCloudinaryUrl throwing the error and never hitting the second attempt, though i certainly may be wrong

i wasn't able to get console.log('attempt', attempt) logged out more than once by placing it at the of the loop

i'm wondering if the loop needs to be inside of the try catch, or perhaps inside of the catch, you first check the attempt number and seeing if it's at the maximum number of attempts before throwing (which we could possibly set up for configuration later)

any chance you were able to get this running locally? if not and you're willing to try, happy to help you get set up if the instructions on the README didnt work, that way you can test it out

kcoderhtml commented 1 year ago

I would be happy to help but could not get the demo site to run, I kept getting this error: https://app.warp.dev/block/AK6xV3BiPb68w5XM3cyhh7.

colbyfayock commented 1 year ago

oh weird - is that after building the project? since it needs to compile the TS, otherwise the files wont exist in dist which is the output folder

kcoderhtml commented 1 year ago

I just tried building, but I got this error: https://app.warp.dev/block/ZpMZMoKUYbvYa2Zv1Qy24Y. I also tried with npm run build but it wanted me to install the node modules, but they wouldn't install: https://app.warp.dev/block/UV6d9X6VzdjyV49kvpYiSi. I'm sure I'm missing something, so thank you for your help.

colbyfayock commented 1 year ago

how about try the following

in the root:

pnpm install
pnpm build

then i usually test with this command:

netlify deploy --build --filter docs --context production

i run it directly instead of trying to use the package.json script

production might not be required but it helps point to images that are on the production site, and sometimes building locally can be problematic

im not sure if you set it up yet but it requires the Netlify CLI and logged in to an account

if you dont have any luck, i wonder if it's reproducible by adding a test for getCloudinaryUrl with it

kcoderhtml commented 1 year ago

The pnpm build fails with You must supply a cloudName when initializing the asset.

colbyfayock commented 1 year ago

yeah, you need to configure the cloud name in order for it to work as it ultimately uploads and uses Cloudinary, which requires an account

to upload though you'll need to add the api key and secret as well

would you rather just create a test for this?

otherwise, once you connect your netlify site through the CLI, you can add your environment variables in the Netlify dashboard, those should be dynamically pulled in when trying to deploy locally

colbyfayock commented 1 year ago

@kcoderhtml were you going to continue on this? if this is wrapped up by mid-November we can still consider this Hackoberfest given it was opened within October

kcoderhtml commented 1 year ago

Sorry, I've been quite busy these last few weeks. I am still working on this and will be writing a test for the code piece.

colbyfayock commented 1 year ago

awesome sounds great! thank you, let me konw if you need a hand

kcoderhtml commented 1 year ago

thanks! will do.

colbyfayock commented 1 year ago

hey @kcoderhtml when you get back to this, please merge in main as it has some updates in the main index.ts related to concurrency. hoping it doesnt conflict with your work

PR: https://github.com/cloudinary-community/netlify-plugin-cloudinary/pull/89

kcoderhtml commented 1 year ago

I think that might have done it! There is a new timeout and maxRetries settings to adjust to your liking.

colbyfayock commented 1 year ago

awesome! the retries worked great

im seeing one issue though that makes me think there's a loose async request somewhere, but i can't tell if its your code or the wraping code

image

if you notice, the 3rd attempts are happening in a later build stage which i think could potentially cause issues if there's a lot of uploads

im goign to play around with it a little more likely next week to try to determine the cause of that, but this code genrally is looking great

colbyfayock commented 1 year ago

yeah its definitely related to this code somehow. i put a before/after console log around the async function and without the code, it all wraps up before continuing

image

but with the code, it never ends

image

everything looks sounds to me though, i wonder if something is happening

the way im testing this btw is replacing fullPath with garbage ./asdf and it fails automatically.

colbyfayock commented 1 year ago

i think i figured it out and i dont think its actually related to this - ithink the error that's being thrown from within getCloudinaryUrl is not being properly caught anymore from within the wrapping code and thats causing the issue. im gonna separately get a bug fix in

colbyfayock commented 1 year ago

apologies for the confusion there but we're good to go!

thanks @kcoderhtml great add here

colbyfayock commented 1 year ago

@all-contributors please add @kcoderhtml for code

allcontributors[bot] commented 1 year ago

@colbyfayock

I've put up a pull request to add @kcoderhtml! :tada:

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 1.14.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

kcoderhtml commented 1 year ago

Thank you! Will this be included in cloudinary hacktoberfest contributions? The swag pack looks pretty sweet!

colbyfayock commented 1 year ago

@kcoderhtml congrats on the merged PR! since you opened this in October, you've qualified for Hacktoberfest swag! please email hacktoberfest@cloudinary.com with your github username and a link to your contribution where I'll follow up to request more information for getting you your swag

https://cloudinary.com/blog/hacktoberfest-celebrate-open-source-sdks

kcoderhtml commented 1 year ago

Thank you!