-
Notifications
You must be signed in to change notification settings - Fork 1
chore: typeScript generation specification #317
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🔒 Scanned for secrets using gitleaks 8.28.0
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #317 +/- ##
==========================================
+ Coverage 45.50% 46.21% +0.71%
==========================================
Files 179 179
Lines 12881 12960 +79
==========================================
+ Hits 5862 5990 +128
+ Misses 6483 6432 -51
- Partials 536 538 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🔒 Scanned for secrets using gitleaks 8.28.0
|
docs/typescript-generation-spec.md
Outdated
| **Screen Event:** | ||
|
|
||
| ```typescript | ||
| export function screen( | ||
| name?: string, | ||
| properties?: ScreenProperties, | ||
| options?: ApiOptions, | ||
| callback?: apiCallback, | ||
| ): void { ... } | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Screen events are not supported by JS SDK. Instead, we support page events.
| * The analytics instance should be available via window.rudderanalytics. | ||
| * You can install it by following instructions at: https://www.rudderstack.com/docs/sources/event-streams/sdks/rudderstack-javascript-sdk/installation/ | ||
| */ | ||
| declare global { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if we can avoid global here and instead use the RudderAnalytics object exposed by the javascript SDK. That would probably require the Typer Analytics object to be defined with as a class with a constructor, and instantiated with new TyperAnalytics (or similar) after the new RudderAnalytics for the actual SDK.
What is the need for RudderAnalyticsPreloader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saikumarrs any idea what this RudderAnalyticsPreloader is about?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For CDN installations, the SDK loading snippet creates function stubs for tracking calls like track, page etc. where window.rudderanalytics just acts as a buffer for collecting all such function invocations until the SDK loads.
Once the SDK is loaded, it'll process all these buffered calls.
RudderAnalyticsPreloader represents the type of window.rudderanalytics when the SDK is not loaded yet.
docs/typescript-generation-spec.md
Outdated
|
|
||
| | Source | Target | Convention | Example | | ||
| | ------------------ | -------------- | ------------------ | --------------------------------------- | | ||
| | Event `name` | Function name | camelCase | `"Some Track Event"` → `someTrackEvent` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we stick to the Kotlin convention, where we start with the event type, followed by the event name in case of tracks. e.g:
identify(...)
trackSomeTrackEvent(...)
docs/typescript-generation-spec.md
Outdated
| | Source | Target | Convention | Example | | ||
| | ------------------ | -------------- | ------------------ | --------------------------------------- | | ||
| | Event `name` | Function name | camelCase | `"Some Track Event"` → `someTrackEvent` | | ||
| | Event `name` | Interface name | PascalCase | `"Some Track Event"` → `SomeTrackEvent` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For event rule types, let's also stick to the same convention as in Kotin. Interfaces start with the event type, followed by name in case of tracks, then followed by the identity section of that rule e.g:
IdentifyProperties
TrackSomeTrackEventProperties
docs/typescript-generation-spec.md
Outdated
| **Output (index.ts):** | ||
|
|
||
| ```typescript | ||
| export interface SomeTrackEvent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general guideline, for every property let's generate an interface or type for each property. e.g:
type PropertySomeString = string;
Then use that type in every place (rule of custom type) that property is used, e.g:
interface TrackSomeTrackEventProperties {
someString?: PropertySomeString;
}
docs/typescript-generation-spec.md
Outdated
| **Output (index.ts):** | ||
|
|
||
| ```typescript | ||
| export enum Somestringwithenum_String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically for enums, in order to avoid to S_ and N_ prefixes, can we considering listing the exact values in the type, e.g:
type Foo = 'GET' | 'POST' | '200' | 200;
docs/typescript-generation-spec.md
Outdated
| **Output (index.ts):** | ||
|
|
||
| ```typescript | ||
| export interface CustomTypeDefs { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's skip the wrapping CustomTypeDefs interface, as it doesn't represent any actual type. Let's instead generate interfaces (or types) with a CustomType prefix, e.g:
type CustomTypeSomeStringType = string;
the property using that would look like:
type PropertyCustomString = CustomTypeSomeStringType;
docs/typescript-generation-spec.md
Outdated
| **Output (index.ts):** | ||
|
|
||
| ```typescript | ||
| export interface SomeTrackEventSomeNestedLevel2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, following Kotlin conventions, let's create a single type for objects with nested properties, e.g:
interface TrackSomeTrackEventProperties { // this is the type for the entire event rule
...
someNestedObject: {
someNestedLevel1?: {
someNestedLevel2?: {
someString?: string;
someInteger?: number;
}
}
}
...
}
This avoids name conflicts for any nested object, without overly complicated interface names. It also makes the type definition local to the event rule type definition, which is what is expected.
docs/typescript-generation-spec.md
Outdated
|
|
||
| // Default case: any other someString value | ||
| export interface SomeVariantTypeDefault { | ||
| someString: string; // any string (catches all other values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a good suggestion out of the box, but it would be nice to see if we can define a type for the discriminator in the default case, where it accepts everything but the values defined in the cases.
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
🔒 Scanned for secrets using gitleaks 8.28.0
162cab1 to
83b1c54
Compare
🔒 Scanned for secrets using gitleaks 8.28.0
83b1c54 to
3425217
Compare
🔒 Scanned for secrets using gitleaks 8.28.0



🔒 Scanned for secrets using gitleaks 8.28.0