Update Serving API - from backend PR #5294 - #315
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @clllaur, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request updates the TypeScript API definitions, specifically within packages/types/src/config/types.gen.ts, to synchronize with recent changes in the backend. The primary focus is on expanding the API's capabilities related to audience targeting and visitor data retrieval, alongside some general type clean-up and documentation improvements.
Highlights
- New Rule Types for Audience Targeting: Introduced
VisitorIdMatchRuleandGroupIdMatchRuletypes, along with their correspondingrule_typeconstants. These new rules are integrated intoRuleElementNoUrland a newly addedRuleElementAudiencetype, enabling more granular audience targeting based on visitor and group identifiers. - Structured Audience Rule Definitions: Added the
RuleObjectAudiencetype, which provides a structured way to define complex logical rules for audience matching, leveraging the newRuleElementAudiencetype for its elements. - New Type for Uploaded File Data: Introduced the
UploadedFileDatatype to represent uploaded file objects. This type includes properties for the file's storage key, original name, and a read-only URL for access. - Clarification on Global JavaScript Behavior: Updated the JSDoc comment for the
global_javascriptproperty withinConfigProjectto clarify its behavior. It now explicitly states that this code will be combined with environment-specific global JavaScript by appending it with a newline character when an environment is specified in the request. - API Endpoints for Visitor Data Retrieval: Implemented new API endpoints (
/visitor-data/{account_id}/{project_id}and/visitor-data/{sdk_key}) to allow for the retrieval of visitor-specific data. Corresponding request types (GetVisitorDataData,GetVisitorDataBySdkKeyData) and a shared response type (VisitorDataResponseData) have also been added. - Project Configuration Type Refinement: Removed the
environmentsproperty from theConfigProjecttype, streamlining the project configuration definition.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
The pull request updates the TypeScript Serving API types, introducing new rules, data structures, and API endpoints. There are areas for improvement regarding naming conventions, code duplication, and documentation clarity, which could impact the long-term maintainability and usability of the SDK types. I've provided specific suggestions to address these points.
| export type rule_type = 'visitor_id'; | ||
|
|
||
| export const rule_type = { | ||
| VISITOR_ID: 'visitor_id' | ||
| } as const; | ||
|
|
||
| export type GroupIdMatchRule = BaseRuleWithStringValue & { | ||
| rule_type: 'group_id'; | ||
| matching?: (BaseMatch & { | ||
| match_type?: TextMatchingOptions; | ||
| }); | ||
| }; | ||
|
|
||
| export type rule_type2 = 'group_id'; | ||
|
|
||
| export const rule_type2 = { | ||
| GROUP_ID: 'group_id' | ||
| } as const; |
There was a problem hiding this comment.
The names rule_type and rule_type2 are not descriptive and could lead to confusion. rule_type2 in particular is a poor name that suggests it's a temporary or automatically generated name, which harms code clarity and maintainability. Additionally, exporting a type and a constant with the same name (rule_type) is valid in TypeScript but can be confusing. Refactor these to use more descriptive and distinct names.
export type VisitorIdRuleType = 'visitor_id';
export const VisitorIdRule = {
VISITOR_ID: 'visitor_id'
} as const;
export type GroupIdRuleType = 'group_id';
export const GroupIdRule = {
GROUP_ID: 'group_id'
} as const;|
|
||
| export type RuleElement = GenericTextMatchRule | GenericNumericMatchRule | GenericBoolMatchRule | GenericTextKeyValueMatchRule | GenericNumericKeyValueMatchRule | GenericBoolKeyValueMatchRule | CookieMatchRule | CountryMatchRule | LanguageMatchRule | GoalTriggeredMatchRule | SegmentBucketedMatchRule | DayOfWeekMatchRule | HourOfDayMatchRule | MinuteOfHourMatchRule | BrowserNameMatchRule | OsMatchRule | WeatherConditionMatchRule | VisitorTypeMatchRule | JsConditionMatchRule; | ||
|
|
||
| export type RuleElementAudience = GenericTextMatchRule | GenericNumericMatchRule | GenericBoolMatchRule | GenericTextKeyValueMatchRule | GenericNumericKeyValueMatchRule | GenericBoolKeyValueMatchRule | CookieMatchRule | CountryMatchRule | LanguageMatchRule | GoalTriggeredMatchRule | SegmentBucketedMatchRule | DayOfWeekMatchRule | HourOfDayMatchRule | MinuteOfHourMatchRule | BrowserNameMatchRule | OsMatchRule | WeatherConditionMatchRule | VisitorTypeMatchRule | JsConditionMatchRule | VisitorIdMatchRule | GroupIdMatchRule; |
There was a problem hiding this comment.
The type RuleElementAudience is identical to RuleElementNoUrl defined on line 698. To avoid code duplication and improve maintainability, define RuleElementAudience as a type alias for RuleElementNoUrl.
export type RuleElementAudience = RuleElementNoUrl;| /** | ||
| * This one describes a logical rule that is being used inside the app for triggering goals, matching audiences etc | ||
| */ | ||
| export type RuleObjectAudience = { | ||
| /** | ||
| * This describes an outer set of blocks which are evaluated using OR's between them | ||
| */ | ||
| OR?: Array<{ | ||
| /** | ||
| * This describes a colections of logical blocks which are evaluated using AND's between them | ||
| */ | ||
| AND?: Array<{ | ||
| /** | ||
| * This describes a colections of logical blocks which are evaluated using OR's between them | ||
| */ | ||
| OR_WHEN?: Array<RuleElementAudience>; | ||
| }>; | ||
| }>; | ||
| } | null; |
There was a problem hiding this comment.
This type RuleObjectAudience is structurally identical to RuleObjectNoUrl. Assuming RuleElementAudience is aliased to RuleElementNoUrl as suggested in another comment, this type also becomes a duplicate and can be simplified to an alias. The JSDoc comment "This one describes..." is a bit informal. Rephrase it to be more direct, for example: "Describes a logical rule...".
/**
* Describes a logical rule that is being used inside the app for triggering goals, matching audiences etc
*/
export type RuleObjectAudience = RuleObjectNoUrl;| /** | ||
| * File Object | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment "File Object" is not very descriptive. Expand it to explain what this object represents for better clarity.
/**
* Represents data for a file that has been uploaded.
*/| /** | ||
| * Select specific fields to be returned | ||
| */ | ||
| fields?: Array<(string)>; |
There was a problem hiding this comment.
For defining an array of strings, the string[] syntax is more idiomatic and concise in TypeScript than Array<(string)>. Use it here for better readability.
fields?: string[];| /** | ||
| * Select specific fields to be returned | ||
| */ | ||
| fields?: Array<(string)>; |
There was a problem hiding this comment.
For defining an array of strings, the string[] syntax is more idiomatic and concise in TypeScript than Array<(string)>.
fields?: string[];| /** | ||
| * Object consumed by SDKs | ||
| * | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment "Object consumed by SDKs" is a bit generic. Make it more specific to the response for better documentation.
/**
* Successful response containing the visitor data.
*/| /** | ||
| * Object consumed by SDKs | ||
| * | ||
| */ |
There was a problem hiding this comment.
The JSDoc comment "Object consumed by SDKs" is a bit generic. Make it more specific to the response for better documentation.
/**
* Successful response containing the visitor data.
*/
412c2b2 to
831216e
Compare
6bcba8b to
ec63810
Compare
ac2f7c8 to
c64384e
Compare
c64384e to
7791d1d
Compare
7791d1d to
2901299
Compare
8aca3d2 to
0f1d8ea
Compare
0f1d8ea to
c363a57
Compare
|
3 participants
Updating TS Serving API after the latest changes from backend repo,
PR #5294