[FSSDK-11003] disposable service implementation by junaed-optimizely · Pull Request #981 · optimizely/javascript-sdk

junaed-optimizely · 2025-01-07T17:28:58Z

Summary

Making services disposable will allow the services to be disconnected from the repeater

Test plan

Test cases have been added

Issues

coveralls · 2025-01-07T17:32:34Z

coverage: 83.928% (-0.3%) from 84.24%
when pulling f2c3b5a on junaed/fssdk-11003-make-services-disposable-for-ssr
into 51e8c1a on master.

raju-opti

added few comments

raju-opti · 2025-01-09T14:18:50Z

lib/project_config/polling_datafile_manager.spec.ts

+      await expect(manager.onRunning()).resolves.not.toThrow();
+      expect(repeater.stop).toHaveBeenCalled();
+    });
+


can we also add tests that stops retrying to initialize after 5 failed attempts if disposable is true?

raju-opti · 2025-01-09T14:23:29Z

lib/project_config/project_config_manager.ts

      this.handleNewDatafile(this.datafile, true);
    }

+    if(this.disposable) {


instead can we override the makeDisposable method like this:

makeDisposable() { super.makeDisposable(); this.datafileManager?.makeDisposable(); }

Yeah. Thanks for the suggestion!

raju-opti · 2025-01-09T14:27:16Z

lib/project_config/polling_datafile_manager.ts

      return;
    }

+    if(this.disposable) {


instead can we ovverride the makeDisposable method like this:

makeDisposable() { super.makeDisposable(); this.initRetryRemaining = this.initRetryRemaining ?? 5; }

raju-opti · 2025-01-09T14:34:52Z

lib/shared_types.ts

  odpManager?: OdpManager;
  notificationCenter: DefaultNotificationCenter;
  vuidManager?: VuidManager
+  disposable?:boolean;


NIT: space after ?:

raju-opti · 2025-01-09T14:35:33Z

lib/shared_types.ts

-  isSsr?: boolean;
  odpManager?: OdpManager;
  vuidManager?: VuidManager;
+  disposable?:boolean;


similar to above

raju-opti · 2025-01-09T14:44:33Z

lib/odp/event_manager/odp_event_manager.spec.ts

+    expect(apiManager.sendEvents).toHaveBeenNthCalledWith(1, config, [event]);
+    expect(repeater.reset).toHaveBeenCalledTimes(1);
+  })
+


can we also add tests for repeater stop when disposable = true?

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).

Oh I see we have assertions for repeater.reset here. We can ignore this comment

we can add a test to assert the repeater is not started, similar to eventProcessor

raju-opti · 2025-01-09T14:54:29Z

lib/event_processor/batch_event_processor.ts

    if (!this.isNew()) {
      return;
    }
+    if(this.disposable) {


can we override the makeDisposable method and do this inside that

raju-opti · 2025-01-09T15:02:36Z

lib/optimizely/index.spec.ts

  const notificationCenter = createNotificationCenter({ logger, errorHandler });

-  it('should pass ssr to the project config manager', () => {
+  it('should pass disposable option to the project config manager', () => {


it should pass the disposable option to eventProcessor and odpManager as well, can we modify this test to assert those as well?

raju-opti · 2025-01-09T15:05:04Z

lib/event_processor/batch_event_processor.ts

+          this.retryConfig?.backoffProvider ||
+          (() => new ExponentialBackoff(DEFAULT_MIN_BACKOFF, DEFAULT_MAX_BACKOFF, 500)),
+      };
+    }


on old line 245 below, we should check disposable before starting failedEventRepeater.

So we will not start the "failedEventRepeater" if event processor is disposable?

raju-opti · 2025-01-09T15:07:10Z

lib/event_processor/batch_event_processor.spec.ts

+      expect(eventDispatcher.dispatchEvent.mock.calls[0][0]).toEqual(buildLogEvent(events));
+      expect(dispatchRepeater.reset).toHaveBeenCalledTimes(1);
+    });
+


can we add tests for the following as well when disposable

FailedEventRepeater is not started

dispatchRepeater is not started

maxRetry is limited

Regarding your number 2 - Current logic is - dispatchRepeater starts, dispatch immediately and then stops. Isn't this expected ? Or am I missing something here ?

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

raju-opti

one small comment

raju-opti · 2025-01-13T14:01:44Z

lib/event_processor/batch_event_processor.ts

-    this.failedEventRepeater?.start();
+
+    if(!this.disposable) {
+      this.dispatchRepeater.start();


we should not start the dispatchRepeater here. It will be started when an event is queued.

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

raju-opti

LGTM

[FSSDK-11003] disposable project config manager

060cbc3

optimizely-codereview-bot assigned junaed-optimizely Jan 7, 2025

junaed-optimizely added 2 commits January 9, 2025 01:44

[FSSDK-11003] disposable odp manager

11c850d

[FSSDK-11003] disposable batch event processor

17d27a3

junaed-optimizely marked this pull request as ready for review January 9, 2025 12:55

junaed-optimizely requested a review from a team as a code owner January 9, 2025 12:55

junaed-optimizely requested a review from raju-opti January 9, 2025 12:55

[FSSDK-11003] polling datafile manager retry update

e516e39

raju-opti requested changes Jan 9, 2025

View reviewed changes

[FSSDK-11003] review update

08d0df4

junaed-optimizely requested a review from raju-opti January 9, 2025 19:07

raju-opti requested changes Jan 13, 2025

View reviewed changes

[FSSDK-11003] review update batch event processor

bad40b4

raju-opti approved these changes Jan 13, 2025

View reviewed changes

[FSSDK-11003] new test addition for event processor

f2c3b5a

junaed-optimizely merged commit 92ab2a7 into master Jan 13, 2025
11 of 18 checks passed

junaed-optimizely deleted the junaed/fssdk-11003-make-services-disposable-for-ssr branch January 13, 2025 15:28

Conversation

junaed-optimizely commented Jan 7, 2025

Summary

Test plan

Issues

Uh oh!

coveralls commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

raju-opti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raju-opti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

junaed-optimizely Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raju-opti left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Jan 7, 2025 •

edited

Loading

junaed-optimizely Jan 13, 2025 •

edited

Loading