feat: Add descriptive enum constant for Azure content filter configuration - Option 2

[KavithaSiva]

Context

Closes SAP/ai-sdk-js-backlog#181.

What this PR does and why it is needed

This PR adds self descriptive constansts for improving Azure content filter configuration.

[KavithaSiva]

The idea is that AzureContentSafety defined in orchestration-types overrides the type generated from the specification.

Modifying the type doesn't seem to be a breaking change as in the end they would both resolve to be the same code in JS.

[ZhongpinWang]

[q] Can we avoid changing these tests and only add new tests (even just type tests) to avoid merge conflicts with #441? I think it is unnecessary to change those types.

[ZhongpinWang]

Also here, don't change the test. Add a new one (or type test).

[ZhongpinWang]

[req] Why are we not directly overriding AzureThreshold? Currently, the generated AzureThreshold can still be imported. If you are worrying about user hover on AzureContentSafety and got navigate to the generated AzureThreshold type, it is simply not avoidable. Even if we override AzureContentSafety, if user navigate from an outside type, they will still end up in the generated threshold. Unless we don't use wildcard..

[deekshas8]

A note: We actively got rid of enums in the Cloud SDK JS because it doesn't add much value and numeric enums behave weird in TS. We also added an Eslint rule to avoid all future additions. The recommended approach is either union types, or object as const. These alternatives are not only more predictable but also align better with TypeScript's design philosophy.

Unless there is an absolutely compelling reason to use enums, I would suggest we avoid them.

@jjtang1985

[ZhongpinWang]

I put my findings here, also communicated with @deekshas8 .

For js users, these two options does not make a big difference. Both can autocomplete with AzureFilterThreshold and there is no type anyway.

// enum version
export var AzureFilterThreshold;
(function (AzureFilterThreshold) {
    AzureFilterThreshold[AzureFilterThreshold["ALLOW_SAFE"] = 0] = "ALLOW_SAFE";
    AzureFilterThreshold[AzureFilterThreshold["ALLOW_SAFE_LOW"] = 2] = "ALLOW_SAFE_LOW";
    AzureFilterThreshold[AzureFilterThreshold["ALLOW_SAFE_LOW_MEDIUM"] = 4] = "ALLOW_SAFE_LOW_MEDIUM";
    AzureFilterThreshold[AzureFilterThreshold["ALLOW_ALL"] = 6] = "ALLOW_ALL";
})(AzureFilterThreshold || (AzureFilterThreshold = {}));
;

console.log(AzureFilterThreshold.ALLOW_SAFE);

// const object version
export const AzureFilterThreshold2 = {
    ALLOW_SAFE: 0,
    ALLOW_SAFE_LOW: 2,
    ALLOW_SAFE_LOW_MEDIUM: 4,
    ALLOW_ALL: 6
};

console.log(AzureFilterThreshold2.ALLOW_SAFE);

For ts users, using const object and keyof typeof will require user to use strings instead of numbers, which is a breaking change (or @deekshas8 I didn't go further about this type mapping ideas, which basically checks if typeof string, we map it to value (or vice versa).)

// enum version
export enum AzureFilterThreshold {
  ALLOW_SAFE = 0,
  ALLOW_SAFE_LOW = 2,
  ALLOW_SAFE_LOW_MEDIUM = 4,
  ALLOW_ALL = 6
};

const t1: AzureFilterThreshold = AzureFilterThreshold.ALLOW_ALL;

// const object version
export const AzureFilterThreshold2 = {
  ALLOW_SAFE: 0,
  ALLOW_SAFE_LOW: 2,
  ALLOW_SAFE_LOW_MEDIUM: 4,
  ALLOW_ALL: 6
} as const;

type AzureFilterThreshold2Type = keyof typeof AzureFilterThreshold2;
const t2: AzureFilterThreshold2Type = "ALLOW_SAFE_LOW";

In the end, my personal idea is to override the original AzureThreshold, so that user won't see it. Using const object can do override as well, only breaking(?).

[jjtang1985]

A note: We actively got rid of enums in the Cloud SDK JS because it doesn't add much value and numeric enums behave weird in TS. We also added an Eslint rule to avoid all future additions. The recommended approach is either union types, or object as const. These alternatives are not only more predictable but also align better with TypeScript's design philosophy.

Unless there is an absolutely compelling reason to use enums, I would suggest we avoid them.

@jjtang1985

Thanks for adding me, @deekshas8 .
@KavithaSiva shared the two options with me.
As discussed with Kavitha, in general, I would avoid using enum (as aligned in the team), if possible.

However, from my understanding, currently, when applying the option 1 (without using enum), the auto completion does not work, and it seems user would not be able to find our convenient API when using IDE.
Therefore, I feel the option 1 has limited value.
If we dont have other good options (from UX perspective), it seems only option 2 is valid.

As the usage of enum is rather a dev topic, so I would leave the decision to the dev team, even though I'm not a fan of using it for TS.

[KavithaSiva]

Autocompletion differences for:

const

vs

enum

[deekshas8]

Using const object can do override as well, only breaking(?).

We can do the Union Type of 0,2,4 and the string types, but mention the number inputs will be deprecated. That allows us to do it non-breaking

[deekshas8]

Please also add an eslint rule so we dont add enums in the future

[KavithaSiva]

Closed in favour of #485