Default uninitialized EppoClient instance #48

aarsilv · 2024-02-03T00:18:26Z

🎟️ (Eppo Internal) FF-1557 - JS Client SDK should return null when not initialized
👯 Related PR: js-client-sdk #37

#45 introduced a change of behavior, where calls to getInstance() before the client was initialized would throw an error instead of returning an uninitialized client. This Pull request reverts that behavior change so that there will always be a client, and if uninitialized, it will just return null on any assignment calls.

This is done by instantiating a client statically at import time--as we were previously doing (link)--as opposed to during init().

So that we can still leverage the client handling polling for configuration requests, we use EppoClients newly-added (see the related PR)setConfigurationRequestParameters() where we can pass in the configuration request parameters during init() instead of needing the when constructing the instance.

aarsilv · 2024-02-03T00:20:44Z

docs/js-client-sdk.eppojsclient.initialized.md

+**Signature:**
+
+```typescript
+static initialized: boolean;


Docs got updated because the way index.ts is structured, initialized has to be public 🙄

aarsilv · 2024-02-03T00:24:34Z

docs/js-client-sdk.iclientconfig.throwonfailedinitialization.md

@@ -4,7 +4,7 @@

 ## IClientConfig.throwOnFailedInitialization property

-Throw on error if unable to fetch an initial configuration during initialization. (default: true)
+Throw an error if unable to fetch an initial configuration during initialization. (default: true)


🧹 Grammar! 📈

aarsilv · 2024-02-03T00:25:03Z

package.json

@@ -58,7 +58,7 @@
    "xhr-mock": "^2.5.1"
  },
  "dependencies": {
-    "@eppo/js-client-sdk-common": "2.1.0",
+    "@eppo/js-client-sdk-common": "file:../js-client-sdk-common",


This will be replaced with "2.1.1" once published

aarsilv · 2024-02-03T00:26:15Z

src/index.spec.ts

@@ -379,7 +382,7 @@ describe('EppoJSClient E2E test', () => {
      flags: {
        [hashedFlagKey]: mockExperimentConfig,
      },
-    };
+    } as unknown as Record<string, IExperimentConfiguration>;


TypeScript insisted on the double as because we are wiling out with our types here.

Ah yes, I've seen typescript do this to me before

aarsilv · 2024-02-03T00:29:08Z

src/index.spec.ts

+        jest.isolateModules(() => {
+          // Isolate and re-require so that the static instance is reset to it's default state
+          // eslint-disable-next-line @typescript-eslint/no-var-requires
+          const reloadedModule = require('./index');
+          init = reloadedModule.init;
+          getInstance = reloadedModule.getInstance;
+        });


This isolateModules() and require() combo to reset the static EppoJSClient instance took me a bit to figure out and it had weird side effects, such as making test double (td) not work (hence me using returnRac() instead of mocking a response like other tests cases). Also, using this outside of a beforeEach()--which I did at first because I just have one test case--results in console.log() not actually logging anything 🤯

TL;DR; use singletons and jest.isolateModules() with extreme caution, but it is a potentially useful tool to have your toolbox.

aarsilv · 2024-02-03T00:30:11Z

src/index.ts

@@ -69,12 +69,15 @@ export interface IClientConfig {

 export { IAssignmentLogger, IAssignmentEvent, IEppoClient } from '@eppo/js-client-sdk-common';

+const localStorage = new EppoLocalStorage();


Moved this back to the module-level (where it was) so it can be used in the static initialization of EppoJSClient.instance

aarsilv · 2024-02-03T00:30:45Z

src/index.ts

@@ -83,6 +86,7 @@ export class EppoJSClient extends EppoClient {
    subjectAttributes?: Record<string, any>,
    assignmentHooks?: IAssignmentHooks,
  ): string | null {
+    EppoJSClient.getAssignmentInitializationCheck();


I think we do want to warn people who request assignments on uninitialized instances.

True, we can mute it in the future if requested. Currently the greater issue seems to be confusion about unexpected null returns

aarsilv · 2024-02-03T00:31:19Z

src/index.ts

-  if (!EppoJSClient.instance) {
-    throw Error('init() must first be called to initialize a client instance');
-  }


Farewell...for now... 😢

I like the intention of alerting if the SDK is functioning in an unintended way, but I won't miss the risk of throwing exceptions in client code

sameerank · 2024-02-03T00:46:18Z

src/index.spec.ts

@@ -379,7 +382,7 @@ describe('EppoJSClient E2E test', () => {
      flags: {
        [hashedFlagKey]: mockExperimentConfig,
      },
-    };
+    } as unknown as Record<string, IExperimentConfiguration>;


Ah yes, I've seen typescript do this to me before

sameerank · 2024-02-03T00:48:54Z

src/index.spec.ts

+      let getInstance: Function;
+      beforeEach(() => {
+        jest.isolateModules(() => {
+          // Isolate and re-require so that the static instance is reset to it's default state


Thanks for the comment

sameerank · 2024-02-03T00:50:57Z

src/index.ts

@@ -83,6 +86,7 @@ export class EppoJSClient extends EppoClient {
    subjectAttributes?: Record<string, any>,
    assignmentHooks?: IAssignmentHooks,
  ): string | null {
+    EppoJSClient.getAssignmentInitializationCheck();


True, we can mute it in the future if requested. Currently the greater issue seems to be confusion about unexpected null returns

sameerank · 2024-02-03T00:56:09Z

src/index.ts

-  if (!EppoJSClient.instance) {
-    throw Error('init() must first be called to initialize a client instance');
-  }


I like the intention of alerting if the SDK is functioning in an unintended way, but I won't miss the risk of throwing exceptions in client code

aarsilv added 2 commits February 2, 2024 16:44

initialize instance off the bat; set request configuration later

6b20a9e

make initialize flag public

992a1d9

aarsilv mentioned this pull request Feb 3, 2024

Ability to set configuration request parameters on an already instantiated client instance Eppo-exp/js-sdk-common#37

Merged

aarsilv added 2 commits February 2, 2024 17:22

correct docs

21a7cc3

more doc fixes

c3a657f

aarsilv commented Feb 3, 2024

View reviewed changes

aarsilv requested review from leoromanovsky and sameerank February 3, 2024 00:31

sameerank approved these changes Feb 3, 2024

View reviewed changes

use newly published common sdk

25bea5f

aarsilv merged commit e842de5 into main Feb 5, 2024
2 checks passed

aarsilv deleted the aaron/ff-1557/optional-dummy-initial branch February 5, 2024 16:13

aarsilv mentioned this pull request Feb 5, 2024

Bumping patch version #49

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default uninitialized EppoClient instance #48

Default uninitialized EppoClient instance #48

aarsilv commented Feb 3, 2024

aarsilv Feb 3, 2024

aarsilv Feb 3, 2024

aarsilv Feb 3, 2024

aarsilv Feb 3, 2024

sameerank Feb 3, 2024

aarsilv Feb 3, 2024

aarsilv Feb 3, 2024

aarsilv Feb 3, 2024

sameerank Feb 3, 2024

aarsilv Feb 3, 2024

sameerank Feb 3, 2024

sameerank Feb 3, 2024

sameerank Feb 3, 2024

sameerank Feb 3, 2024

sameerank Feb 3, 2024

		@@ -69,12 +69,15 @@ export interface IClientConfig {

		export { IAssignmentLogger, IAssignmentEvent, IEppoClient } from '@eppo/js-client-sdk-common';

		const localStorage = new EppoLocalStorage();

Default uninitialized EppoClient instance #48

Default uninitialized EppoClient instance #48

Conversation

aarsilv commented Feb 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment