[email protected]

[colbyfayock]

Description

This PR migrates the repo off of Zod schemas toward pure TS with JSDoc annotations that can eventually be used to extract metadata for display in docs.

Generally the external behavior should be identical, with obvious exceptions where schema entry points are no longer available etc., so this would constitute a breaking change for consumers relying on those entrypoints or whose inputs may no longer be valid with new type safety around video/image options.

I've also updated all the tests to .ts so we can be sure the types for the API are working as we expect going forward.

Although all the tests are passing, the doc generation issue remains unsolved, causing the repo-wide build to fail, so that will need to be updated to use a JSDoc parsing tool. However, it should be quite straightforward to wire that up in place of the previous Zod-embedded metadata.

For now, I'm opening this against the beta branch so the required JSDoc parsing logic can be added and other changes can be experimented with before broader consumption.

Issue Ticket Number

N/A

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Fix or improve the documentation
• This change requires a documentation update

Checklist

(Still a draft, need to figure out how to work around missing schemas in build)

• I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
• I have created an issue ticket for this PR
• I have checked to ensure there aren't other open Pull Requests for the same update/change?
• I have performed a self-review of my own code
• I have run tests locally to ensure they all pass
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes needed to the documentation

BREAKING CHANGES: removes zod schemas, refactors type system

Description

Issue Ticket Number

Fixes #<ISSUE_NUMBER>

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Fix or improve the documentation
• This change requires a documentation update

Checklist

• I have followed the contributing guidelines of this project as mentioned in CONTRIBUTING.md
• I have created an issue ticket for this PR
• I have checked to ensure there aren't other open Pull Requests for the same update/change?
• I have performed a self-review of my own code
• I have run tests locally to ensure they all pass
• I have commented my code, particularly in hard-to-understand areas
• I have made corresponding changes needed to the documentation

[colbyfayock]

packages released for testing, may need to install all the beta dependencies for it to work properly

[colbyfayock]

@ssalbdivad - @ghostdevv noticed an issue where when using only CldImage, because all of the plugins run, she noticed ABR plugin warnings

CldImage would not directly set any assetType but assetType should default to image (or at least it did)

any ideas what might be causing that?

[ssalbdivad]

@colbyfayock What API do you mean specifically by "only CldImage"?

I don't think the logic around defaulting assetType changed at all in constructCloudinaryUrl (see below).

[colbyfayock]

the example given was a page that simply only loaded CldImage, so it appears by using CldImage (asset type image), we get warnings about trying to use video plugins

[ssalbdivad]

@colbyfayock Ahh okay I see the issue here. You can fix it by migrating AbrPlugin to use the new applyWhen logic to make it clearer when the plugin should do anything. It also handles the typeof check for you implicitly!

In abr.ts:

  applyWhen: "streamingProfile",
  apply: (asset, opts) => {
    asset.addTransformation(`sp_${opts.streamingProfile}`);

    return {};
  },

applyWhen accepts either a single key representing a key that must have a value for the plugin to apply (streamingProfile, in this case). This covers most of the cases pretty easily and narrows what you get for opts in apply so that you know that key is defined. However, there's a fallback for more complex logic where you can just provide a function that can access all the options and return a boolean for whether the plugin should apply. It was an oversight on my part not to add that as a note to the plugin function so maybe you want to copy some of it over :P

Would potentially be worth migrating more of these over in the future as the inverse problem could occur for a video asset where you get a bunch of warnings about image types. I think it should result in the logic for actually handling what the plugin does at that point much clearer though!

[ssalbdivad]

@colbyfayock Also looking back on why exactly this change was made, some of the motivation was that we didn't have the explicit props object we just added back in that listed the relevant keys.

Potentially you could remove applyWhen in favor of just checking if any of those props are present? I wanted to leave it more open ended because some of the plugin logic seemed complex and I wasn't sure there was a 1:1 between one of the owned props being present and that plugin applying.

I think in either case, it would be super helpful for separation of concerns if the logic of "should this plugin do anything based on these options" lived outside each individual plugin, so if we can do that by adding some centralized logic that checks for props overlap similarly to before and then only run apply in those cases, that may be ideal.

[github-actions]

🎉 This PR is included in version @cloudinary-util/util-v4.1.0 🎉

The release is available on:

• pnpm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version @cloudinary-util/url-loader-v5.11.0 🎉

The release is available on:

• pnpm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This PR is included in version @cloudinary-util/types-v1.6.0 🎉

The release is available on:

• pnpm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