Update Serving API - from backend PR #6604 - #379
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates numerous TypeScript type and constant definitions to enums within packages/types/src/config/types.gen.ts and introduces new enums for PlanStatus and Products. The reviewer identified critical issues where the new PlanStatus and Products enums conflict with existing definitions, potentially causing compilation errors. Additionally, the use of intersection operators for KeyValueMatchRulesTypes and RulesTypes needs to be replaced with union operators to avoid never types, and there is a recommendation to improve non-descriptive enum names like type for better maintainability.
| export enum PlanStatus { | ||
| PAID = 'paid', | ||
| TRIAL = 'trial', | ||
| TRIAL_EXPIRED = 'trialExpired', | ||
| CANCELED = 'canceled', | ||
| PAUSED = 'paused' | ||
| } |
There was a problem hiding this comment.
The addition of the PlanStatus enum here creates a duplicate identifier conflict with the existing type PlanStatus and const PlanStatus definitions located at lines 61 and 66 of this file. This will cause a compilation error in TypeScript. Please remove the redundant definitions further down in the file.
| export enum Products { | ||
| EXPERIENCES = 'experiences', | ||
| DEPLOY = 'deploy', | ||
| ADDONS = 'addons' | ||
| } |
There was a problem hiding this comment.
The addition of the Products enum here creates a duplicate identifier conflict with the existing type Products and const Products definitions at lines 81 and 90. This will result in a "Duplicate identifier" error. The old definitions should be removed to maintain consistency with the new enum-based approach.
| JS_CONDITION = 'js_condition' | ||
| } | ||
|
|
||
| export type KeyValueMatchRulesTypes = GenericTextKeyValueMatchRulesTypes & GenericNumericKeyValueMatchRulesTypes & GenericBoolKeyValueMatchRulesTypes; |
There was a problem hiding this comment.
Using the intersection operator & with enums (which are now string-based) results in the never type, as a value cannot be a member of multiple distinct enums simultaneously. This should be a union type using the | operator to allow any of the key-value match rule types.
| export type KeyValueMatchRulesTypes = GenericTextKeyValueMatchRulesTypes & GenericNumericKeyValueMatchRulesTypes & GenericBoolKeyValueMatchRulesTypes; | |
| export type KeyValueMatchRulesTypes = GenericTextKeyValueMatchRulesTypes | GenericNumericKeyValueMatchRulesTypes | GenericBoolKeyValueMatchRulesTypes; |
| WEATHER_CONDITION = 'weather_condition' | ||
| } | ||
|
|
||
| export type RulesTypes = TextMatchRulesTypes & NumericMatchRulesTypes & BoolMatchRulesTypes & KeyValueMatchRulesTypes & VisitorDataExistsMatchRulesTypes & CookieMatchRulesTypes & CountryMatchRulesTypes & VisitorTypeMatchRulesTypes & LanguageMatchRulesTypes & GoalTriggeredMatchRulesTypes & SegmentBucketedMatchRulesTypes & DayOfWeekMatchRulesTypes & HourOfDayMatchRulesTypes & MinuteOfHourMatchRulesTypes & BrowserNameMatchRulesTypes & OsMatchRulesTypes & WeatherConditionMatchRulesTypes; |
There was a problem hiding this comment.
Similar to KeyValueMatchRulesTypes, the RulesTypes definition should use the union operator | instead of the intersection operator &. As currently written, this type evaluates to never, which will break any logic relying on a generic rule type.
| export type RulesTypes = TextMatchRulesTypes & NumericMatchRulesTypes & BoolMatchRulesTypes & KeyValueMatchRulesTypes & VisitorDataExistsMatchRulesTypes & CookieMatchRulesTypes & CountryMatchRulesTypes & VisitorTypeMatchRulesTypes & LanguageMatchRulesTypes & GoalTriggeredMatchRulesTypes & SegmentBucketedMatchRulesTypes & DayOfWeekMatchRulesTypes & HourOfDayMatchRulesTypes & MinuteOfHourMatchRulesTypes & BrowserNameMatchRulesTypes & OsMatchRulesTypes & WeatherConditionMatchRulesTypes; | |
| export type RulesTypes = TextMatchRulesTypes | NumericMatchRulesTypes | BoolMatchRulesTypes | KeyValueMatchRulesTypes | VisitorDataExistsMatchRulesTypes | CookieMatchRulesTypes | CountryMatchRulesTypes | VisitorTypeMatchRulesTypes | LanguageMatchRulesTypes | GoalTriggeredMatchRulesTypes | SegmentBucketedMatchRulesTypes | DayOfWeekMatchRulesTypes | HourOfDayMatchRulesTypes | MinuteOfHourMatchRulesTypes | BrowserNameMatchRulesTypes | OsMatchRulesTypes | WeatherConditionMatchRulesTypes; |
| export enum type { | ||
| GA3 = 'ga3' | ||
| } |
There was a problem hiding this comment.
The enum name type is non-descriptive and potentially confusing as it is also a TypeScript keyword. While this file is auto-generated, it is highly recommended to improve the naming in the source OpenAPI specification or generator configuration to something more specific, such as IntegrationTypeGA3.
652c47a to
9858341
Compare
9858341 to
c379e1d
Compare
|
3 participants
Updating TS Serving API after the latest changes from backend repo,
PR #6604