cloudinary-community / next-cloudinary

⚡️ High-performance image delivery and uploading at scale in Next.js powered by Cloudinary.
https://next.cloudinary.dev
MIT License
252 stars 74 forks source link

CldUploadWidget - onSucces overwrites previous state cant preview two images #491

Closed alessiotortora closed 1 month ago

alessiotortora commented 2 months ago

i was following a tutorial and they were using onUpload but that is deprecated. Now onSucces should do the same as onUpload but it overwrites the previous state.

here is an example: ImageUpload in the parent should change the field and pass the value so i can be previewed. Works fine with one Image but with two images only the last uploaded gets previewed.

<ImageUpload value={field.value.map((image) => image.url)} disabled={loading} onChange={(url) => field.onChange([...field.value, { url }])} onRemove={(url) => field.onChange([...field.value.filter((current) => current.url !== url)])} />

here is the widget:

'use client'

import { useEffect, useState } from 'react' import { Button } from './button' import { ImagePlus, Trash } from 'lucide-react' import Image from 'next/image'

import { CldUploadWidget, CloudinaryUploadWidgetResults } from 'next-cloudinary'

interface ImageUploadProps { disabled?: boolean onChange: (value: string) => void onRemove: (value: string) => void value: string[] }

function ImageUpload({ disabled, onChange, onRemove, value }: ImageUploadProps) { const [isMounted, setIsMounted] = useState(false)

useEffect(() => { setIsMounted(true) }, [])

console.log('value:', value)

const onUpload = (result: CloudinaryUploadWidgetResults) => { const info = result.info as object

console.log('start')
console.log(value)

if ('secure_url' in info) {
  const url = info.secure_url as string
  console.log('Uploaded URL:', url)
  onChange(url)
}

}

if (!isMounted) return null

return (

{value.map((url) => (
image
))}
{({ open }) => { const onClick = () => { open() } return ( ) }}

) }

export default ImageUpload

colbyfayock commented 2 months ago

hey @alessiotortora we deprecated onUpload to more closely follow the Upload Widget's event methods so that we could lean on that documentation (and expectations), as well as the event name being more explicitly clear about when it would get triggered: https://cloudinary.com/documentation/upload_widget_reference#events

as far as what you're seeing, i may be wrong but i think that behavior would be the same. the onSuccess callback would get fired for each successfully uploaded asset as opposed to at the end of all assets being uploaded. i can make that more clear in the documentation as i believe i haven't done that yet

so in your example, i can't see where you're storing the images, but im assuming that's based on:

onChange={(url) => field.onChange([...field.value, { url }])}

where you're pushing the new upload into an array stored in state, is that correct? that seems like it should work as expected

i created a basic example of pushing it into state and it seems to work as expected

https://codesandbox.io/p/devbox/strange-yalow-t62ftf?file=%2Fapp%2Fpage.tsx%3A7%2C26

will likely delete that sandbox at some point since its using my credentials to allow you to test the uploading there

doanletuanthanh commented 2 months ago

hey @alessiotortora we deprecated onUpload to more closely follow the Upload Widget's event methods so that we could lean on that documentation (and expectations), as well as the event name being more explicitly clear about when it would get triggered: https://cloudinary.com/documentation/upload_widget_reference#events

as far as what you're seeing, i may be wrong but i think that behavior would be the same. the onSuccess callback would get fired for each successfully uploaded asset as opposed to at the end of all assets being uploaded. i can make that more clear in the documentation as i believe i haven't done that yet

so in your example, i can't see where you're storing the images, but im assuming that's based on:

onChange={(url) => field.onChange([...field.value, { url }])}

where you're pushing the new upload into an array stored in state, is that correct? that seems like it should work as expected

i created a basic example of pushing it into state and it seems to work as expected

https://codesandbox.io/p/devbox/strange-yalow-t62ftf?file=%2Fapp%2Fpage.tsx%3A7%2C26

will likely delete that sandbox at some point since its using my credentials to allow you to test the uploading there

