-
Notifications
You must be signed in to change notification settings - Fork 0
feat(events): Use new event dispatcher API #103
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
Conversation
package.json
Outdated
}, | ||
"dependencies": { | ||
"@eppo/js-client-sdk-common": "4.3.0" | ||
"@eppo/js-client-sdk-common": "https://github.com/Eppo-exp/js-sdk-common.git#0bd1de8d06426d91006cfda5070554001ff1184e" |
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.
this will eventually point to the next release version of js-client-sdk-common
once that's published
skipInitialPoll: skipInitialRequest, | ||
}; | ||
instance.setConfigurationRequestParameters(requestConfiguration); | ||
instance.setEventDispatcher(newEventDispatcher(apiKey)); |
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.
relevant change
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.
🚀
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.
Pieces coming together! 💪 💪
package.json
Outdated
}, | ||
"dependencies": { | ||
"@eppo/js-client-sdk-common": "4.3.0" | ||
"@eppo/js-client-sdk-common": "https://github.com/Eppo-exp/js-sdk-common.git#66aa69971c488927297e7aa611300d6dda0fd706" |
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.
Just a reminder not to check this in
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore test code | ||
// noinspection JSConstantReassignment | ||
navigator.onLine = true; |
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.
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.
🎉 despite the awful onLine
casing 🤮
applicationLogger.error( | ||
`Failed to parse event queue ${this.name} state. Initializing empty queue.`, | ||
); |
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 appreciate the fallback modes (vs exploding)
for (let i = 0; i < serializedEvent.length; i++) { | ||
hash = (hash << 5) - hash + serializedEvent.charCodeAt(i); | ||
hash |= 0; // Convert to 32bit integer | ||
} |
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.
Any reason we're doing this ourselves vs some hashing library? Or reusing the hashing code we do for shard numbers?
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.
good point, actually no reason. I'll look into reusing that
* Configuration used for initializing the Eppo client | ||
* @public | ||
*/ | ||
export interface IClientConfig { |
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.
Thanks for pulling this out into it's own file, much cleaner 🧼
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.
🧹
skipInitialPoll: skipInitialRequest, | ||
}; | ||
instance.setConfigurationRequestParameters(requestConfiguration); | ||
instance.setEventDispatcher(newEventDispatcher(apiKey)); |
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.
🚀
@@ -0,0 +1,11 @@ | |||
/* Returns elements from arr until the predicate returns false. */ | |||
export function takeWhile<T>(arr: T[], predicate: (item: T) => boolean): T[] { |
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.
oh interesting utility method
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.
poor man's lodash
}, | ||
"dependencies": { | ||
"@eppo/js-client-sdk-common": "4.3.0" | ||
"@eppo/js-client-sdk-common": "^4.5.0" |
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.
@sameerank fyi
Towards: FF-3608
Towards: FF-3609
Motivation and Context
Hook into Eppo-exp/js-sdk-common#139 to use new event dispatcher API
Description
Integrate new event tracking API
How has this been tested?
Wrote tests