-
Notifications
You must be signed in to change notification settings - Fork 8
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
create event interface #7
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,21 @@ | ||
export class EventType { | ||
public name: string; | ||
|
||
private constructor(name: string) { | ||
this.name = name; | ||
} | ||
|
||
public static OPENED_PUSH: EventType = new EventType('$opened_push'); | ||
public static CUSTOM = (name: string): EventType => new EventType(name); | ||
export enum EventType { | ||
OPENED_PUSH = '$opened_push', | ||
} | ||
|
||
export class EventProperty { | ||
public name: string; | ||
|
||
private constructor(name: string) { | ||
this.name = name; | ||
} | ||
|
||
public static EVENT_ID: EventProperty = new EventProperty('$event_id'); | ||
public static CUSTOM = (name: string): EventProperty => | ||
new EventProperty(name); | ||
export enum EventProperty { | ||
EVENT_ID = '$event_id', | ||
VALUE = 'value', | ||
} | ||
|
||
export interface IEvent { | ||
event: EventType; | ||
properties?: Object; | ||
type KlaviyoEventPropertyType = EventProperty | string; | ||
|
||
export interface KlaviyoEventAPI { | ||
readonly createEvent: ( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
name: EventType, | ||
properties?: Record<KlaviyoEventPropertyType, Object> | ||
) => void; | ||
readonly createCustomEvent: ( | ||
name: string, | ||
properties?: Record<KlaviyoEventPropertyType, Object> | ||
) => void; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
import type { TurboModule } from 'react-native'; | ||
import { TurboModuleRegistry } from 'react-native'; | ||
import type { IEvent } from './Event'; | ||
import type { KlaviyoEventAPI } from './Event'; | ||
|
||
export interface Spec extends TurboModule { | ||
createEvent(event: IEvent): void; | ||
} | ||
export interface KlaviyoSpec extends TurboModule, KlaviyoEventAPI {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This may cause a turbo modules issue. I was about to suggest we extract this interface to its own file but I happened on this first which explains some naming conventions involved in turbo modules code gen. It mentions the filename, but doesn't say whether this interface has to be called There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yea here we go https://dev.to/amazonappdev/a-guide-to-turbo-modules-in-react-native-5aa3
Maybe lets leave this file as-is from the code generator, and have a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i mean sure the naming of the file can be Spec if thats required but i dont see a point in defining another interface that mirrors this spec interface |
||
|
||
export default TurboModuleRegistry.getEnforcing<Spec>('KlaviyoReactNativeSdk'); | ||
export default TurboModuleRegistry.getEnforcing<KlaviyoSpec>( | ||
'KlaviyoReactNativeSdk' | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,13 @@ | ||
import type { IEvent } from './Event'; | ||
import { KlaviyoReactNativeSdk } from './KlaviyoReactNativeSdk'; | ||
import { EventType } from './Event'; | ||
import type { KlaviyoSpec } from './NativeKlaviyoReactNativeSdk'; | ||
|
||
interface IKlaviyo { | ||
readonly createEvent: (event: IEvent) => void; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll wind up with IKlaviyo again for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this can just implement Spec instead if thats the case, as long as its a single interface |
||
} | ||
|
||
export const Klaviyo: IKlaviyo = { | ||
createEvent(event: IEvent): void { | ||
KlaviyoReactNativeSdk.createEvent(event); | ||
export const Klaviyo: KlaviyoSpec = { | ||
createEvent(name: EventType, properties?: Record<any, Object>): void { | ||
this.createCustomEvent(name, properties); | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this need |
||
createCustomEvent(name: string, properties?: Record<any, Object>): void { | ||
KlaviyoReactNativeSdk.createEvent(name, properties); | ||
}, | ||
}; | ||
|
||
|
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 think we should keep a pinned version
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.
Yah or maybe a range of versions?