chore: move initialization into class methods, limit singleton use #170

typotter · 2025-02-11T23:24:07Z

labels: mergeable

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

Motivation and Context

This PR is carved from #166 in an attempt to make #166 focused on the the "new" API. This PR adds no net new functionality, however, the new EppoJSClient.init method, marked @internal does allow for unbuffered initialization calls just as the global init method previously allowed. This internal method can be made private once we remove the global init method in the next major release.

The init and getInstance() methods work on a singleton instance of the EppoJSClient 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 EppoJSClient 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.

TL;DR: We want to move away from singleton access and allow for multiple instance of the EppoJSClient while maintaining the current API

Description

Move bulk of initialization to a class method
funnel all references to the static instance through getInstance()
deprecate EppoJSClient.instance
make initialized and ensureInitialized class members; still set EppoJSClient.initialized as before to ensure backward compatibility with the current API

How has this been documented?

doc comments

How has this been tested?

tests

typotter · 2025-02-11T23:25:27Z

src/index.ts

@@ -287,308 +584,42 @@ export function buildStorageKeySuffix(apiKey: string): string {
 * @public
 */
 export function offlineInit(config: IClientConfigSync): EppoClient {
-  const isObfuscated = config.isObfuscated ?? false;


moved to class

typotter · 2025-02-11T23:25:48Z

src/index.ts

-  return client;
-}
-
-async function explicitInit(config: IClientConfig): Promise<EppoClient> {


moved to EppoJSClient.init

typotter · 2025-02-12T03:53:10Z

src/index.ts

+   * @internal
+   * @param config
+   */
+  async init(config: IClientConfig): Promise<EppoJSClient> {


no changes here from explicitInit

typotter · 2025-02-12T03:53:26Z

src/index.ts

+   * @internal
+   * @param config
+   */
+  offlineInit(config: IClientConfigSync) {


no changes from offline init

typotter · 2025-02-13T19:50:10Z

src/index.ts

-        : initFromConfigStoreError
-          ? initFromConfigStoreError
-          : new Error('Eppo SDK: No configuration source produced a valid configuration');
+      applicationLogger.warn(


The diff makes it hard to see what's left in this method;

We keep the forceReinitialize check here to keep it out of the EppoJSClient.init method

Warn if an initialization is currently in progress

felipecsl · 2025-02-14T23:25:03Z

Just a thought here, since this will require a major version bump, does it even make sense to deprecate the old APIs?

Since we're doing a major departure in terms of API design, it might not make sense to keep deprecated APIs around, as that will make the internal design a lot more complicated.

Since upgrading will require code changes anyways (assuming it will be a major bump), then we might as well just redesign it as a blank slate, not worrying about deprecations. WDYT?

aarsilv

Thanks for carving out this PR, makes things very clear. I like what I see! 🚀

Consider throwing a package.json minor version bump in here just so you don't forget 😛

aarsilv · 2025-02-16T05:07:55Z

src/index.ts

+  /**
+   * @deprecated. use `getInstance()` instead.
+   */


Guessing the plan in the future is to make this private? Or have getInstance() construct just-in time if one not set?

Affirmative to making this private, when we make the next major bump. The first step is to encourage everyone to move to the single accessor.

aarsilv · 2025-02-16T05:08:55Z

src/index.ts

  public static initialized = false;

+  private initialized = false;


aarsilv · 2025-02-16T05:10:33Z

src/index.ts

@@ -287,308 +584,42 @@ export function buildStorageKeySuffix(apiKey: string): string {
 * @public
 */
 export function offlineInit(config: IClientConfigSync): EppoClient {
-  const isObfuscated = config.isObfuscated ?? false;


aarsilv

Thanks for iterating on all the comments!

The one thing I'm still confused about (probably because it's late 😛) is the deprecation of getInstance(). Are we expecting people to handle managing the instance they get from buildAndInit()? Does this mean that they would need to write their own getInstance() methods and wrapper assignment methods with null checks that return default value if not initialized? If so, I think this may be unnecessary friction and we should think about how to make the 99% use case--a singleton in a code base where they are not concerned about conflicts--as friction-free as possible.

typotter

Just a thought here, since this will require a major version bump, does it even make sense to deprecate the old APIs?
The intent for this PR was no more than a minor bump. Is there a breaking change to the API I overlooked? @felipecsl

Since we're doing a major departure in terms of API design, it might not make sense to keep deprecated APIs around, as that will make the internal design a lot more complicated.

This I agree on, however, that's better left for #166. This PR doesn't change any if the current API, nor deprecates it (without an immediately available equivalent, ex: getInstance instead of EppoJSClient.instance)

Since upgrading will require code changes anyways (assuming it will be a major bump), then we might as well just redesign it as a blank slate, not worrying about deprecations. WDYT?

Again, the intent of this PR is no breaking changes.
The current focus is to deliver on the concrete ask of multiple instances and leveraging the opportunity to to incrementally work towards the ideal interface.
Deprecating with a replacement doesn't require a code change but if one is made to avoid the annotation, it's a very simple change. Accumulating some @deprecateds while we work out what the ideal is seems like a good tradeoff to allow us to continue to ship improvements and bug fixes to the current major version.

typotter · 2025-02-17T05:37:59Z

src/index.ts

+  /**
+   * @deprecated. use `getInstance()` instead.
+   */


Affirmative to making this private, when we make the next major bump. The first step is to encourage everyone to move to the single accessor.

typotter · 2025-02-18T16:45:03Z

The one thing I'm still confused about (probably because it's late 😛) is the deprecation of getInstance(). Are we expecting people to handle managing the instance they get from buildAndInit()? Does this mean that they would need to write their own getInstance() methods and wrapper assignment methods with null checks that return default value if not initialized? If so, I think this may be unnecessary friction and we should think about how to make the 99% use case--a singleton in a code base where they are not concerned about conflicts--as friction-free as possible.

@aarsilv no deprecation of getInstance in this PR. Will likely drop that in #166 so we can ship the customer-requested feature and continue discussion around the evolution of the SDK in a non-blocking way.

felipecsl

I think this is directionally correct, LGTM!

typotter added 2 commits February 11, 2025 16:08

chore: Move initialization code from static to the instance

621cccb

docs

ed3c4ce

typotter commented Feb 11, 2025

View reviewed changes

typotter changed the title ~~Typo/no singleton~~ chore: move initialization into class methods, limit singleton use Feb 11, 2025

typotter marked this pull request as ready for review February 11, 2025 23:31

typotter commented Feb 12, 2025

View reviewed changes

typotter added 5 commits February 11, 2025 20:57

lint

5f96888

move reinit check

228e1e4

restore buffered init

4d35d4f

docs

cf3c2ba

docs

132c0a5

typotter commented Feb 13, 2025

View reviewed changes

typotter requested review from felipecsl, aarsilv and rasendubi February 13, 2025 19:55

typotter mentioned this pull request Feb 14, 2025

feat: non-singleton EppoJSClient #166

Open

aarsilv approved these changes Feb 16, 2025

View reviewed changes

aarsilv assigned rasendubi Feb 16, 2025

aarsilv reviewed Feb 16, 2025

View reviewed changes

aarsilv self-requested a review February 16, 2025 05:27

typotter added 3 commits February 16, 2025 22:48

minor bump

0f6e2de

undeprecate

86b1665

docs

cd28979

typotter commented Feb 17, 2025

View reviewed changes

typotter assigned felipecsl Feb 17, 2025

typotter added 2 commits February 16, 2025 23:04

merge main

605199a

docs

0a5db5c

felipecsl approved these changes Feb 18, 2025

View reviewed changes

typotter merged commit a602ba5 into main Feb 18, 2025
8 checks passed

typotter deleted the typo/no-singleton branch February 18, 2025 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: move initialization into class methods, limit singleton use #170

chore: move initialization into class methods, limit singleton use #170

typotter commented Feb 11, 2025 •

edited

Loading

typotter Feb 11, 2025

aarsilv Feb 16, 2025

typotter Feb 11, 2025

typotter Feb 12, 2025

typotter Feb 12, 2025

typotter Feb 13, 2025

felipecsl commented Feb 14, 2025

aarsilv left a comment

aarsilv Feb 16, 2025

typotter Feb 17, 2025

aarsilv Feb 16, 2025

aarsilv Feb 16, 2025

aarsilv left a comment

typotter left a comment

typotter Feb 17, 2025

typotter commented Feb 18, 2025

felipecsl left a comment

		public static initialized = false;

		private initialized = false;

chore: move initialization into class methods, limit singleton use #170

chore: move initialization into class methods, limit singleton use #170

Conversation

typotter commented Feb 11, 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

felipecsl commented Feb 14, 2025

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

aarsilv left a comment

Choose a reason for hiding this comment

typotter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typotter commented Feb 18, 2025

felipecsl left a comment

Choose a reason for hiding this comment

typotter commented Feb 11, 2025 •

edited

Loading