feat: [do not re-init and perform a network request if the singleton … #95
Conversation
…has already been init] (FF-2921)
| @@ -191,6 +191,7 @@ describe('EppoJSClient E2E test', () => { | |||
| apiKey, | |||
| baseUrl, | |||
| assignmentLogger: mockLogger, | |||
| forceReinitialize: true, | |||
There was a problem hiding this comment.
A lot of this is pretty messy - ideally we would not need to do this in tests but the singletons persist across test cases. Maybe this trick is what's needed for an isolated test:
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;
});
There was a problem hiding this comment.
I think it's nice to be able to force re-initialize, for tests and possibly other applications
| @@ -871,6 +932,7 @@ describe('initialization options', () => { | |||
| assignmentLogger: mockLogger, | |||
| updateOnFetch, | |||
| useExpiredCache: true, | |||
| forceReinitialize: true, | |||
There was a problem hiding this comment.
some of the test cases here could be incorrect. this one:
// Init again with an expired cache so a fetch will kicked off too
// We allow the expired cache, so it will succeed and fetch will be delayed
might be contrary to the singleton approach - or perhaps we need to check more conditions than just initialized
There was a problem hiding this comment.
I think we were using multiple initializations as a hack to check populating and then using the cache. If we want, we could rework tests to check cache contents and set up an already-populated cache another way but I think this is probably fine for now.
leoromanovsky
requested review from
felipecsl,
aarsilv,
greghuels and
sameerank
| assignmentLogger: mockLogger, | ||
| }); | ||
|
|
||
| expect(callCount).toBe(1); |
src/index.ts
Outdated
| if (EppoJSClient.initialized) { | ||
| if (forceReinitialize) { | ||
| applicationLogger.warn( | ||
| 'Eppo SDK is already initialized. Since forceReinitialize is true, reinitializing.', |
There was a problem hiding this comment.
i'd change this to an info instead of a warn. also minor rewording: Eppo SDK is already initialized. Reinitializing since forceReinitialize is true.
There was a problem hiding this comment.
TBH none of these logs are getting emitted anyways since this is the browser 😄
| @@ -191,6 +191,7 @@ describe('EppoJSClient E2E test', () => { | |||
| apiKey, | |||
| baseUrl, | |||
| assignmentLogger: mockLogger, | |||
| forceReinitialize: true, | |||
There was a problem hiding this comment.
I think it's nice to be able to force re-initialize, for tests and possibly other applications
| forceReinitialize: true, | ||
| }); | ||
|
|
||
| expect(callCount).toBe(2); |
| @@ -871,6 +932,7 @@ describe('initialization options', () => { | |||
| assignmentLogger: mockLogger, | |||
| updateOnFetch, | |||
| useExpiredCache: true, | |||
| forceReinitialize: true, | |||
There was a problem hiding this comment.
I think we were using multiple initializations as a hack to check populating and then using the cache. If we want, we could rework tests to check cache contents and set up an already-populated cache another way but I think this is probably fine for now.
…has already been init] (FF-2921)
labels: mergeable
Fixes: #issue
Motivation and Context
If a user calls
initrepeatedly, the client will issue multiple network requests. This is not desirable behavior as the configuration and assignment cache are also reset.To support users explicitly reloading configuration we should offer a dedicated refetch method.
Description
initmethod completing (not taking into account theAsyncStoreor others), return it immediately without re-instantiating.forceReinitialize- setting totruere-instantiates the client. I'm not convinced this is needed for users but is useful for tests.3.5.0since this is new functionality - you can argue that functionality is "breaking" but I view this as a correction. We could even argue to publish it as a bug fix.How has this been tested?
Question for reviewers