`"use client"; import { AlertModal } from "@/components/modals/alert-modal";

import { Button } from "@/components/ui/button"; import { Form, FormControl, FormField, FormItem, FormLabel, FormMessage, } from "@/components/ui/form"; import Heading from "@/components/ui/heading"; import ImageUpload from "@/components/ui/image-upload"; import { Input } from "@/components/ui/input"; import { Separator } from "@/components/ui/separator"; import { useOrigin } from "@/hooks/use-origin"; import { zodResolver } from "@hookform/resolvers/zod"; import { Product, Image } from "@prisma/client"; import axios from "axios"; import { Trash } from "lucide-react"; import { useParams, useRouter } from "next/navigation"; import React, { useState } from "react"; import { useForm } from "react-hook-form"; import toast from "react-hot-toast"; import { set, z } from "zod";

const formSchema = z.object({ name: z.string().min(1), images: z.object({ url: z.string() }).array(), price: z.coerce.number().min(1), categoryId: z.string().min(1), colorId: z.string().min(1), sizeId: z.string().min(1), isFeatured: z.boolean().default(false).optional(), isArchived: z.boolean().default(false).optional(), }); type ProductFormValues = z.infer; interface ProductFormProps { initialData: (Product & { images: Image[] }) | null; }

export const ProductForm: React.FC = ({ initialData }) => { const params = useParams(); const router = useRouter(); const [open, setOpen] = useState(false); const [loading, setLoading] = useState(false);

const title = initialData ? "Edit product" : "Create product"; const description = initialData ? "Edit a product" : "Add a new product"; const toastMessage = initialData ? "Product updated." : "Product created."; const action = initialData ? "Save changes" : "Create"; const form = useForm({ resolver: zodResolver(formSchema), defaultValues: initialData ? { ...initialData, price: parseFloat(String(initialData?.price)) } : { name: "", images: [], price: 0, categoryId: "", colorId: "", sizeId: "", isFeatured: false, isArchived: false, }, }); const onSubmit = async (values: ProductFormValues) => { console.log(values);

};

return ( <> <AlertModal isOpen={open} onClose={() => setOpen(false)} onConfirm={onDelete} loading={loading} />

{initialData && ( )}
  <Separator />
  <Form {...form}>
    <form
      onSubmit={form.handleSubmit(onSubmit)}
      className="space-y-8 w-full"
    >
      <FormField
        control={form.control}
        name="images"
        render={({ field }) => (
          <FormItem>
            <FormLabel>Images</FormLabel>
            <FormControl>
              <ImageUpload
                value={field.value.map((image) => image.url)}
                disabled={loading}
                onChange={(url) => {
                  field.onChange((field.value = [...field.value, { url }]));
                }}
                onRemove={(url) =>
                  field.onChange([
                    ...field.value.filter((current) => current.url !== url),
                  ])
                }
              />
            </FormControl>
            <FormMessage />
          </FormItem>
        )}
      />

      <Button disabled={loading} className="ml-auto" type="submit">
        {action}
      </Button>
    </form>
  </Form>
</>

); }; this is the form using that ImageUpload. There are some issue with the onSuccess that the onChange instead append the url to the images array, it replace the old one. I tried to use the onUpload (Deprecated), it can update proper so that all the images is uploaded and stored in the images array. there a way i read that you can modify the onChange to onChange={(url) => { field.onChange((field.value = [...field.value, { url }]));}} but it is not a proper way

alessiotortora commented 2 months ago

@doanletuanthanh this works: onChange={(url) => { field.onChange((field.value = [...field.value, { url }]));}}

Can you explain me how you find this ? and why my code was not working ?

alessiotortora commented 2 months ago

@colbyfayock hey thanks for the code, your approach is working but if you put it in a form it does not work. the line that @doanletuanthanh sends works.

So like he said: onSuccess gives a problem with appending the url to the state. I dont know if this problem has to do with next-cloudinary or the form onChange.

Maybe you can let me know so we can fix this ?

colbyfayock commented 2 months ago

the difference in how the callback is invoked is onUpload is invoked on results state change after that state was updated due to a successful callback:

https://github.com/cloudinary-community/next-cloudinary/blob/main/next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.tsx#L90-L101

where onSuccess is invoked immediately at the time of the event

https://github.com/cloudinary-community/next-cloudinary/blob/main/next-cloudinary/src/components/CldUploadWidget/CldUploadWidget.tsx#L261-L267

im not sure yet how that actually impacts your usage, but that would potentially impact how it's called. perhaps it's due to the state change potentially impacting the render lifecycle somewhere

aside from the naming, the CldUploadWidget state updates as a callback seem to be less reliable if i remember correctly due to the fact that there could be many state updates in a short period of time, where invoking the callback based on the event itself should always be reliable

i'm not familiar enough with the form component's API to know how onChange technically works (those look like shadcn?) but this bit:

field.onChange((field.value = [...field.value, { url }]));

seems to be directly mutating the value which i suspect isn't "safe" but im not really sure

do you have access somewhere within the form component that you can log out the value of field.value in such that you can see it change over time? then add console.logs in the onSuccess and onUpload callbacks, where you may be able to see where and why things are getting out of sync? and could even validate that both of those callbacks are working as expected which may point to it being something to do with the forms and not the CldUploadWidget component

hard to tell without a working example that i could debug through, if you have time to throw it reproducible into a Stackblitz/Codesandbox i can poke around in it

colbyfayock commented 1 month ago

im going to close this issue for now unless we can point to there being an issue here, but from this case, it seems like the issue may be in conjunction with the other library. let me know if im missing something and happy to repopen