[Bug] URL Loader missing type definitions for effects on ImageOptions

[colbyfayock]

Bug Report

Describe the bug

ImageOptions doesn't include all of the effects such as pixelate that are applied via the effects plugin

Blocking being able to support these features in Nuxt Cloudinary

https://github.com/nuxt-modules/cloudinary/issues/119

[colbyfayock]

Notes from chatting with Josh Goldberg (thanks)

This is to help with how we define types for Plugins of the URL Loader

Plugin Types

The general methodology we talked about was around locating types near -if not generated from- the place(s) in code that ideate them:

• If a plugin file (e.g. fill-background.ts) creates the idea of certain options, then put the types and/or Zod schemas in that specific file.
• If multiple files happen to use the exact same shape, it makes sense to give it a more generic name and pull it out into its own file.

[danieltott]

@colbyfayock want to assign this one to me too?

[colbyfayock]

yes!

[colbyfayock]

hey @danieltott i know this was a monster of an Issue to work on, but just wanted to check in and see how it's going, if you're still working on it, and if so, if I can help in any way to get it past the finish line for you!

[danieltott]

Hey @colbyfayock! It's going pretty well - finally have my head wrapped around most of it I'm pretty sure. I filed an issue/PR alllll the way up at cloudinary/js-transformation-builder-sdk that will probably be required to get this done well: https://github.com/cloudinary/js-transformation-builder-sdk/issues/32

That thing is preventing us from passing string values to CloudinaryImage.effect() without using any and will allow us to make use of types coming from upstream.

Do you have any access to that repo and/or know the maintainers?

[danieltott]

I'm sure I could work around it if that's a no-go, but it would be nice/ideal to be about to count on the types coming from upstream to be correct.

[colbyfayock]

oh okay, i was flagged on that Issue but didn't realize it was a requirement. definitely in contact with the maintainers but im not quite sure if there is any realistic timeline of that getting addressed as they're pretty slammed and i konw there's some precedent around preferring to pass in Actions in some instances, so not sure if theyd go for it. i was somewhat bypassing that here for what i consider is an easier to read usage as well as being able to avoid importing all the additional modules to support that (lighter package)

ill try to see what i can find out, but my guess is we'd probably want to look at a workaround, but agree having something coming from upstream would be ideal

[danieltott]

@colbyfayock yeah that makes sense. Definitely wasn't planning on enabling using the actual actions there - it's just frustrating since those actions get passed directly to addAction which totally does take a string and works well as shown here in this lib.

I'll start working assuming we'll have to work around, but if there's any chance of this getting merged that would be the most ideal.

[colbyfayock]

@danieltott getting some pushback on the changes, but im realizing i think we may be able to get around it with avoiding using effect and using addTransformation everywhere. i dont think there would be negative consequences there as i think they may in effect do the same thing

i can play around with this and test the changes locally to see if tests pass, and if so i can push it out, which will resolve this for you right?

[danieltott]

Yeah I had already started using addAction instead of addEffect (I think that's what they're called - on my phone atm)

That's working well so far 👍

[colbyfayock]

oh awesome, sounds great!

[colbyfayock]

hey @danieltott i know hacktoberfest has passed, but did you ever start digging into this?

[danieltott]

Yes! A lot. I actually had some questions but they're sort of "big picture" questions at this point.

I guess the big thing is...the way that this library applies transforms is not really 100% compatible with the way that cloudinary actually uses transforms.

For instance something like gravity - gravity is only a qualifier, and so shouldn't be set on it's own but as a qualifier for specific actions (and the list of actions changes based on the value of gravity). But since there's only one gravity prop at the top level, it can't be set differently for different actions.

So I keep running into situations where to really fix this accurately we'd have to change the API, which would obviously result in breaking changes etc.

It seems like sometimes things are starting to get set up like this, like with the overlays prop, which accepts a list of objects that can set their own qualifiers. But others are stuck with a string, but it's not possible to just inject the entire string with qualifiers, since the library attaches prefixes and sometimes applies qualifiers.

BTW this is not at all meant as a criticism - I know first-hand the ~pain~joy of creating and maintaining a library over years of updates etc.

I have some good ideas for updating things more towards a system that will work well with cloudinary, but it seems like it might be too big of a change for a non-maintainer to even suggest, much less do (not that I'm not willing to work on it).

The other option would be to simply try to add in the missing props and have them work the same way as the others. In that case I'll probably just have some specific questions about how to handle certain scenarios.

Here's a good case since I've worked through this one already (since it starts with a 😂): angle.

angle can be used as an action, where it takes a number (45), a mode keyword (vflip, hflip, or ignore), or multiple combinations of the above joined with . ('45.hflip.23.vflip').

However, it can also be used as a qualifier with a cropping action, where it must be either auto_right, auto_left, or a combination that begins with one of those.

I could add an angle prop that just accepts a string and doesn't really do anything else aside from apply a_${angleString} to the url.

Or I could provide an action and qualifier options for angle and expose an API like this:

const Y = constructCloudinaryUrl({
  options: {
    src: 'https://res.cloudinary.com/demo/image/upload/sample.jpg',
    angle: ['hflip', 45],  //typed against angle action
    fill: {
      width: 300,
      aspectRatio: 0.7,
      angle: 'auto_left', //typed against angle qualifier
    },
  }
})

Where angle and fill have knowledge about what options are valid and what's allowed to be in there (based on the cloudinary docs, which are actually missing angle as an optional qualifier to fill but that's besides the point...).

Would be happy to discuss my thoughts more or even meet up if you wanted - I'm down to keep working on this - I just need some guidance on direction 👍

