Make types more strict

[karlhorky]

Brief Summary of Changes

Make types more strict, to disallow typos on common literal string option values. Without these changes, there is no type safety for correct option values to be used as transformation options.

What Does This PR Address?

• [ ] GitHub issue (Add reference - #XX)
• [ ] Refactoring
• [ ] New feature
• [x] Bug fix
• [ ] Adds more tests

Are Tests Included?

• [ ] Yes
• [x] No

Reviewer, Please Note:

cc @patrick-tolosa

[patrick-tolosa]

@karlhorky Thanks for the PR. The reason these string types exist, even though if they're less accurate, is to accommodate simple future proofing when the new features are added to the API.

At the end, the SDK is a proxy to the transformation API, and we'd like to avoid forcing people to update the SDK just to use a new transformation API feature.

Yes the SDK will include all the future changes, but it's easier for existing users to use the looser types when updates are introduced.

[karlhorky]

I was thinking that some kind of leniency was the original reasoning behind this, but it does not help TypeScript tooling to have string | <other literals>.

With this change, the cloudinary library behaves more like untyped JavaScript. You are losing features like:

• Preventing typos
• Autocomplete

To demonstrate this:

With string

No real autocomplete on possible values - it only knows about abc and def because they are literals within the same file)

https://www.typescriptlang.org/play?#code/C4TwDgpgBAglC8UDeAoKUCGB+AXFAzsAE4CWAdgOZQA+UA5BgEYDGdN9AJhAGZ0A0KAL4oUzAPZlCmPHESp0GPHTpCUQA

Using only literal types in the union

Autocomplete works, detection of typos works, etc

https://www.typescriptlang.org/play?#code/C4TwDgpgBAglC8UDeAoKUCGB+AXFA5BgEYDG+UAPgQCYQBm+ANCgL4ookD2AdgM7CY8cRKnQY8+fKxRA

For this reason, I would argue that it's better DX for users to have the strict types and that this repo just gets updates often when new changes are introduced to the API. In my experience working with other external services and APIs, this is a more common approach that improves the experience for users.

[patrick-tolosa]

I honestly believe you're right, but the issue isn't with the quickness of updating this project.

In fact, many people still use old versions of this SDK, and are unable (or unwilling for their own good reasons) to update the SDK.

If we block future proofing, sometime down the line a person would not be able to use a new API feature because his SDK is out-dated, and perhaps that person cannot update his SDK at this time. That's the use-case we're aiming to address with future proofing.

This will mean that we'll be forcing them to handle TS errors on their end (with TS-ignore) if they'd like to use new features that don't exist yet in their old version.

[karlhorky]

Yeah, this is a common thing in software in general - it becomes out of date and unsupported (also in big, very visible projects such as Node.js, macOS, etc). Deprecating old versions is part of the cycle.

There are many things that could break if using older versions of things, and I think it's not the maintainer's responsibility to support every release of a piece of software indefinitely into the future (that would also be a nightmare - at least with how software is currently built).

So I would suggest that it's ok - and in this case, even desirable - to have an SDK that fits the API like a glove.

[patrick-tolosa]

Unfortunately this is where software becomes opinionated, with no one clearly in the right. It's currently Cloudinary's point of view that supporting older users is more beneficial than having a stricter type definition - I can see why some would disagree with this approach.

However, since this is a matter of opinion I'm closing this PR, if you have a suggestion that improves types while keeping this type of functionality and support in place we'll be more than happy to review it.

Thanks again for wanting to make this more friendly to others!

[karlhorky]

if you have a suggestion that improves types while keeping this type of functionality and support in place we'll be more than happy to review it

You could recommend that any users who want to keep their old version either:

1. stay on version 1.32.0 (if they don't want to upgrade anyway)
2. patch their package with patch-package to add back the string

Would definitely recommend strongly to not keep these more general types in the codebase, it reduces the effectiveness and point of using TypeScript.

[patrick-tolosa]

Perhaps the same argument falls for the use for future keys in many of the objects. I agree it reduces the effectiveness, but this decision is around balancing various things, including type correctness.

[karlhorky]

Hm, just can't see it - I think it's a negative outcome with only a weak potential upside. If there were precedent for this elsewhere with some justification then I'd reconsider, but I don't see any other service / SDK providers that made this decision to neuter the power of literals.

If the library cloudinary should decide to ignore all of these warnings and community precedent and still move forward with string, Object and similar, you might as well remove the literal types altogether 😬 less code for you to maintain and ship to your users. The literals are useless anyway, currently.

[karlhorky]

@patrick-tolosa Just saw the tweet from @thoughtspile and found a way that the literals can still show up in autocomplete:

• https://github.com/cloudinary/cloudinary_npm/pull/595/