feat: Add public API for setting event dispatcher #139
Conversation
| // noinspection JSUnusedGlobalSymbols | ||
| setEventDispatcher(eventDispatcher: EventDispatcher) { | ||
| this.eventDispatcher = eventDispatcher; | ||
| } |
There was a problem hiding this comment.
i really don't love these mutable setter APIs we have in the EppoClient, as a fan of immutable data types, i'd much rather do it like that, but I'm following the existing pattern here
There was a problem hiding this comment.
Agreed, anything that won't toggle after instantion should be constructor options and immutable
There was a problem hiding this comment.
that would be a bigger refactoring, so maybe we can consider for a future major release
| // noinspection JSUnusedGlobalSymbols | ||
| setEventDispatcher(eventDispatcher: EventDispatcher) { | ||
| this.eventDispatcher = eventDispatcher; | ||
| } |
There was a problem hiding this comment.
Agreed, anything that won't toggle after instantion should be constructor options and immutable
| return this.queue.length === 0; | ||
| } | ||
|
|
||
| [Symbol.iterator](): IterableIterator<T> { |
There was a problem hiding this comment.
Your ruby is bleeding into typescript 😛
There was a problem hiding this comment.
lol this is actually part of the standard typescript API :) maybe they were inspired by ruby 🤣
| @@ -198,4 +201,26 @@ describe('DefaultEventDispatcher', () => { | |||
| expect(global.fetch).toHaveBeenCalled(); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('newDefaultEventDispatcher', () => { | |||
| if (!config.ingestionUrl) { | ||
| throw new Error('Missing required ingestionUrl in EventDispatcherConfig'); | ||
| } | ||
| if (!config.deliveryIntervalMs) { | ||
| throw new Error('Missing required deliveryIntervalMs in EventDispatcherConfig'); | ||
| } | ||
| if (!config.retryIntervalMs) { | ||
| throw new Error('Missing required retryIntervalMs in EventDispatcherConfig'); | ||
| } | ||
| if (!config.maxRetryDelayMs) { | ||
| throw new Error('Missing required maxRetryDelayMs in EventDispatcherConfig'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Minor, but this could probably be made into a loop to simplify.
|
thanks for the review 🙏🏽 |
Fixes: FF-3604
Motivation and Context
Description
This change accomplishes mainly 2 things:
EventDispatcherinto theEppoClientDefaultEventDispatcherinstance via factory functionHow has this been tested?
Wrote tests