feat: non-singleton EppoJSClient #166

typotter · 2025-01-31T16:40:41Z

labels: mergeable

Eppo Internal
🎟️ Fixes: FF-3888
📜 Multiple Eppo Clients
Configuration Side-loading

Motivation and Context

The init and getInstance() methods work on a singleton instance of the EppoClient which makes for a generally smooth developer experience given that the Eppo SDK essentially handles dependency management for the dev. This does have its own drawbacks, but those are not particularly relevant here.

There are use cases, however, when a non-singleton instance of the EppoClient is required. One such use case is embedding the Eppo SDK into a library which itself can be included in other applications. If that 3p library shipped with Eppo is integrated into another application that has the Eppo SDK running, there would be undefined behaviour as the SDK cannot handle this use case.

The init method (and class constructors) for EppoClient and subclasses have evolved organically over time, adding new options as new features are added to the clients. The very large options type is beginning to become a little untenable and disorganized so we take the opportunity to clean that up a bit here.

There are other limitations and drawbacks to the current model of instantiating an EppoClient statically and then initializing it when the code calls init including the awkward coupling of needing to wait for init to resolve in order to get a reference to an initialized client. We have an opportunity to decouple initialization and waiting to make for a better DX (in addition to giving the dev full control over managing the EppoClient reference (intrinsically allows for easier mocking in tests and will be consistent with the host applications existing DI approach).

** This change must be done without a major version bump and completely preserve the existing singleton API**

Description

First, refactor the initialization options by grouping related options and extracting different types for each.
Each option type is then combined back into a type using the existing IClientConfig name (this keeps the change backwards compatible while offering a big win in option clarity)
Move the static reference initialized to non static.
Accept an EppoJSClient instance param instead of accessing though EppoJSClient.instance where applicable
Where the static instance must be accessed, change to getInstance
Allow the EppoJSClient constructor to accept the full options object. When full options are passed, kick off initializing the client. Otherwise, follow the "old" contstructor path
Docs

How has this been documented?

https://linear.app/eppo/issue/FF-3926/[docs]-update-js-client-docs-to-describe-the-non-singleton-client

How has this been tested?

tests

package.json

src/i-client-config.ts

typotter · 2025-01-31T19:39:56Z

src/index.ts

  public static instance = new EppoJSClient({
    flagConfigurationStore,
    isObfuscated: true,
  });
-  public static initialized = false;
+  initialized = false;


move static members to the instance

typotter · 2025-01-31T19:41:48Z

src/index.ts

+    }
+  }
+
+  public waitForReady(): Promise<void> {


new method! 🎉

what does "ready" mean in this context? adding comments is likely to help, also we should always do it for public APIs anyways

renamed to waitForInitialization

typotter · 2025-01-31T19:42:05Z

src/index.ts


  public getStringAssignment(
    flagKey: string,
    subjectKey: string,
    subjectAttributes: Record<string, AttributeType>,
    defaultValue: string,
  ): string {
-    EppoJSClient.ensureInitialized();
+    this.ensureInitialized();


moving static methods and fields to the instance

typotter · 2025-01-31T19:42:41Z

src/index.ts

-    if (!EppoJSClient.initialized) {
+  private ensureInitialized() {
+    if (!this.initialized) {
+      // TODO: check super.isInitialized?


EppoClient.initialized also exists, but means something different than it does here.

what does it mean and how is that different than this? consider renaming either if so

the super method checks that all the config stores are initialized while this this.initialized is true when all the initialization workload is done so they are pretty much the same.
I think we can move to deprecate EppoJSClient.initialized and defer to super.isInitialized().

typotter · 2025-01-31T19:43:39Z

src/index.ts

+  /**
+   * Tracks pending initialization. After an initialization completes, the value is removed from the map.
+   */
+  private static initializationPromise: Promise<EppoJSClient> | null = null;
+
+  /**
+   * This method is part of a bridge from using a singleton to independent instances. More specifically, it exists so
+   * that the init method can access the private field, `readyResolver`. It should not be called by any
+   * methods other than the `init` method. There are limited guards in place; the behaviour if called inappropriately
+   * is undefined.
+   *
+   * It also keeps code that relies on internal details of EppoJSClient colocated in the class.
+   *
+   * @internal
+   *
+   * @param client
+   * @param config
+   */
+  static async initializeClient(
+    client: EppoJSClient,
+    config: IClientConfig,
+  ): Promise<EppoJSClient> {
+    validation.validateNotBlank(config.apiKey, 'API key required');
+
+    // If there is already an init in progress for this apiKey, return that.
+    if (!EppoJSClient.initializationPromise) {
+      EppoJSClient.initializationPromise = explicitInit(config, client).then((client) => {
+        // Resolve the ready promise if it exists
+        if (client.readyPromiseResolver) {
+          client.readyPromiseResolver();
+          client.readyPromiseResolver = null;
+        }
+        return client;
+      });
+    }
+
+    const readyClient = await EppoJSClient.initializationPromise;
+    EppoJSClient.initializationPromise = null;
+
+    return readyClient;
+  }
 }


This is the old init method and initialization buffer tracker.
Moved the method to a static member of EppoJSClient so that it can access the readyResolver and notify waitForReady

src/index.ts

typotter · 2025-01-31T19:46:45Z

src/index.ts

 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).
+ *
+ *
+ * @deprecated


controversial, I know.
willing to negotiate

Seems like a minor change for the end user if the argument interface stays identical

The @deprecated annotation is here to help push devs to using the new construct + wait paradigm. This method will be removed and the argument interface for the new constructor is slightly different sdkKey vs apiKey

typotter · 2025-01-31T19:46:53Z

src/index.ts

 * @param config - client configuration
 * @public
 */
 export async function init(config: IClientConfig): Promise<EppoClient> {
-  validation.validateNotBlank(config.apiKey, 'API key required');


moved to EppoJSClient

src/index.ts

rasendubi · 2025-02-03T14:46:00Z

src/index.ts

+// For use only with the singleton instance.
 const flagConfigurationStore = configurationStorageFactory({


minor: would it make sense to nest this under singleton initialization method, so it's not reused accidentally. As another option, we could also rename it to something like singletonFlagConfigurationStore

More specifically, it's used as a stand-in singleton until initialization is complete

moved this FCS to anonymous creation in the singleton constructor call; access to this variable is not needed outside of the eppo client.

rasendubi · 2025-02-03T14:51:46Z

src/index.ts

-  public static initialized = false;
+  initialized = false;
+
+  constructor(optionsOrConfig: EppoClientParameters | IClientOptions) {


major: why do we need to support two versions of options in the constructor? Given we make constructor public, this means that both options can be used by users (which I don't think we want)

The constructor is already public, so we needed to keep the EppoClientParameters in the constructor to avoid a major change.

That being said, I moved to a static builder method instead that lets us get away from the dual constructor.

May be moot based on this comment, but I think you could have a backwards-compatible type as well (e.g., one that is a union that allows for either apiKey or sdkKey

The constructor is already public, so we needed to keep the EppoClientParameters in the constructor to avoid a major change.

My understanding is that constructor is only technically public (similar to how we have many internal methods public) and nobody is calling it directly. At the very least, because parameters are unsafe to be set by users (e.g., isObfuscated or sdkVersion are not meant to be set by users). So I don't think we need to keep it backward-compatible and/or keep supporting old/unsafe options

rasendubi · 2025-02-03T14:53:52Z

src/index.ts

+              ? newEventDispatcher(optionsOrConfig.sdkKey, optionsOrConfig.eventIngestionConfig)
+              : undefined,
+          ) // "New" construction technique; isolated instance.
+        : (optionsOrConfig as EppoClientParameters), // Legacy instantiation of singleton.


minor: I think if we inline 'sdkKey' in optionsOrConfig in the ternary condition here, typescript should be able to figure out the correct type of optionsOrConfig in both branches

obviated by other changes.

rasendubi · 2025-02-03T14:57:02Z

src/index.ts

+      ).then(() => {
+        return;
+      });


minor: this then call looks like a no-op. As far as I figured, it's just discarding the result of explicitInit, so it would help adding this explanation as a comment (so people don't have to dig into the code to figure it out)

Thanks for the note. Refactored around this anyway

rasendubi · 2025-02-03T15:06:45Z

src/index.ts

 }

-async function explicitInit(config: IClientConfig): Promise<EppoClient> {
+async function explicitInit(config: IClientConfig, instance: EppoJSClient): Promise<EppoJSClient> {


minor: wonder if we can make it an instance method now that it accepts an instance?

Wondering the same. Maybe it could just live in export class EppoJSClient extends EppoClient {?

i do believe we can! good catch.

rasendubi · 2025-02-03T15:11:10Z

src/index.ts

+  ): Promise<EppoJSClient> {
+    validation.validateNotBlank(config.apiKey, 'API key required');
+
+    // If there is already an init in progress for this apiKey, return that.


The code below does not actually check that initialization is unique per api key

good catch.

rasendubi · 2025-02-03T15:13:05Z

src/index.ts

+      EppoJSClient.initializationPromise = explicitInit(config, client).then((client) => {
+        // Resolve the ready promise if it exists
+        if (client.readyPromiseResolver) {
+          client.readyPromiseResolver();
+          client.readyPromiseResolver = null;
+        }
+        return client;
+      });


minor: now that explicitInit accepts a client as a parameter, would it make sense to make it a class member? it could call readyPromiseResolver, so we wouldn't need this hacky initializeClient.

I also think it was cleaner when initializationPromise stayed closer to global init()—initializeClient() was hard to follow because it takes a client instance but also accesses static fields (having that in init() was cleaner because init() knows that client instance stands for the singleton.)

Thanks for the perspective; switched to a builder method and really cleaned everything up

rasendubi · 2025-02-03T15:24:07Z

src/index.ts

 }

-async function explicitInit(config: IClientConfig): Promise<EppoClient> {
+async function explicitInit(config: IClientConfig, instance: EppoJSClient): Promise<EppoJSClient> {


minor: maybe I'm asking too much from this PR, but have you considered pushing more of this initialization stuff into common SDK? I imagine that initialization sequence is more or less the same between client/node/native, only differing in default parameters.

Singleton handling is also a good candidate for sharing.

rasendubi · 2025-02-03T15:33:52Z

src/index.ts

- * Tracks pending initialization. After an initialization completes, the value is removed from the map.
- */
-let initializationPromise: Promise<EppoClient> | null = null;
-
 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).


unrelated to the PR but I just noticed that the field is called forceReinitialize

Suggested change

* initialization routine (if `forceReinitialization` is `true`).

* initialization routine (if `forceReinitialize` is `true`).

rasendubi · 2025-02-03T15:40:42Z

src/i-client-config.ts

-   * than the default storage provided by the SDK.
-   */
-  persistentStore?: IAsyncStore<Flag>;
+  // TODO: Add initial config (stringified IConfigurationWire) here.


minor: it's better to add a proper Configuration class instead of passing untyped/unvalidated strings

We have the client returning a stringified IConfigurationWire so devs don't need to worry about serializing the config string.

In my ideal world, we have a Configuration class with toString() and fromString() methods. This way, devs still don't need to worry about serialization but they have a stronger hint at what they are expected to pass in

rasendubi · 2025-02-03T15:41:30Z

src/i-client-config.ts

+export type IClientOptions = IApiOptions &
+  ILoggers &
+  IEventOptions &
+  IStorageOptions &
+  IPollingOptions;


Are these sub-types ever used separately? Having the type split like this increases the chance of getting name clashes on the fields (which typescript won't really warn us about)

The idea is that a dev could build different options objects then combine them for initialization. If they don't need custom event and storage options, for example, they wouldn't have those configs.

Could you give an example of how you see it used in users' code?

rasendubi · 2025-02-03T15:41:45Z

src/i-client-config.ts


-  /** Configuration settings for the event dispatcher */


This doc string is now missing

sameerank

Don't forget to address the "defined but never used" warnings! Oleksii raised good points so please address those. Otherwise lgtm

src/index.spec.ts

sameerank · 2025-02-03T15:41:55Z

src/index.spec.ts

+
+        // deep copy the mock data since we're going to inject a change below.
+        const flagConfig: Flag = JSON.parse(JSON.stringify(mockObfuscatedUfcFlagConfig));
+        // Inject the encoded variant as a single split for the flag's only allocation.


Thanks! Helps to know that this is the flag's only allocation

sameerank · 2025-02-03T15:55:31Z

src/index.ts

 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).
+ *
+ *
+ * @deprecated


Seems like a minor change for the end user if the argument interface stays identical

src/client-options-converter.ts

sameerank · 2025-02-03T16:08:24Z

src/index.ts

 }

-async function explicitInit(config: IClientConfig): Promise<EppoClient> {
+async function explicitInit(config: IClientConfig, instance: EppoJSClient): Promise<EppoJSClient> {


Wondering the same. Maybe it could just live in export class EppoJSClient extends EppoClient {?

felipecsl

I like this directionally but doesn't seem like we're quite there yet. I might need to pull this branch locally to review more in depth once some of the current round of feedback is addressed

felipecsl · 2025-02-03T16:39:55Z

src/index.ts

+    }
+  }
+
+  public waitForReady(): Promise<void> {


what does "ready" mean in this context? adding comments is likely to help, also we should always do it for public APIs anyways

felipecsl · 2025-02-03T16:40:36Z

src/index.ts

-    if (!EppoJSClient.initialized) {
+  private ensureInitialized() {
+    if (!this.initialized) {
+      // TODO: check super.isInitialized?


what does it mean and how is that different than this? consider renaming either if so

felipecsl · 2025-02-03T16:40:53Z

src/index.ts

-  // Note that we use the first 8 characters of the API key to create per-API key persistent storages and caches
-  return apiKey.replace(/\W/g, '').substring(0, 8);
+  /**
+   * Tracks pending initialization. After an initialization completes, the value is removed from the map.


aarsilv

Thanks for getting after this! I left a bunch of comments and have some higher-level musings as well. You're much closer to this problem than I am so much of what I say may not make sense 😅

Agree with @rasendubi that I'm thinking much of this could actually be pushed down to the common package.
I'm having trouble seeing the win of having two sets of initialization parameters
I don't think we want people actually newing up the client--one of the reasons we did the singleton pattern was to avoid people shooting themselves in the foot. Plus constructors that do "work" make testing hard and whatnot
We seem to be putting the responsibility of keeping track of the client on the user. What if we basically kept most things the same, but had an optional "namespace" parameter for initialization and we stored the map (as well as returned the instance on init).

Normal flow:

await init(config);
getInstance().getBooleanAssignment(...); // true

"parallel flow" cartoon example:

await init(config);
await init({...config, namespace: 'another'});
getInstance().getBooleanAssignment(...); // true
getInstance('another').getBooleanAssignment(...); // false

Then implementing customers don't need to pass an instance everywhere, just a string namespace, which could be some shared constant or something.

But if they want to pass an instance around, they of course always could with the return value:

const defaultSingleton = await init(config);
const anotherSingleton = await init({...config, namespace: 'another'});
defaultSingleton.getBooleanAssignment(...); // true
anotherSingleton.getBooleanAssignment(...); // false

The stand-in client could be returned for any getInstance() without a ready instance so things don't explode if something goes wrong.

aarsilv · 2025-02-04T05:10:13Z

src/client-options-converter.ts

+  // Always include configuration request parameters
+  parameters.configurationRequestParameters = {
+    apiKey: options.sdkKey,
+    sdkVersion, // dynamically picks up version.
+    sdkName, // Hardcoded to `js-client-sdk`
+    baseUrl: options.baseUrl,
+    requestTimeoutMs: options.requestTimeoutMs,
+    numInitialRequestRetries: options.numInitialRequestRetries,
+    numPollRequestRetries: options.numPollRequestRetries,
+    pollAfterSuccessfulInitialization: options.pollAfterSuccessfulInitialization,
+    pollAfterFailedInitialization: options.pollAfterFailedInitialization,
+    pollingIntervalMs: options.pollingIntervalMs,
+    throwOnFailedInitialization: options.throwOnFailedInitialization,
+    skipInitialPoll: options.skipInitialRequest,
+  };


This seems like it could e a pain to keep in sync. If these parameters are just a supserset, could we use the spread operator instead?

const parameters = {...options, eventDispatcher, isObfuscated: true}

Side note: I've heard a couple of places they'd like to be able to tell client not to obfuscate, such as in development environments. So I wonder if we take this opportunity to make isObfuscated configurable.

isObfuscated is actually a booby trap.

The client doesn't obfuscate anything (it only de-obfuscates), except a couple of log fields(?). Whether configuration is obfuscated or not is determined by the server based on sdkName/sdkVersion. isObfuscated is a flag that says "we know that server will serve obfuscated config for my sdkName/sdkVersion pair." You can't change isObfuscated without things blowing up.

isObfuscated should actually be removed altogether because we have format and obfuscated fields in configurations which determine whether configuration is really obfuscated.

aarsilv · 2025-02-04T05:11:56Z

src/i-client-config.ts

+  persistentStore?: IAsyncStore<Flag>;
+};
+
+export type IPollingOptions = {


oooh I like this additional organization

aarsilv · 2025-02-04T05:12:55Z

src/i-client-config.ts

+ * Configuration for regular client initialization
+ * @public
+ */
+export type IClientConfig = Omit<IClientOptions, 'sdkKey' | 'offline'> &


I think the omit/pick makes it clear what is happening

These types are so similar is it worth having separate ones for the constructor vs. init? What if we just allow sdkKey or apiKey for backward compatibility? I worry having two constructor input types could be confusing

I would actually keep apiKey and save sdkKey rename for the next major bump

fixed up. moved sdkKey to the next major

aarsilv · 2025-02-04T05:15:50Z

src/index.spec.ts

+    it('should be independent of the singleton', async () => {
+      const apiOptions: IApiOptions = { sdkKey: '<MY SDK KEY>' };
+      const options: IClientOptions = { ...apiOptions, assignmentLogger: mockLogger };
+      const isolatedClient = new EppoJSClient(options);


do we want people calling constructors or should we have some method that does this? My spidey senses tell me the later, but that may be left over bias from the Java community's war on constructors a decade ago

The main reason to prefer builder methods is that they make it easier to maintain backward compatibility. If constructor is private, we can fundamentally change how construction works without breaking API. We can also add builder methods for common setups and give them meaningful names

moved to a builder

aarsilv · 2025-02-04T05:17:11Z

src/index.spec.ts

+        // Get the value of the apiKey parameter and serve a specific variant.
+        const apiKey = urlParams.get('apiKey');
+
+        // differentiate between the SDK keys by changing the variant that `flagKey` assigns.


aarsilv · 2025-02-04T05:23:55Z

src/index.ts

+// For use only with the singleton instance.
 const flagConfigurationStore = configurationStorageFactory({


More specifically, it's used as a stand-in singleton until initialization is complete

aarsilv · 2025-02-04T05:24:25Z

src/index.ts

-  public static initialized = false;
+  initialized = false;
+
+  constructor(optionsOrConfig: EppoClientParameters | IClientOptions) {


aarsilv · 2025-02-04T05:26:46Z

src/index.ts

+        this.readyPromiseResolver = resolve;
+      });
+    } else {
+      this.readyPromise = explicitInit(


what happens if an error is hit? I think we may need outer try-catch here to respect our throw on failed initialization flag. And if that is false (which we hope to make the new default) we'll probably want to return a stand-in client similar to what the singleton has.

init handles catching exceptions on init and throwing/logging depending on the option

aarsilv · 2025-02-04T05:29:26Z

src/index.ts

 }

-async function explicitInit(config: IClientConfig): Promise<EppoClient> {
+async function explicitInit(config: IClientConfig, instance: EppoJSClient): Promise<EppoJSClient> {


aarsilv · 2025-02-04T05:31:18Z

src/i-client-config.ts

@@ -165,4 +189,77 @@ export interface IClientConfig extends IBaseRequestConfig {
     */
    maxQueueSize?: number;
  };
+};
+
+export type IStorageOptions = {


I think storage uniqueness is based on SDK which should be fine for this use case as I don't imagine multiple instances all using same SDK key, but I wonder if we should include that in the documentation somewhere

typotter

Thanks everyone for the great comments and perspectives.

Refactored back to a little closer to where we started; dropped all the dual-constructor/config stuff.

typotter · 2025-02-04T16:25:30Z

src/index.ts

-  public static initialized = false;
+  initialized = false;
+
+  constructor(optionsOrConfig: EppoClientParameters | IClientOptions) {


The constructor is already public, so we needed to keep the EppoClientParameters in the constructor to avoid a major change.

That being said, I moved to a static builder method instead that lets us get away from the dual constructor.

typotter · 2025-02-04T16:29:57Z

src/index.ts

 /**
 * Initializes the Eppo client with configuration parameters.
 * This method should be called once on application startup.
 * If an initialization is in process, calling `init` will return the in-progress
 * `Promise<EppoClient>`. Once the initialization completes, calling `init` again will kick off the
 * initialization routine (if `forceReinitialization` is `true`).
+ *
+ *
+ * @deprecated


The @deprecated annotation is here to help push devs to using the new construct + wait paradigm. This method will be removed and the argument interface for the new constructor is slightly different sdkKey vs apiKey

typotter · 2025-02-04T16:30:23Z

src/index.ts

 }

-async function explicitInit(config: IClientConfig): Promise<EppoClient> {
+async function explicitInit(config: IClientConfig, instance: EppoJSClient): Promise<EppoJSClient> {


i do believe we can! good catch.

typotter · 2025-02-05T04:30:50Z

src/i-client-config.ts


-  /** Configuration settings for the event dispatcher */


typotter · 2025-02-05T04:46:31Z

src/index.ts

+    return client;
+  }
+
+  async init(config: IClientConfig): Promise<EppoJSClient> {


the old explicitInit method

typotter · 2025-02-05T05:16:19Z

src/i-client-config.ts

+export type IClientOptions = IApiOptions &
+  ILoggers &
+  IEventOptions &
+  IStorageOptions &
+  IPollingOptions;


The idea is that a dev could build different options objects then combine them for initialization. If they don't need custom event and storage options, for example, they wouldn't have those configs.

typotter · 2025-02-05T05:21:16Z

src/client-options-converter.ts

+  // Always include configuration request parameters
+  parameters.configurationRequestParameters = {
+    apiKey: options.sdkKey,
+    sdkVersion, // dynamically picks up version.
+    sdkName, // Hardcoded to `js-client-sdk`
+    baseUrl: options.baseUrl,
+    requestTimeoutMs: options.requestTimeoutMs,
+    numInitialRequestRetries: options.numInitialRequestRetries,
+    numPollRequestRetries: options.numPollRequestRetries,
+    pollAfterSuccessfulInitialization: options.pollAfterSuccessfulInitialization,
+    pollAfterFailedInitialization: options.pollAfterFailedInitialization,
+    pollingIntervalMs: options.pollingIntervalMs,
+    throwOnFailedInitialization: options.throwOnFailedInitialization,
+    skipInitialPoll: options.skipInitialRequest,
+  };


typotter · 2025-02-05T05:21:37Z

src/i-client-config.ts

+ * Configuration for regular client initialization
+ * @public
+ */
+export type IClientConfig = Omit<IClientOptions, 'sdkKey' | 'offline'> &


fixed up. moved sdkKey to the next major

typotter · 2025-02-05T05:21:52Z

src/index.spec.ts

+    it('should be independent of the singleton', async () => {
+      const apiOptions: IApiOptions = { sdkKey: '<MY SDK KEY>' };
+      const options: IClientOptions = { ...apiOptions, assignmentLogger: mockLogger };
+      const isolatedClient = new EppoJSClient(options);


moved to a builder

typotter · 2025-02-05T05:22:42Z

src/index.ts

+        this.readyPromiseResolver = resolve;
+      });
+    } else {
+      this.readyPromise = explicitInit(


init handles catching exceptions on init and throwing/logging depending on the option

rasendubi

Exposing constructor and init() method publicly is my only major concern.

The better handling of re-initialization is nice to have and we can implement it in future PRs (though it's a good idea to remove forceInitialize option now to avoid a breaking change in the future)

rasendubi · 2025-02-05T09:22:11Z

src/i-client-config.ts

-  forceReinitialize?: boolean;
-
-  /** Configuration settings for the event dispatcher */
+/** Configuration settings for the event dispatcher */


minor: this doc string is going to be stripped from the final IClientConfig. It should stay on eventIngestionConfig field

rasendubi · 2025-02-05T09:26:09Z

src/index.ts

    isObfuscated: true,
  });
-  public static initialized = false;
+
+  constructor(options: EppoClientParameters) {


Shall we keep constructor private now that we use static builders?

rasendubi · 2025-02-05T09:31:44Z

src/index.ts

+  /**
+   * Resolved when the client is initialized
+   * @private
+   */
+  private readonly initializedPromise: Promise<void>;
+
+  initialized = false;


nit: shall we move these fields (together with initializedPromiseResolver) above the constructor? It's too common to group all class fields in one fields that it becomes surprising when extra fields are "hidden" between other methods

rasendubi · 2025-02-05T09:32:35Z

src/index.ts

+
+  initialized = false;
+
+  async init(config: IClientConfig): Promise<EppoJSClient> {


Do we want to keep init private / package-only?

rasendubi · 2025-02-05T09:52:43Z

src/index.ts

+      // If the instance was polling, stop.
+      this.stopPolling();
+      // Set up assignment logger and cache
+      this.setAssignmentLogger(config.assignmentLogger);


minor: again, unrelated to the PR per se and good to handle in the future — we're actually in a good position now to fix all re-initialization bugs and race conditions by re-creating a client instead of doing partial re-init.

tl;dr on the bugs: most client components are not designed to be stopped and re-started with a different configuration. In particular, the poller does not handle it well and stopPolling stops future requests but not in-progress ones, so it may still set unexpected configuration in the store. It might be fixed with some extra checks but I still wouldn't trust the client as a whole to handle re-initialization, especially because creating a new client and discarding the old one is now extremely easy

More specific proposal:

Keep client's constructor and init methods private/package-local. (Constructor is exposing internal/unsafe configuration so we don't want users calling it. init makes it too easy too shoot yourself in the foot by re-initializing the client, so I wouldn't give this gun to users either.)

Re-initialization is disallowed on client. (If constructor and init methods are private, we don't need to check for that.)

The global init function should handle forceReinitialize by creating a new singleton and stopping the old one. This is to keep backward compatibility but also because it's the only case where this makes sense.

I would argue that we should deprecate forceReinitialize in global init — if users need re-configuring the client on the fly, they are venturing into the advanced territory and using standalone clients is easier / less error-prone.

rasendubi · 2025-02-05T10:06:12Z

src/index.ts

+  if (initializationPromise === null) {
+    initializationPromise = getInstance().init(config);


minor: again, unrelated to the PR per se but I find this initializationPromise disturbing and a potential source of errors.

If init() is called with different configuration and forceReinitialize: true, the second call is going to be silently discarded. Which may lead to all sorts of troubles as client is now running with unexpected configuration.

With forceReinitialize: false, this handling will also silence a warning, so users have less opportunity to catch their bug.

At the very least, the case of concurrent initialization is worth a warning. But overall, I think it might be a good idea to handle forceReinitialize here in this function (as per my other comment)

rasendubi · 2025-02-05T10:57:02Z

src/i-client-config.ts

+  /**
+   * Force reinitialize the SDK if it is already initialized.
+   */
+  forceReinitialize?: boolean;


As per my other comment, I propose to forbid re-initialization of standalone clients (because it's buggy and it's now possible to create new clients). While we can postpone the implementation of that, I believe that buildAndInit should not accept forceReinitialize already (as removing it later will be a major change)

typotter added 3 commits January 30, 2025 12:13

detatched construction

e20ef73

combine new and old constructors

50e66aa

use spark for md5 instead of non-exported member from common

de9f653

typotter commented Jan 31, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

typotter changed the title ~~Tp/namespace~~ feat: non-singleton EppoJSClient Jan 31, 2025

typotter added 2 commits January 31, 2025 12:13

update to latest common, use exported members

fa5baab

doc comments

953c4e4

typotter commented Jan 31, 2025

View reviewed changes

src/i-client-config.ts Outdated Show resolved Hide resolved

typotter added 2 commits January 31, 2025 12:33

wip

159df11

skip initial config for now

a73bf0c

typotter force-pushed the tp/namespace branch from 5c466e9 to a73bf0c Compare January 31, 2025 19:35

exports

7e5abe1

typotter commented Jan 31, 2025

View reviewed changes

waitForReady

92cdd1d

typotter commented Jan 31, 2025

View reviewed changes

clean up offline init method

c4b2717

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

typotter commented Jan 31, 2025

View reviewed changes

src/index.ts Show resolved Hide resolved

typotter added 2 commits January 31, 2025 12:49

no need to export

ddab6c0

DOCS

d55d23d

typotter requested review from aarsilv, felipecsl and rasendubi January 31, 2025 19:54

typotter requested a review from sameerank January 31, 2025 19:54

typotter marked this pull request as ready for review January 31, 2025 19:54

rasendubi reviewed Feb 3, 2025

View reviewed changes

sameerank reviewed Feb 3, 2025

View reviewed changes

felipecsl reviewed Feb 3, 2025

View reviewed changes

aarsilv reviewed Feb 4, 2025

View reviewed changes

aarsilv assigned typotter Feb 4, 2025

typotter added 2 commits February 4, 2025 15:35

builder method

3c18bc9

polish

4b8077f

typotter commented Feb 5, 2025

View reviewed changes

typotter requested review from felipecsl, aarsilv, rasendubi and sameerank February 5, 2025 05:24

rasendubi reviewed Feb 5, 2025

View reviewed changes

		// For use only with the singleton instance.
		const flagConfigurationStore = configurationStorageFactory({

	* initialization routine (if `forceReinitialization` is `true`).
	* initialization routine (if `forceReinitialize` is `true`).


		initialized = false;

		async init(config: IClientConfig): Promise<EppoJSClient> {

		if (initializationPromise === null) {
		initializationPromise = getInstance().init(config);

feat: non-singleton EppoJSClient #166

Are you sure you want to change the base?

feat: non-singleton EppoJSClient #166

Conversation

typotter commented Jan 31, 2025 • edited Loading

labels: mergeable

Motivation and Context

Description

How has this been documented?

How has this been tested?

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

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

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

sameerank left a 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

felipecsl left a 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

aarsilv left a 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

Choose a reason for hiding this comment

typotter commented Jan 31, 2025 •

edited

Loading