feat: [add syncInit method] (FF-2431) #81

leoromanovsky · 2024-06-19T01:54:26Z

Fixes: #issue

Motivation and Context

Users need to be able to bootstrap the JS SDK from configuration obtained from their server, or another external process. The format of this configuration might or might not be obfuscated.

Furthermore users need to be able to instantiate the SDK synchronously.

Description

adds a new public function syncInit - it accepts a map of flags in both obfuscated and not-obfuscated formats. pairs with the setIsObfuscated parameter to make the SDK understand the incoming data.
Augments the README with instructions for how to use this method.

How has this been tested?

New unit tests to validate behavior of providing obfuscated and not-obfuscated configuration.

leoromanovsky · 2024-06-21T03:00:00Z

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

@@ -16,7 +16,7 @@ getIntegerAssignment(flagKey: string, subjectKey: string, subjectAttributes: Rec
 |  --- | --- | --- |
 |  flagKey | string |  |
 |  subjectKey | string |  |
-|  subjectAttributes | Record&lt;string, any&gt; |  |
+|  subjectAttributes | Record&lt;string, AttributeType&gt; |  |


I don't get why this is changing - I'm assuming someone else is checking in doc changes without this?

leoromanovsky · 2024-06-21T03:00:57Z

src/index.spec.ts

+describe('sync init', () => {
+  it('initializes with flags in obfuscated mode', () => {
+    const client = initSync({
+      isObfuscated: true,
+      flagsConfiguration: {
+        [obfuscatedFlagKey]: mockObfuscatedUfcFlagConfig,
+      },
+    });
+
+    expect(client.getStringAssignment(flagKey, 'subject-10', {}, 'default-value')).toEqual(
+      'variant-1',
+    );
+  });
+
+  it('initializes with flags in not-obfuscated mode', () => {
+    const client = initSync({
+      isObfuscated: false,
+      flagsConfiguration: {
+        [flagKey]: mockNotObfuscatedFlagConfig,
+      },
+    });
+
+    expect(client.getStringAssignment(flagKey, 'subject-10', {}, 'default-value')).toEqual(
+      'variant-1',
+    );
+  });
+});
+


new tests for the functionality being added in this PR

leoromanovsky · 2024-06-21T03:01:29Z

src/index.spec.ts

@@ -47,7 +55,64 @@ const obfuscatedFlagKey = md5Hash(flagKey);
 const allocationKey = 'traffic-split';
 const obfuscatedAllocationKey = base64Encode(allocationKey);

-const mockUfcFlagConfig: Flag = {


Renamed this to be called obfuscated for clarity

leoromanovsky · 2024-06-21T03:03:45Z

src/index.ts

+export interface IClientConfigSync {
+  flagsConfiguration: Record<string, Flag>;
+
+  assignmentLogger?: IAssignmentLogger;
+
+  isObfuscated?: boolean;
+
+  throwOnFailedInitialization?: boolean;
+}
+


These are the possible configuration options for calling the initSync method - note that no SDK key is provided as it will be operating in off-line mode.

Assuming that on the server the user stringified the configurations to a JSON string, they will need to parse it back into the object using the types we export in the SDK.

leoromanovsky · 2024-06-21T03:03:59Z

src/index.ts

+    }
+
+    // There is no SDK key in the offline context.
+    const storageKeySuffix = buildStorageKeySuffix('no_api_key');


Got another idea for handling this?

just use a random string instead, I think that would be cleaner

The only thing I don't love about this:

generate a random string in JS seems like so much code [...Array(8)].map(() => Math.random().toString(36)[2]).join(''); but that's fine

if we move to having a persistent store i don't want to generate a different random string each initialization which would make the store useless. perhaps a different static string like offline is more clear?

yeah I think either should be fine, as long as the intention is clear. could be as simple as extracting to a function like generateStorageCacheKeySuffixForOffline or something like that

leoromanovsky · 2024-06-21T03:04:42Z

src/index.ts

+    // As this is a synchronous initialization,
+    // we are unable to call the async `init` method on the assignment cache
+    // which loads the assignment cache from the browser's storage.
+    // Therefore there is no purpose trying to use a persistent assignment cache.
+    const assignmentCache = assignmentCacheFactory({
+      storageKeySuffix,
+      forceMemoryOnly: true,
+    });
+    EppoJSClient.instance.useCustomAssignmentCache(assignmentCache);


There is still value to have an in-memory cache as it will remove duplicates for the duration of the session. Perhaps we can think creatively how to support this.

leoromanovsky · 2024-06-21T03:04:55Z

src/index.ts

+    // Default to obfuscated mode when requesting configuration from the server.
+    instance.setIsObfuscated(true);


Making it clear here.

felipecsl

Main feedback is around the assignment cache/config cache usage, otherwise looks good to me

felipecsl · 2024-06-21T16:04:34Z

README.md

+// configuration from your server SDK
+const configurationJsonString: string = getConfigurationFromServer();
+// The configuration will be not-obfuscated from your server SDK. If you have obfuscated flag values, you can use the `ObfuscatedFlag` type.
+const flagsConfiguration: Record<string, Flag | ObfuscatedFlag> = JSON.parse(configurationJsonString);


nit: I think i would omit the types for brevity since they are inferred anyways

docs/js-client-sdk.chromestorageengine._constructor_.md

src/index.ts

felipecsl · 2024-06-21T16:08:15Z

src/cache/assignment-cache-factory.ts

  const simpleCache = new SimpleAssignmentCache();
+  if (forceMemoryOnly) {


instead of doing this, which makes this method more complex (unecessarily, I think), i'd just instantiate SimpleAssignmentCache directly from syncInit

Yea - I don't have strong feelings about it either way. On the one hand this method becomes the central way to instantiate caches without the caller needing to know anything about the SimpleAssignmentCache besides saying they want an inMemory.

i feel like it's an unnecessary abstraction that adds complexity. not gonna block on it, but it's my personal preference ;)

felipecsl · 2024-06-21T16:09:48Z

src/index.ts

+    }
+
+    // There is no SDK key in the offline context.
+    const storageKeySuffix = buildStorageKeySuffix('no_api_key');


