Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Output Url Generation approach compared to the V1 pkg #382

Closed
yoadsn opened this issue Apr 23, 2021 · 5 comments
Closed

Output Url Generation approach compared to the V1 pkg #382

yoadsn opened this issue Apr 23, 2021 · 5 comments

Comments

@yoadsn
Copy link
Contributor

yoadsn commented Apr 23, 2021

Hi,
Really want this to work so we can have a smaller bundle.

I tried to convert parts of an app to use this SDK and compared the old SDK urls side by side - here are some examples:
(-old, +new)

-https://rs.wescover.com/c_pad,h_250,w_250/v1/wescover-image-store/e3toavbx0drg7xxozw8c.jpg
+https://rs.wescover.com/c_pad,h_250,w_250/f_jpg/v1/wescover-image-store/e3toavbx0drg7xxozw8c

-https://rs.wescover.com/c_limit,f_auto,q_auto,w_500/v1/wescover-image-store/lhxpyb380gvknqrmdxbj
+https://rs.wescover.com/c_limit,w_500/q_auto/f_auto/v1/wescover-image-store/lhxpyb380gvknqrmdxbj

As you can see, the URLs are not exactly compatible - usage of the / approach vs , and appending an extension vs using the f_ modifier - make the old URLs and new not compatible.

I know the output image will be the same! So why is this important?

Some sites (like us) have been using the old URLs for years, with many (many) images already crawled by search engines (like Google) and in use.
Swapping all URLs suddenly, even if the same image is produced would cause a massive re-crawl of all images since the URL means "another image". The result is:

  1. Huge waste of crawl budget
  2. Huge waste of transfer bandwidth from Cloudinary (which costs$$)
  3. Possible transformations if Cloudinary thinks those new URLs need a new transform (even if the output is identical)
  4. Possible, image ranking could be lost since there is no such thing as "image 404" to advertise to google.

This means for us, URL equivalency, given the same transformation needs is extremely important.

WDYT?

@aleksandar-cloudinary
Copy link

Hi @yoadsn! Of course, it makes sense to be able to generate the same URLs with transformations through both versions.
Could you please share the code snippet you've used to generate those URLs with v1 and v2 versions of the SDK?

@yoadsn
Copy link
Contributor Author

yoadsn commented Apr 24, 2021

@aleksandar-cloudinary Thanks!
Since I was sanity-testing the two version - I did the following to compare them side by side within a real app.
I knew which subset of transformations are in use to created a simple transformer from "options" object to an actions API calls.
This way I just ran the app and any mismatches were printed to the log.

import { Cloudinary } from 'cloudinary-core';

import { Cloudinary as CLD } from '@cloudinary/base';
import { Actions } from '@cloudinary/base';

const cld = new CLD({
  cloud: {
    cloudName: cloudinaryCloudName,
  },
  url: {
    secure: true,
    cname: imageCdnHost,
    secureDistribution: imageCdnHost,
    privateCdn: true,
    useRootPath: true,
    analytics: false,
  },
});

const cloudinaryConfig = {
  cloud_name: cloudinaryCloudName,
  cname: imageCdnHost,
  secure_distribution: imageCdnHost,
  secure: true,
  private_cdn: true,
  use_root_path: true,
};
const cloudinary = Cloudinary.new();
cloudinary.config(cloudinaryConfig);

function cloudinaryUrl(publicId, options) {
  const cimg = cld.image(publicId);

  if (options) {
    if (options.crop) {
      if (options.crop === 'limit') {
        cimg.resize(Actions.Resize.limitFit(options.width, options.height));
      } else if (options.crop === 'pad') {
        cimg.resize(Actions.Resize.pad(options.width, options.height));
      }
    }

    if (options.quality === 'auto') {
      cimg.quality(Actions.Delivery.quality('auto'));
    }

    if (options.fetchFormat || options.format) {
      cimg.delivery(
        Actions.Delivery.format(options.fetchFormat || options.format)
      );
    }

    if (options.transformation && typeof options.transformation === 'string') {
      cimg.namedTransformation(
        Actions.NamedTransformation.name(options.transformation)
      );
    }
  }

  const v2url = cimg.toURL();

  const v1url = cloudinary.url(publicId, { ...options });

  if (v1url !== v2url) {
    console.log(`-${v1url}\n+${v2url}`);
  }
  return v1url;
}

Assuming the order in which you apply those transformations also effects the order they are applied to the URL - another layer of compatibility needs to be in place to ensure unordered "options objects" are always converted in the same order to the functional approach with the V2 SDK.

Also, I think, since in many apps I assume "option objects" are passed down from top level components to lower level image URL producing layers. (at least we have such abstractions in many places). Converting a V1 to V2 is going to be a full re-write of those layers or implementing a conversion function like I did (but of course robust enough to handle all options).

IMHO - this layer should be provided built-into V2 for easier migrations and more importantly compatibility with existing usage.

Also, the API style of V2 is clearly well thought out - trying to be simple to use, typed, future proof etc.
The problem is that it does not lend itself well for configuration based code architecture such that a top level layer can "define a configuration" and let lower layers parse it to URLs.
E.g any top layer who needs control over "width" needs to "know" the SDK methods to define such an action. (Or, a parser needs to be built in between which is what I did in the example above, but clearly is not part of a normal "user-land" code).
This is a down side in my opinion but only my 2cents.

@patrick-tolosa
Copy link
Contributor

patrick-tolosa commented Apr 25, 2021

Hey @yoadsn

Thanks for the well-thought-out input.

To your immediate question, yes there's an in-between way of creating the old URLs, but with better package size.

There's a method called createCloudinaryV1URL exposed at the top of the package that creates the old URLs
You can find test examples here, but the syntax is identical to what you already know.

import {createCloudinaryV1URL} from '@cloudinary/base';

createCloudinaryV1URL(/* v1-style-options */)

As to your second argument, the new SDK is indeed imperative, this is not to say that we aren't considering a more declarative API (on top of, not instead of) as well.

As an extra bonus tip, if you use the new imperative syntax, you can cut down the bundle size even further by only importing what you need at that time.

so instead of import {Actions} from '@cloudinary/base', you could use import {pad} from '@cloudinary/base/actions/resize'

@yoadsn
Copy link
Contributor Author

yoadsn commented Apr 25, 2021

Thank you @patrick-tolosa your answer is spot on and shame on me for not finding that top level import. I just assumed it's not there. Will test it out right away.

@yoadsn yoadsn closed this as completed Apr 25, 2021
@yoadsn
Copy link
Contributor Author

yoadsn commented Apr 25, 2021

Hi @patrick-tolosa after some initial test, it's looking good except for some bugs.
I have opened a PR with failing test that cover those bugs so I hope it would be easy to spot.
Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants