-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat(events): Use new event dispatcher API #77
Conversation
const eventQueue = new FileBackedNamedEventQueue('events'); | ||
const emptyNetworkStatusListener = | ||
// eslint-disable-next-line @typescript-eslint/no-empty-function | ||
{ isOffline: () => false, onNetworkStatusChange: () => {} }; |
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 opted to not provide an implementation for NetworkStatusListener
since I didn't think it made sense for node server side. We could expose an optional property in IClientConfig
though, to allow users to provide their own implementation. thoughts?
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.
Makes sense to me to leave it out for now
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.
if the server is offline, then nothing else will work, so it feels like pointless to handle that, except in the client side
|
const emptyNetworkStatusListener = | ||
// eslint-disable-next-line @typescript-eslint/no-empty-function | ||
{ isOffline: () => false, onNetworkStatusChange: () => {} }; | ||
return newDefaultEventDispatcher(eventQueue, emptyNetworkStatusListener, sdkKey); |
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 chance it would help to wrap this with a try-catch as proposed here Eppo-exp/js-client-sdk#104
This also throws an exception, right?
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.
correct
* feat(events): Use new event dispatcher API * add tests for splice * fix tests * fix tests * Add test matrix
Towards: FF-3608
Towards: FF-3609
Motivation and Context
Hook into Eppo-exp/js-sdk-common#139 to use new event dispatcher API
See also Eppo-exp/js-client-sdk#103
Description
Integrate new event tracking API
How has this been tested?
Wrote tests