[FSSDK-11003] disposable service implementation #981
Conversation
| await expect(manager.onRunning()).resolves.not.toThrow(); | ||
| expect(repeater.stop).toHaveBeenCalled(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
can we also add tests that stops retrying to initialize after 5 failed attempts if disposable is true?
| return; | ||
| } | ||
|
|
||
| if (this.datafile) { | ||
| this.handleNewDatafile(this.datafile, true); | ||
| } | ||
|
|
||
| if(this.disposable) { |
There was a problem hiding this comment.
instead can we override the makeDisposable method like this:
makeDisposable() {
super.makeDisposable();
this.datafileManager?.makeDisposable();
}
There was a problem hiding this comment.
Yeah. Thanks for the suggestion!
| @@ -89,6 +89,10 @@ export class PollingDatafileManager extends BaseService implements DatafileManag | |||
| return; | |||
| } | |||
|
|
|||
| if(this.disposable) { | |||
There was a problem hiding this comment.
instead can we ovverride the makeDisposable method like this:
makeDisposable() {
super.makeDisposable();
this.initRetryRemaining = this.initRetryRemaining ?? 5;
}
lib/shared_types.ts
Outdated
| odpManager?: OdpManager; | ||
| notificationCenter: DefaultNotificationCenter; | ||
| vuidManager?: VuidManager | ||
| disposable?:boolean; |
lib/shared_types.ts
Outdated
| odpManager?: OdpManager; | ||
| vuidManager?: VuidManager; | ||
| disposable?:boolean; |
| expect(apiManager.sendEvents).toHaveBeenNthCalledWith(1, config, [event]); | ||
| expect(repeater.reset).toHaveBeenCalledTimes(1); | ||
| }) | ||
|
|
There was a problem hiding this comment.
can we also add tests for repeater stop when disposable = true?
There was a problem hiding this comment.
In the actual implementationrepeater.reset calls repeater.stop. But in the mock version of the repeater thats not the case. Whats your suggestion on that? Should I update the mock to support this ? (Not meaningful IMO).
There was a problem hiding this comment.
Oh I see we have assertions for repeater.reset here. We can ignore this comment
There was a problem hiding this comment.
we can add a test to assert the repeater is not started, similar to eventProcessor
| } | ||
|
|
||
| start(): void { | ||
| if (!this.isNew()) { | ||
| return; | ||
| } | ||
| if(this.disposable) { |
There was a problem hiding this comment.
can we override the makeDisposable method and do this inside that
lib/optimizely/index.spec.ts
Outdated
| @@ -39,12 +36,12 @@ describe('Optimizely', () => { | |||
|
|
|||
| const notificationCenter = createNotificationCenter({ logger, errorHandler }); | |||
|
|
|||
| it('should pass ssr to the project config manager', () => { | |||
| it('should pass disposable option to the project config manager', () => { | |||
There was a problem hiding this comment.
it should pass the disposable option to eventProcessor and odpManager as well, can we modify this test to assert those as well?
| this.retryConfig?.backoffProvider || | ||
| (() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)), | ||
| }; | ||
| } |
There was a problem hiding this comment.
on old line 245 below, we should check disposable before starting failedEventRepeater.
There was a problem hiding this comment.
So we will not start the "failedEventRepeater" if event processor is disposable?
| expect(eventDispatcher.dispatchEvent.mock.calls[0][0]).toEqual(buildLogEvent(events)); | ||
| expect(dispatchRepeater.reset).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
can we add tests for the following as well when disposable
- FailedEventRepeater is not started
- dispatchRepeater is not started
- maxRetry is limited
There was a problem hiding this comment.
Regarding your number 2 - Current logic is - dispatchRepeater starts, dispatch immediately and then stops. Isn't this expected ? Or am I missing something here ?
There was a problem hiding this comment.
according to the refactored logic in this PR, if batchSize == 1, dispatch repeater should never start. It will immediately dispatch the event, the repeater start is in the else branch
| this.failedEventRepeater?.start(); | ||
|
|
||
| if(!this.disposable) { | ||
| this.dispatchRepeater.start(); |
There was a problem hiding this comment.
we should not start the dispatchRepeater here. It will be started when an event is queued.
There was a problem hiding this comment.
Oh right! My bad! I guess , I am still wrapping my head around this service pattern. Adding something somewhere, forget to remove from somewhere. Appologies
junaed-optimizely
deleted the
junaed/fssdk-11003-make-services-disposable-for-ssr
branch
Summary
Making services disposable will allow the services to be disconnected from the repeater
Test plan
Test cases have been added
Issues
FSSDK-11003