cloudinary-community / next-cloudinary

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

[Feature] Bundle `use client` with components #137

Open colbyfayock opened 1 year ago

colbyfayock commented 1 year ago

Feature Request

Is your feature request related to a problem? Please describe.

To fix an issue with use client not appropriately being added when used in Next.js 13 app directory, the directive was pulled out of the bundle instead recommending adding the use client directive to the file.

This isn't ideal, where the hope would be someone could use the components in this library interchangeably without having to worry about that additional distinction

This was a regression when moving to tsup, where however it's being bundled / compiled isn't supported or generally working.

Describe the solution you'd like

use client should be bundled with the components that require it, at a minimum CldImage as it uses useState and I believe it's additionally needed due to the loader prop which is a function.

hades200082 commented 1 year ago

Function props can't be passed into client components.

We might need to give this one more thought

colbyfayock commented 1 year ago

Maybe I'm missing something but that's how the Image component works for this type of thing, by passing a function to the loader prop

https://nextjs.org/docs/api-reference/next/image#loader

hades200082 commented 1 year ago

Next/Image is a server component.

You can't pass a function from a server component to a client component because functions can't be serialised.

Ideally the CldImage component could be refactored to remove the need for useState allowing it to be server rendered, at which time this whole "use client" issue goes away anyway.

In addition, it would make the whole thing more performant because the image would be pre-rendered in the page on the server rather than having to be hydrated, evaluated and rendered at page-load time in the browser.

I might take a look at that at some point if you don't get to it first.

hades200082 commented 1 year ago

@colbyfayock just noting here also that React 18 "Shared Components", once supported by NextJS might be another way forwards.

colbyfayock commented 1 year ago

ah got it, thanks for the clarification

currently state is being used in order to force an update to the Next Image component, particularly in the event that the component receives a 423 error, which means the image is processing, that we poll for the results, and refresh the image once it's processed

its helpful for tools like background removal

colbyfayock commented 1 year ago

looks like one option here is multiple entry points: https://github.com/timolins/react-hot-toast/blob/main/tsup.config.ts

could potentially provide a default export of a server component with a separate entrypoint that wraps that component with the clientside helper if it would gracefully work that way

colbyfayock commented 1 year ago

clarification on this thread - Next IMage is a Client component as of July 17th: https://github.com/vercel/next.js/issues/41924#issuecomment-1637940636

the multiple entry points work well, going to spin up a draft PR, though id like to try to solve a way for everything to still be importable from the top level module