ascorbic / unpic

Universal image CDN translator
https://unpic.pics/lib
292 stars 35 forks source link

cloudinaryRegex not covering all requirements #8

Closed codercatdev closed 1 year ago

codercatdev commented 1 year ago

https://cloudinary.com/documentation/advanced_url_delivery_options#private_cdns_and_cnames

I am horrible at Regex or I would help fix this https://github.com/ascorbic/unpic/blob/7b05563beba186c2c2d091d67729bbbceb25a5b2/src/transformers/cloudinary.ts#L10

Test Cases

Basic https://res.cloudinary.com/demo/image/upload/c_lfill/dog

Private https://private-name.cloudinary.com/demo/image/upload/c_lfill/dog

Custom Cname https://media.codingcat.dev/ajonp/image/upload/v1658570793/main-codingcatdev-photo/CSS-Animations.png

Because I leave mine open this is equivalent to https://res.cloudinary.com/ajonp/image/upload/v1658570793/main-codingcatdev-photo/CSS-Animations.png

Right now this does not pass the regex test so it is not getting the correct srcsets

codercatdev commented 1 year ago

I just found that there is an issue with versions as well, both are valid:

Works https://res.cloudinary.com/ajonp/image/upload/v1658570793/main-codingcatdev-photo/CSS-Animations.png

Does not work https://res.cloudinary.com/ajonp/image/upload/main-codingcatdev-photo/CSS-Animations.png

Antonio-Bennett commented 1 year ago

Hmm @codercatdev from what I understand then this url https://res.cloudinary.com/mabelslabels/image/upload/v1676569378/Dev-Mabels3.0-Assets/Home/main.png should work correct? However I get incorrect cloudinary url error when I set this as source.

codercatdev commented 1 year ago

Hmm @codercatdev from what I understand then this url https://res.cloudinary.com/mabelslabels/image/upload/v1676569378/Dev-Mabels3.0-Assets/Home/main.png should work correct? However I get incorrect cloudinary url error when I set this as source.

Yes @Antonio-Bennett this is a valid image url on Cloudinary.

This is going to be such a complex check for cloudinary at least, if you look at their sdk there are a lot of toggles depending on what you have setup.

colbyfayock commented 1 year ago

this is the latest regex version i've been using in my own tooling

https://github.com/colbyfayock/cloudinary-util/blob/main/packages/util/src/lib/cloudinary.ts#L2

2 issues here - the host and the version

it's very difficult if not impossible to handle all possible scenarios both with and without the version. spent a lot of time playing with this and trying to get people to help with no luck: https://regex101.com/r/DBOUaH/2 (these examples dont include custom hosts)

for instance, thinking about .../upload/f_auto/myid

is f_auto a transformation or a folder name? seems like a silly question, but it's something you can do. and might be more of an actual use case with the 100's of other parameters we support. im not sure how this is handled on the backend

thats why ive resorted to requiring a version if im doing any parsing of URLs with my tool linked above

as far as the host is concerned, not sure why the unpic version isn't working as it clearly captures the host

image

i think the issue is there are explicitly defined hosts that are mapped to the URL patterns:

https://github.com/ascorbic/unpic/blob/50b3f8f3080793616edbf11ece7b06274d9c7d34/data/domains.json

and not reasonable to include custom ones in there, so not sure how someone could solve configuring a custom host here

codercatdev commented 1 year ago

I think the best thing to offer is that when I add the paramater cdn: "cloudinary", know that I understand what url I am using. Then if we need to add another parser into the fold that could work further, and not just fail the test.

Antonio-Bennett commented 1 year ago

Just to update. My cloudinary link is indeed valid but the folder name is messing up regex parsing I think. Probably the dashes in the name. Switching the folder name to be something else worked for me.

ascorbic commented 1 year ago

Hi all, I've just merged #29, which fixes the domain issue. It should now detect custom subdomains, and handle cusotm domains properly if you manually set the CDN.

@codercatdev I checked the URLs with and without versions that you shared in your comment and they both work for me. Perhaps it was fixed in another PR?

I think I've checked all of the examples in the thread, but it would be good if others could check the playground and see if it works for them too.

codercatdev commented 1 year ago

@ascorbic I tried this

https://media.codingcat.dev/image/upload/v1/main-codingcatdev-photo/FlutterFlameFlappy

And

https://media.codingcat.dev/image/upload/main-codingcatdev-photo/FlutterFlameFlappy

In the playground and still get undefined.