refactor: JS SDK v5 #256

rasendubi · 2025-03-20T16:09:28Z

See documentation for a bit more context.

Make Configuration a passive/immutable piece of data that encapsulates flags/bandits/etc
Split current configuration store into two components:
- ConfigurationStore as a central piece that just tracks the currently active configuration (in memory). The rest of components integrate with ConfigurationStore and communicate via getting/setting/listening for Configuration (see picture)
- Persistence component with two features: loading initial configuration and persisting subsequent configurations (in background)
Configuration feed a simple observer for components to learn about new configuration (that are not necessarily activated)

TODO:

Integrate persistence (race initial load in init, store configurations on change)
cleanup
fix tests
fix precomputed client 🤕
updates to client and node sdks

This integrates better with editor tools.

typotter

Love seeing more code deleted than added, yet new features developing from it.

typotter · 2025-03-21T04:57:15Z

.prettierrc

@@ -0,0 +1,5 @@
+{


src/configuration-requestor.ts

typotter · 2025-03-21T05:04:00Z

src/configuration.ts

+}
+
+/** @internal */
+type ConfigurationWire = {


heads up, IConfigurationWire is already defined here

Yes. I tried using it but ran into it having readonly fields, so I couldn't construct it like this:

const wire: ConfigurationWire = { version: 1, }; if (this.flags) { wire.config = { ...this.flags, response: JSON.stringify(this.flags.response), }; } // ...

It's also quite a bit more convoluted with braided strings and class implementation, etc.

So my idea is to have the simplest definition here, just for sanity check—it's not exported and is not used for anything but Configuration.toString()/Configuration.fromString(). (The design idea is that it's just a string representation of Configuration.)

Maybe call it ConfigurationWireStrings then? 😅

typotter · 2025-03-21T05:05:05Z

src/configuration-wire/configuration-wire-helper.ts

Why delete?

I guess the BaseEppoClient would actually provide this functionality.

Yeah, ConfigurationRequestor is basically doing the same thing now with the new fetchConfiguration() method (when paired with Configuration.toString())

typotter · 2025-03-21T05:06:35Z

src/configuration.ts