just use a random string instead, I think that would be cleaner

src/index.ts

leoromanovsky force-pushed the lr/ff-2431/sync-init branch from 2e5636d to 8c18a65 Compare June 21, 2024 02:59

feat: [Add syncInit method] (FF-2431)

2e12c7b

leoromanovsky force-pushed the lr/ff-2431/sync-init branch from 8c18a65 to 2e12c7b Compare June 21, 2024 03:43

leoromanovsky marked this pull request as ready for review June 21, 2024 03:46

leoromanovsky commented Jun 21, 2024

View reviewed changes

leoromanovsky requested review from felipecsl and sameerank June 21, 2024 03:46

add correct imports to readme

694c57b

felipecsl reviewed Jun 21, 2024

View reviewed changes

leoromanovsky added 3 commits June 21, 2024 12:06

Merge branch 'main' into lr/ff-2431/sync-init

e9442d6

docs

cfab88c

fix lint imports

194522e

sameerank approved these changes Jun 21, 2024

View reviewed changes

src/index.ts Outdated Show resolved Hide resolved

leoromanovsky added 7 commits June 21, 2024 12:36

offlineSync

edcd794

static suffix

c62a3b9

Merge branch 'main' into lr/ff-2431/sync-init

845c494

offlineInit

cc60579

no docs

df414fd

clarify readme

b331b4c

add table of options to readme

6ff891e

leoromanovsky merged commit c8f84db into main Jun 21, 2024
2 checks passed

leoromanovsky deleted the lr/ff-2431/sync-init branch June 21, 2024 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [add syncInit method] (FF-2431) #81

feat: [add syncInit method] (FF-2431) #81

leoromanovsky commented Jun 19, 2024 •

edited

Loading

leoromanovsky Jun 21, 2024

leoromanovsky Jun 21, 2024

leoromanovsky Jun 21, 2024

leoromanovsky Jun 21, 2024

leoromanovsky Jun 21, 2024

felipecsl Jun 21, 2024

leoromanovsky Jun 21, 2024

felipecsl Jun 21, 2024

leoromanovsky Jun 21, 2024

leoromanovsky Jun 21, 2024

felipecsl left a comment

felipecsl Jun 21, 2024

felipecsl Jun 21, 2024

leoromanovsky Jun 21, 2024

felipecsl Jun 21, 2024

felipecsl Jun 21, 2024

		// Default to obfuscated mode when requesting configuration from the server.
		instance.setIsObfuscated(true);

		const simpleCache = new SimpleAssignmentCache();
		if (forceMemoryOnly) {

feat: [add syncInit method] (FF-2431) #81

feat: [add syncInit method] (FF-2431) #81

Conversation

leoromanovsky commented Jun 19, 2024 • edited Loading

Motivation and Context

Description

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoromanovsky commented Jun 19, 2024 •

edited

Loading