cloudinary-community / cloudinary-util

https://cloudinary-util.vercel.app
MIT License
12 stars 19 forks source link

feat: added Zod schema for the effects array in the overlay schema #174

Closed nickytonline closed 1 month ago

nickytonline commented 3 months ago

Description

This PR adds the effect schema to the effects property of an overlay. Prior to the change, it was an empty object.

It was mentioned in #169 to update the effects with radius into main/packages/url-loader/src/constants/qualifiers.ts#L90, but the radius schema is already in there.

From what I can tell, only the overlay schema needed to be updated. Side note, but this doesn't just fix the issue with radius not being found, but adds all the available effects settings to the overlay effects array schema. I'm assuming that all effects are available in an overlay @colbyfayock, or is it only a subset of effects that are available to overlays?

~I marked this as a breaking change in case anyone using the cloudinary-util package has a value which is not in the newly introduced Zod enum, as previously it was any string. If that is the case, that gravity option probably never worked for them, but just erring on the safe side for a release.~

I haven't made any documentation updates here as there does not appear to be any docs related to the options including gravity, but if there is a separate repository that requires a documentation update, happy to make changes there as well.

Issue Ticket Number

Closes #169

Type of change

Checklist

nickytonline commented 3 months ago

I've run the build and tests locally, but just wondering if I need to pnpm link into a project like the getCldOgImageUrl to test, or is the test suite here good enough to confirm the fix?

colbyfayock commented 3 months ago

thanks @nickytonline - yeah the docs will be autogenerated from the zod schema, it may not fully expose it yet, but that's the WIP goal

the overlay effects options are a bit complicated, it accepts a lot of different options: https://github.com/cloudinary-community/cloudinary-util/blob/main/packages/url-loader/src/plugins/overlays.ts#L196-L199

there's also the option of passing in text effects: https://github.com/cloudinary-community/cloudinary-util/blob/main/packages/url-loader/src/plugins/overlays.ts#L311

it may be worth doing something similar to what we did with Gravity, where we configure it so that we can expose all of the available options, but allow someone to free add options, at least for now to avoid over-restricting, until we're more confident everything is covered

i need rework some of that code, both the overlay code itself and ive been slowly reworking how some of hte parameters are organized and managed, but its not a high priority at the moment

got some community input on the breaking change aspect as my instincts were that this shouldnt be a breaking change as it's really fixing a type that isn't accurate

https://x.com/colbyfayock/status/1825542769412297144

few particular points:

IMO allowing an invalid input is a types bug, so this would be ok as a patch release. https://x.com/JoshuaKGoldberg/status/1825546401465729187

I would say it’s a bug fix. The user should not have passed the invalid option and is helped by TypeScript now showing an error. https://x.com/remcohaszing/status/1825565394805063989

the previous path was silently failing, and the type wasn’t helping. Notice that changing the type helps someone fix the runtime bug. https://x.com/chriskrycho/status/1825573173380686035

colbyfayock commented 2 months ago

hey @nickytonline i feel like i saw a comment come across but i can't find it. did you have any thoughts on my notes about the type?

nickytonline commented 2 months ago

I think I had one in draft. I think the same approach as the previous PR makes sense.

colbyfayock commented 2 months ago

sounds good, did you want to do that, or did you want me to?

nickytonline commented 2 months ago

I'm a little busy with work atm, so go for it.

nickytonline commented 1 month ago

Going to close this since you've taken over.