+
+  /** @internal
+   *
+   * Returns flag configuration for the given flag key. Obfuscation is


typotter · 2025-03-21T05:07:39Z

src/evaluator.ts

@@ -14,7 +15,7 @@ import {
  Allocation,
  Split,
  VariationType,
-  ConfigDetails,


typotter · 2025-03-21T05:08:56Z

src/evaluator.ts

    );
+    const configFormat = flagsConfig?.response.format;
+    const obfuscated = configFormat !== FormatEnum.SERVER;


Should isObfuscated be a method on Configuration so we're not leaking the FormatEnum details?

isObfuscated() is oversimplification in a sense that Configuration contains multiple pieces (flags, bandits, precomputed), where each could be obfuscated, not obfuscated, or simply missing. So a single isObfuscated() method doesn't cut it.

It also spreads this oversimplified model in programmers' brains, so we're more likely to miss some edge cases. I would rather have us working with raw data instead of half-baked abstractions. It may not be as pretty (initially) but at least it's obvious what's going on

re leaking details: my philosophy is that SDKs are in the know. Evaluation is highly dependent on flags response type, so there's no sense hiding it. When talking about leaking details, the only meaningful distinction for me is user–SDK. Configuration is exposing full server responses but is marking them @internal for this reason

re FormatEnum specifically: it's currently mixing formats from two independent and unrelated configuration pieces (flags and precomputed), that actually have different rules for determining obfuscation

Next refactoring step is splitting it into FlagsConfigurationFormat and PrecomputedConfigurationFormat. And then we can have a proper isObfuscated(FlagsConfigurationFormat): boolean helper to encode this piece of knowledge

Longer-term still, we shall add isObfuscated field to flags configuration

I don't think we need to handle differently formatted flags and bandits and whatnot. If bandit format doesn't match flags we could just throw an error or something.

typotter · 2025-03-21T05:09:54Z

src/http-client.ts

+      return undefined;
+    }
+    return {
+      response,


great steps towards passing and checking the eTag in this SDK

typotter · 2025-03-21T05:16:27Z

src/client/eppo-client.ts

+      obfuscated:
+        this.getConfiguration()?.getFlagsConfiguration()?.response.format === FormatEnum.CLIENT,


obfuscated computation should move somewhere centralized

[ci skip]

typotter · 2025-04-07T15:58:26Z

src/broadcast.ts

+      }
+    }
+  }
+}


nit: newline

typotter · 2025-04-07T16:01:12Z

src/configuration-feed.ts

+/**
+ * Enumeration of possible configuration sources.
+ */
+export enum ConfigurationSource {


What about initial/offline/bootstrap?

It is considered as "cache" but I think it makes sense to add "User" type as well

typotter · 2025-04-08T15:47:52Z

src/configuration-store/configuration-store.ts

@@ -21,6 +21,8 @@ import { Environment } from '../interfaces';
 *
 * The policy choices surrounding the use of one or more underlying storages are
 * implementation specific and handled upstream.
+ *
+ * @deprecated To be replaced with ConfigurationStore and PersistentStorage.


Are we not able to remove these entirely?

We'll be able to once I finish precomputed client migration

typotter · 2025-04-08T15:51:01Z

src/persistent-configuration-cache.ts

+
+/**
+ * ConfigurationCache is a helper class that subscribes to a configuration feed and stores latest
+ * configuration in persistent storage.


also loads from storage and publishes to the feed

typotter · 2025-04-08T15:53:02Z

src/persistent-configuration-cache.ts

+    private readonly storage: PersistentConfigurationStorage,
+    private readonly configurationFeed?: ConfigurationFeed,
+  ) {
+    configurationFeed?.addListener(async (configuration, source) => {


do we need to worry about cancelling the listener?

In what situation would you cancel the listener?

typotter · 2025-04-08T15:56:43Z

src/configuration-requestor.ts

+ * has proper versions for all bandits references in flags
+ * configuration.
+ */
+const banditsUpToDate = (flags: FlagsConfig, bandits: BanditsConfig | undefined): boolean => {


💯 I like the inversion here over what we've coded this as (needToFetchBandits). This makes it much more clear.

typotter · 2025-04-08T15:58:59Z

src/persistent-configuration-cache.ts

+   * The method is allowed to do async work (which is not awaited) or
+   * throw exceptions (which are ignored).
+   */
+  storeConfiguration(configuration: Configuration | null): PromiseLike<void>;


I haven't finished reading all the code yet but something to call out here if it's not already integrated, is the need for the persistent store to be keyed by API key (or some other function thereof) so that a single storage subsystem can store config for multiple api keys simultaneously.

I think that's best handled by individual constructors. We don't allow changing sdk key on the fly, so a persistent store instance is always used with the same key

Does the upstream creation have a way to not have these configurations clobber each other if different keys are used?

…f string

rasendubi · 2025-04-23T18:38:52Z

package.json

+    "./internal": {
+      "types": "./dist/internal.d.ts",
+      "default": "./dist/internal.js"


Want to clearly delineate public and internal APIs, so @eppo/js-client-sdk-common/internal is intended for internal SDK usage only and should not be re-exposed to users.

rasendubi · 2025-04-23T18:41:10Z

src/client/eppo-client.ts

+   * If the client is configured to precompute, returns a Subject-scoped instance for the
+   * precomputed configuration.
+   */
+  public getPrecomputedSubject(): Subject | undefined {


This API is replacing a dedicated PrecomputedClient

It still feels like a "client" to me as it will be doing assignment logging and other stuff beyond the scope of a "subject"

Perhaps we should call these things something like getClientForSubject() or getSubjectClient(), getPrecomputedSubjectClient(), etc.,

rasendubi · 2025-04-23T18:42:17Z

src/client/eppo-client.ts

+    const precomputed = config.getPrecomputedConfiguration();
+    if (precomputed && precomputed.subjectKey === subjectKey) {
+      // Short-circuit evaluation if we have a matching precomputed configuration.
+      const precomputedResult = this.evaluatePrecomputedAssignment(
+        precomputed,
+        flagKey,
+        expectedVariationType,
+      );
+
+      return precomputedResult.flagEvaluation;
+    }
+
+    const flag = config.getFlag(flagKey);


EppoClient now automatically handles precomputed assignments if precomputed configuration is present for the specified subject key

rasendubi · 2025-04-23T18:46:16Z

src/client/subject.ts

+/**
+ * A wrapper around EppoClient that automatically supplies subject key, attributes, and bandit
+ * actions for all assignment and bandit methods.
+ *
+ * This is useful when you always want to use the same subject and attributes for all flag
+ * evaluations.
+ */
+export class Subject {


This is subject-aware client

Initially named it subject-bound client but then was debating with myself whether I should duplicate ALL public API from client or just evaluation methods. We can always expose more methods later, so decided to only implement evaluation ones. Then realized that we can name it "Subject" and have getPrecomputedSubject() on EppoClient, so that seemed like a neat idea

rasendubi · 2025-04-23T18:47:56Z

src/configuration-requestor.ts

  }

-  public getConfiguration(): IConfiguration {
-    return this.configuration;
+  public async fetchConfiguration(): Promise<Configuration> {


fetchConfiguration() now returns full Configuration object

ConfigurationRequestor is now also able to handle both regular and precomputed requests

rasendubi · 2025-04-23T18:49:08Z

src/configuration.ts

+ *
+ * @public
+ */
+export class Configuration {


This is the boss

rasendubi · 2025-04-23T18:51:31Z

src/evaluator.ts

+export interface FlagEvaluation {
+  assignmentDetails: AssignmentResult;
+  assignmentEvent?: IAssignmentEvent;
+  banditEvent?: IBanditEvent;
 }

 export class Evaluator {


TODO for the future (non-breaking): evaluation is currently spread thin between EppoClient and Evaluator. It would make a ton of sense for Evaluator accept configuration storage as input and handle all evaluation

Are you talking about the checks done in getAssignmentDetail()? Agree that could be brought out of the client.

Yes. Checks, handling of overrides, disabled flags, etc.

rasendubi · 2025-04-23T18:55:02Z

src/kvstore.ts

+/**
+ * Simple key-value store interface. JS client SDK has its own implementation based on localStorage.
+ */
+export interface KVStore<T> {


This is old IAsyncStore — renamed and removed isInitialized() as it was unused. It is only used for overrides store now

rasendubi · 2025-04-23T18:55:44Z

src/precomputed-configuration.ts

+import { ContextAttributes, FlagKey, HashedFlagKey } from './types';
+
+// Base interface for all configuration responses
+interface IBasePrecomputedConfigurationResponse {


no changes to types here — just moved from configuration-wire-types

rasendubi · 2025-04-23T18:56:44Z

src/salt.ts

+ *
+ * @internal
+ */
+export function generateSalt() {


actually found that SDK was not generating salt and getPrecomputedConfiguration was asking user to pass salt (which is a bad idea as I guess it was never passed)

aarsilv · 2025-04-29T19:54:50Z

docs/configuration-lifecycle.md

@@ -0,0 +1,70 @@
+# Configuration Lifecycle
+
+This document explains how configuration is managed throughout its lifecycle in the Eppo SDK.


aarsilv · 2025-04-29T19:57:23Z

docs/configuration-lifecycle.md

+
+1. **Configuration Loading Strategy**:
+   - `stale-while-revalidate`: Uses cached config if within `maxStaleSeconds`, while fetching fresh data
+   - `only-if-cached`: Uses cached config without network requests


even if stale? If so we should put that in

oh, no, maxStaleSeconds is still respected

aarsilv · 2025-04-29T19:57:55Z

docs/configuration-lifecycle.md

+During initialization, the client:
+
+1. **Configuration Loading Strategy**:
+   - `stale-while-revalidate`: Uses cached config if within `maxStaleSeconds`, while fetching fresh data


~~where do we specify what we do with the fetched data? Save for next time? Switch immediately?~~
I see that is covered later in "Configuration Activation" perhaps we should specify that here. "...while fetching fresh data to be activated per the configuration activation strategy"

aarsilv · 2025-04-29T19:58:52Z

docs/configuration-lifecycle.md

+4. **Completion**:
+   - Initialization completes when either:
+     - Fresh configuration is fetched (for network strategies)
+     - Cache is loaded (for cache-only strategies)


or stale-while-revalidate right?

For stale-while-revalidate, the initialization completes when a fresh configuration is fetched

Oh I thought that strategy means serve stale assignments and fetch and background? I think that would mean initialization should finish prior to fetch.

aarsilv · 2025-04-29T20:03:41Z

src/broadcast.ts

+ *
+ * @internal
+ */
+export class BroadcastChannel<T extends unknown[]> {


Any particular reason you're rolling your own vs. using a multi-js-environment third party library like eventemitter3?

This seems simple enough and keeps dependencies down, which are advantages so I'm cool rolling with this just curious if you had thoughts about the pros and cons.

Wanted it to have proper typescript support and be lightweight (eventemitter3 is neither + unmaintained). Couldn't find a good library quickly, so it was easier to roll my own

makes sense!

aarsilv · 2025-04-29T21:14:49Z

src/configuration.ts

+    if (banditKey) {
+      return this.parts.bandits?.response.bandits[banditKey] ?? null;
+    }
+    return null;


Could combine to simply

return banditKey && this.parts.bandits?.response.bandits[banditKey] ?? null;

aarsilv · 2025-04-29T21:15:34Z

src/configuration.ts

+}
+
+/** @internal */
+type ConfigurationWire = {


Maybe call it ConfigurationWireStrings then? 😅

aarsilv · 2025-04-29T21:19:10Z

src/evaluator.ts

+export interface FlagEvaluation {
+  assignmentDetails: AssignmentResult;
+  assignmentEvent?: IAssignmentEvent;
+  banditEvent?: IBanditEvent;
 }

 export class Evaluator {


Are you talking about the checks done in getAssignmentDetail()? Agree that could be brought out of the client.

aarsilv · 2025-04-29T21:20:47Z

src/evaluator.ts

    );
+    const configFormat = flagsConfig?.response.format;
+    const obfuscated = configFormat !== FormatEnum.SERVER;


I don't think we need to handle differently formatted flags and bandits and whatnot. If bandit format doesn't match flags we could just throw an error or something.

aarsilv · 2025-04-29T21:25:49Z

src/persistent-configuration-cache.ts

+   * The method is allowed to do async work (which is not awaited) or
+   * throw exceptions (which are ignored).
+   */
+  storeConfiguration(configuration: Configuration | null): PromiseLike<void>;


Does the upstream creation have a way to not have these configurations clobber each other if different keys are used?

leoromanovsky · 2025-05-01T00:18:43Z

docs/configuration-lifecycle.md

+## Communication Flow
+
+The ConfigurationFeed acts as a central hub through which different components communicate:
+
+![](./configuration-lifecycle.excalidraw.png)
+
+When a new configuration is received (either from network or cache), it's broadcast through the ConfigurationFeed. Components subscribe to this feed to react to configuration changes. Importantly, configurations broadcast on the ConfigurationFeed are not necessarily activated - they may never be activated at all, as they represent only the latest discovered configurations. For components interested in the currently active configuration, the ConfigurationStore provides its own broadcast channel that only emits when configurations become active.


This is a really exciting new interface - something I want to support in the near future is a concept that the requested configuration is not going to be "complete": we might want to deliver updates via a websocket or the edge JSON will be paged. Either way having a separation between the network response being the canonical source of truth and the configuration store is very important.

Do you feel like the interface you are creating makes us read to push updated in this way? I am wondering if the broadcaster should have a more verbose API that tells listeners whether flags are being added or removed.

rasendubi added 9 commits March 20, 2025 13:18

refactor: add ConfigurationStore abstraction

60fafe1

refactor: add PersistentConfigurationStorage interface

68c8b2f

refactor: add Configuration type (temp aliased to IConfiguration)

b51b576

chore: add top-level .prettierrc

a491d0b

This integrates better with editor tools.

refactor: make evaluation use new ConfigurationStore

9325c88

refactor: fix linter errors

c2a07c1

refactor: add Configuration type

6df8767

refactor: make requestor work with new ConfigurationStore

4d3b534

refactor: eppo client cleanup

2f70a51

rasendubi requested a review from typotter March 20, 2025 16:09

refactor: remove temp configuration

d4c6f06

typotter reviewed Mar 21, 2025

View reviewed changes

rasendubi added 4 commits March 21, 2025 18:13

refactor: don't fetch bandits if current model is up-to-date

60c8617

[ci skip]

refactor: implement new initialization/poller behavior

51f4abb

refactor: introduce configuration feed

34ff6c3

Merge remote-tracking branch 'origin/main' into HEAD

a7da43d

typotter self-requested a review April 7, 2025 14:26

rasendubi added 2 commits April 7, 2025 20:22

feat: extend Configuration/Requestor to support precomputed

6b4725c

feat: support precomputed config for flags evaluation

311f61f

typotter reviewed Apr 8, 2025

View reviewed changes

rasendubi added 8 commits April 11, 2025 18:41

feat: make EppoClient handle precomputed bandits

e5e5b9f

feat: add Subject (subject-scoped client)

9ee4834

feat: make getPrecomputedConfiguration return Configuration instead o…

3b79e68

…f string

refactor: remove old configuration store

bef0393

refactor: revise public/internal APIs

311364d

refactor: cleanup/prettier

9647e30

refactor: remove IConfigurationWire

4c9a8f8

Merge remote-tracking branch 'origin/main' into HEAD

896ffb8

rasendubi commented Apr 23, 2025

View reviewed changes

src/configuration.ts

*

* @public

*/

export class Configuration {

Copy link

Collaborator Author

rasendubi Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the boss

rasendubi commented Apr 23, 2025

View reviewed changes

rasendubi added 2 commits April 23, 2025 21:52

refactor: re-run prettier

a8866fb

refactor: remove ConfigDetails

6bea66d

rasendubi commented Apr 23, 2025

View reviewed changes

refactor: fix lints

badd643

rasendubi changed the title ~~refactor: new ConfigurationStore~~ refactor: JS SDK v5 Apr 23, 2025

typotter requested review from aarsilv, sameerank and typotter April 24, 2025 16:12

aarsilv reviewed Apr 29, 2025

View reviewed changes

aarsilv requested a review from leoromanovsky April 29, 2025 21:27

aarsilv assigned leoromanovsky Apr 29, 2025

leoromanovsky reviewed May 1, 2025

View reviewed changes

		obfuscated:
		this.getConfiguration()?.getFlagsConfiguration()?.response.format === FormatEnum.CLIENT,

		@@ -0,0 +1,70 @@
		# Configuration Lifecycle

		This document explains how configuration is managed throughout its lifecycle in the Eppo SDK.

refactor: JS SDK v5 #256

Are you sure you want to change the base?

refactor: JS SDK v5 #256

Uh oh!

Conversation

rasendubi commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typotter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

rasendubi commented Mar 20, 2025 •

edited

Loading