[colbyfayock]

starting off by saying thanks for how much thought you've put into this. because of the amount of work you've already put into this id still like to send you a hacktoberfest swag kit. email here about that with your github username and a link to this issue: hacktoberfest@cloudinary.com

next a little bit of context. my goal with this library and the downstream consumers is to create an intuitive API with the idea that people want to do something but they don't really care how it happens under the hood. for instance, with AI Generative Fill, you're required to provide a mix of different transformations to ultimately achieve the effect:

b_gen_fill,c_mpad,h_1100,w_1100

but i dont want someone to have to think about that or worry about all of those details, where we already have the image size they want rendered, why do we need all of that? so by default, if you do:

width="1100"
height="1100"
fillBackground

That will automatically be taken care of

https://github.com/colbyfayock/cloudinary-util/blob/main/packages/url-loader/src/plugins/fill-background.ts

(fwiw i dont think fillBackground is the most perfect property name, but couldnt think of something better, and i didnt think generativeFill really descriptively worked well for what was happening)

or a simpler one of background removal, rather than knowing the exact transformation, a little bit easier way is to just simply say:

removeBackground

https://github.com/colbyfayock/cloudinary-util/blob/main/packages/url-loader/src/plugins/remove-background.ts

so far people have seemed to like this approach, i feel like it's more in line with how people think as opposed a bunch of configurations on how to get there

thats what led to the API for overlays for instance, where someone knows they want to add an overlay, but how do you get there? you can have multiple, so an array of objects makes sense, and each can optionally be positioned, each overlay instance can have its own effects applied to it, etc.

but anyways, i dont think any of this is quite perfect yet, and i think you've pointed out some of the holes that have yet to be addressed

i feel that the way that Cloudinary exposes some of the APIs like Actions within the official SDKs makes more sense in how we internally think about it and construct it, but not as much for the consumer (correct me if im wrong). it's certainly important, and some people do care about that, so its good to have it documented why. i think the example code you sent supports that, where someone doesn't really care which one is an "action" or "qualifier", all they care about is can i configure it how i need where i need it?

so with that in mind, i think that example is a fantastic representation, where depending on where it's used, it may have different context

as far as moving forward, id absolutely love to hear your thoughts on how to restructure this, but in the nearterm, i think my recommendation would be to provide type coverage generically, such as using simple strings or if it's an easy enum case we can add it, then perhaps on another Issue, or Discussion, perhaps there's room for a "formal" RFC to really start to think about all of the moving pieces

what do you think?

[danieltott]

starting off by saying thanks for how much thought you've put into this. because of the amount of work you've already put into this id still like to send you a hacktoberfest swag kit.

Thank you! 🎉

I think that plan sounds good in general for sure, and I definitely like that approach of concentrating on how a consumer would think about it.

However your example perfectly illustrates one of the issues:

width is a qualifier, and can be used in different ways. When combined with cropping, it specifies the width of the final crop. But when applied to e_blur_region for example, it specifies the width of the blurred region.

So reducing complexity is good - but when done in a way that doesn't cover all the bases it can get a little more confusing.

I guess one way to handle that would be to treat width and height top-level options to just always be qualifiers of cropping (and have a default cropping which can be overridden by crop at the top level), since that would be an expected outcome, especially when used in a down-stream library like cloudinary-next - this example it would be pretty unreasonable to assume that width and height do anything but change the global width and height of the outputted asset:

<CldImage
  width="987"
  height="987"
/>

So that's what I mean about probably running in to situations where I'd need input on a case-by-case basis.

[colbyfayock]

yeah, good pointout. thats exactly where my minds at, where width and height are contextual and in the top level, they're modifying the image itself, similarly being right next to other properties that control the image such as the src or format. i like to think of it as the main / base layer where you can then add additional layers using overlays, each have their own context, similar to photoshop if you're familiar

in the example you gave, e_blur_region, the shape might look like:

constructCloudinaryUrl({
  options: {
    src: 'sample',
    width: 1000,
    height: 1000,
    blurRegion: {
      x: 10,
      y: 10,
      width: 300,
      height: 100
    },
  }
})

since this isn't intended to follow the chaining pattern of transformations, unless you use effects/rawTransformations explicitly, the context is a little bit different. i guess you can kind of think of it as the top level describes what you're trying to do where you then describe the qualifiers you want to use for that context

but yeah, lmk if you have any more questions in the meantime and we can follow up a bigger discussion

appreciate all the thought here

[colbyfayock]

hey @danieltott haven't seen you reach out about the Hacktoberfest swag. please do as the cutoff to claim swag will be end of day this Friday 11/17. thanks

[colbyfayock]

that is if you still plan on completing this :D

[danieltott]

Hey - just wanted to confirm the address is hacktoberfest@cloudinary.com - I send an email there when you first mentioned that. Sent another one just now 👍🏼

[colbyfayock]

went to spam for some reason it seems 😅 got it now and replied

[colbyfayock]

hey @danieltott do you still intend to finish this out?

[danieltott]

Yes! I'm sorry for the delay. I should have something up next week!

[colbyfayock]

awesome thanks Dan! feel free to PR the progress even if it's not 100% complete, we can move forward with whatever you got

[colbyfayock]

hi @danieltott - i was trying to wait until you had some of this in to begin work on other items to avoid conflicts, as i realize this will be a wide-spanning project, but at this point im going to move forward.

if you have any work in progress, it would be great to at least get that in, though unsure if you've actually began

thanks

[colbyfayock]

Resolved via https://github.com/colbyfayock/cloudinary-util/pull/121