Skip to content

Commit 5bb710f

Browse files
zashraf1985zashraf1985
authored
fix: Fixed redundant re-rendering issues (#125)
## Summary Made the following re-rendering fixes. 1. When SDK is initialized using a datafile, hooks were doing an empty first render and the real decision was returned on the second render. Fixed the SDK to render the correct decision on first render when initialized synchronously. 2. When SDK is initialized with datafile and SDK key both, The hook was being evaluated three times instead of two. Fixed the redundant re-rendering in this case. Also checked the SDK thoroughly in many combination of sync/async intialization parameters and made fixes where needed. ## Test Plan 1. All unit tests pass. 2. Tested all the hooks and components thoroughly with hard coded data file, SDK key and both along with async/sync user initialzation and autoUpdate true/false.
1 parent d370252 commit 5bb710f

File tree

5 files changed

+97
-31
lines changed

5 files changed

+97
-31
lines changed

‎src/Experiment.spec.tsx

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
/// <reference types="jest" />
1717
import * as React from 'react';
1818
import * as Enzyme from 'enzyme';
19+
import { act } from 'react-dom/test-utils';
1920
import Adapter from 'enzyme-adapter-react-16';
2021
Enzyme.configure({ adapter: new Adapter() });
2122

@@ -30,8 +31,10 @@ describe('<OptimizelyExperiment>', () => {
3031
const variationKey = 'variationResult';
3132
let resolver: any;
3233
let optimizelyMock: ReactSDKClient;
34+
let isReady: boolean;
3335

3436
beforeEach(() => {
37+
isReady = false;
3538
const onReadyPromise = new Promise((resolve, reject) => {
3639
resolver = {
3740
reject,
@@ -51,7 +54,9 @@ describe('<OptimizelyExperiment>', () => {
5154
id: 'testuser',
5255
attributes: {},
5356
},
54-
isReady: jest.fn().mockReturnValue(false),
57+
isReady: jest.fn().mockImplementation(() => isReady),
58+
getIsReadyPromiseFulfilled: () => true,
59+
getIsUsingSdkKey: () => true,
5560
onForcedVariationsUpdate: jest.fn().mockReturnValue(() => {}),
5661
} as unknown) as ReactSDKClient;
5762
});
@@ -282,8 +287,8 @@ describe('<OptimizelyExperiment>', () => {
282287

283288
// Simulate client becoming ready
284289
resolver.resolve({ success: true });
285-
286-
await optimizelyMock.onReady();
290+
isReady = true;
291+
await act(async () => await optimizelyMock.onReady());
287292

288293
component.update();
289294

@@ -321,8 +326,8 @@ describe('<OptimizelyExperiment>', () => {
321326

322327
// Simulate client becoming ready
323328
resolver.resolve({ success: true });
324-
325-
await optimizelyMock.onReady();
329+
isReady = true;
330+
await act(async () => await optimizelyMock.onReady());
326331

327332
component.update();
328333

‎src/Feature.spec.tsx

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
import * as React from 'react';
1717
import * as Enzyme from 'enzyme';
18+
import { act } from 'react-dom/test-utils';
1819
import Adapter from 'enzyme-adapter-react-16';
1920
Enzyme.configure({ adapter: new Adapter() });
2021

@@ -27,11 +28,13 @@ describe('<OptimizelyFeature>', () => {
2728
let resolver: any;
2829
let optimizelyMock: ReactSDKClient;
2930
const isEnabledMock = true;
31+
let isReady: boolean;
3032
const featureVariables = {
3133
foo: 'bar',
3234
};
3335

3436
beforeEach(() => {
37+
isReady = false;
3538
const onReadyPromise = new Promise((resolve, reject) => {
3639
resolver = {
3740
reject,
@@ -52,7 +55,9 @@ describe('<OptimizelyFeature>', () => {
5255
id: 'testuser',
5356
attributes: {},
5457
},
55-
isReady: jest.fn().mockReturnValue(false),
58+
isReady: jest.fn().mockImplementation(() => isReady),
59+
getIsReadyPromiseFulfilled: () => true,
60+
getIsUsingSdkKey: () => true,
5661
} as unknown) as ReactSDKClient;
5762
});
5863
it('throws an error when not rendered in the context of an OptimizelyProvider', () => {
@@ -209,7 +214,8 @@ describe('<OptimizelyFeature>', () => {
209214
// Simulate client becoming ready
210215
resolver.resolve({ success: true });
211216

212-
await optimizelyMock.onReady();
217+
isReady = true;
218+
await act(async () => await optimizelyMock.onReady());
213219

214220
component.update();
215221

@@ -226,7 +232,7 @@ describe('<OptimizelyFeature>', () => {
226232
}));
227233

228234
const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
229-
updateFn();
235+
act(updateFn);
230236

231237
component.update();
232238

@@ -253,7 +259,8 @@ describe('<OptimizelyFeature>', () => {
253259
// Simulate client becoming ready
254260
resolver.resolve({ success: true });
255261

256-
await optimizelyMock.onReady();
262+
isReady = true;
263+
await act(async () => await optimizelyMock.onReady());
257264

258265
component.update();
259266

@@ -269,7 +276,7 @@ describe('<OptimizelyFeature>', () => {
269276
foo: 'baz',
270277
}));
271278

272-
updateFn();
279+
act(updateFn);
273280

274281
component.update();
275282

‎src/client.ts

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,8 @@ export interface ReactSDKClient extends Omit<optimizely.Client, 'createUserConte
4646
setUser(userInfo: UserInfo): void;
4747
onUserUpdate(handler: OnUserUpdateHandler): DisposeFn;
4848
isReady(): boolean;
49+
getIsReadyPromiseFulfilled(): boolean;
50+
getIsUsingSdkKey(): boolean;
4951

5052
activate(
5153
experimentKey: string,
@@ -165,27 +167,37 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
165167
id: null,
166168
attributes: {},
167169
};
168-
private userPromiseResovler: (user: UserInfo) => void;
170+
private userPromiseResolver: (user: UserInfo) => void;
169171
private userPromise: Promise<OnReadyResult>;
170172
private isUserPromiseResolved = false;
171173
private onUserUpdateHandlers: OnUserUpdateHandler[] = [];
172174
private onForcedVariationsUpdateHandlers: OnForcedVariationsUpdateHandler[] = [];
173175

176+
// Is the javascript SDK instance ready.
177+
private isClientReady: boolean = false;
178+
179+
// We need to add autoupdate listener to the hooks after the instance became fully ready to avoid redundant updates to hooks
180+
private isReadyPromiseFulfilled: boolean = false;
181+
182+
// Its usually true from the beginning when user is provided as an object in the `OptimizelyProvider`
183+
// This becomes more significant when a promise is provided instead.
184+
private isUserReady: boolean = false;
185+
186+
private isUsingSdkKey: boolean = false;
187+
174188
private readonly _client: optimizely.Client;
175189

176190
// promise keeping track of async requests for initializing client instance
177191
private dataReadyPromise: Promise<OnReadyResult>;
178192

179-
private dataReadyPromiseFulfilled = false;
180-
181193
/**
182194
* Creates an instance of OptimizelyReactSDKClient.
183195
* @param {optimizely.Config} [config={}]
184196
*/
185197
constructor(config: optimizely.Config) {
186198
this.initialConfig = config;
187199

188-
this.userPromiseResovler = () => {};
200+
this.userPromiseResolver = () => {};
189201

190202
const configWithClientInfo = {
191203
...config,
@@ -194,19 +206,39 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
194206
};
195207
this._client = optimizely.createInstance(configWithClientInfo);
196208

209+
this.isClientReady = !!configWithClientInfo.datafile;
210+
this.isUsingSdkKey = !!configWithClientInfo.sdkKey;
211+
197212
this.userPromise = new Promise(resolve => {
198-
this.userPromiseResovler = resolve;
199-
}).then(() => ({ success: true }));
213+
this.userPromiseResolver = resolve;
214+
}).then(() => {
215+
this.isUserReady = true;
216+
return { success: true }
217+
});
218+
219+
this._client.onReady().then(() => {
220+
this.isClientReady = true;
221+
});
200222

201223
this.dataReadyPromise = Promise.all([this.userPromise, this._client.onReady()]).then(() => {
202-
this.dataReadyPromiseFulfilled = true;
224+
225+
// Client and user can become ready synchronously and/or asynchronously. This flag specifically indicates that they became ready asynchronously.
226+
this.isReadyPromiseFulfilled = true;
203227
return {
204228
success: true,
205229
reason: 'datafile and user resolved',
206230
};
207231
});
208232
}
209233

234+
getIsReadyPromiseFulfilled(): boolean {
235+
return this.isReadyPromiseFulfilled;
236+
}
237+
238+
getIsUsingSdkKey(): boolean {
239+
return this.isUsingSdkKey;
240+
}
241+
210242
onReady(config: { timeout?: number } = {}): Promise<OnReadyResult> {
211243
let timeoutId: number | undefined;
212244
let timeout: number = DEFAULT_ON_READY_TIMEOUT;
@@ -235,12 +267,13 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
235267
// TODO add check for valid user
236268
if (userInfo.id) {
237269
this.user.id = userInfo.id;
270+
this.isUserReady = true;
238271
}
239272
if (userInfo.attributes) {
240273
this.user.attributes = userInfo.attributes;
241274
}
242275
if (!this.isUserPromiseResolved) {
243-
this.userPromiseResovler(this.user);
276+
this.userPromiseResolver(this.user);
244277
this.isUserPromiseResolved = true;
245278
}
246279
this.onUserUpdateHandlers.forEach(handler => handler(this.user));
@@ -275,7 +308,8 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
275308
}
276309

277310
isReady(): boolean {
278-
return this.dataReadyPromiseFulfilled;
311+
// React SDK Instance only becomes ready when both JS SDK client and the user info is ready.
312+
return this.isUserReady && this.isClientReady;
279313
}
280314

281315
/**

‎src/hooks.spec.tsx

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,8 @@ describe('hooks', () => {
118118
attributes: {},
119119
},
120120
isReady: () => readySuccess,
121+
getIsReadyPromiseFulfilled: () => true,
122+
getIsUsingSdkKey: () => true,
121123
onForcedVariationsUpdate: jest.fn().mockImplementation(handler => {
122124
forcedVariationUpdateCallbacks.push(handler);
123125
return () => {};

‎src/hooks.ts

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -210,18 +210,24 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri
210210

211211
const finalReadyTimeout = options.timeout !== undefined ? options.timeout : timeout;
212212
useEffect(() => {
213-
if (!isClientReady) {
213+
// Subscribe to initialzation promise only
214+
// 1. When client is using Sdk Key, which means the initialization will be asynchronous
215+
// and we need to wait for the promise and update decision.
216+
// 2. When client is using datafile only but client is not ready yet which means user
217+
// was provided as a promise and we need to subscribe and wait for user to become available.
218+
if (optimizely.getIsUsingSdkKey() || !isClientReady) {
214219
subscribeToInitialization(optimizely, finalReadyTimeout, initState => {
215220
setState({
216221
...getCurrentDecision(),
217222
...initState,
218223
});
219224
});
220225
}
221-
}, [isClientReady, finalReadyTimeout, getCurrentDecision, optimizely]);
226+
}, []);
222227

223228
useEffect(() => {
224-
if (options.autoUpdate) {
229+
// Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering.
230+
if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) {
225231
return setupAutoUpdateListeners(optimizely, HookType.EXPERIMENT, experimentKey, hooksLogger, () => {
226232
setState(prevState => ({
227233
...prevState,
@@ -230,7 +236,7 @@ export const useExperiment: UseExperiment = (experimentKey, options = {}, overri
230236
});
231237
}
232238
return (): void => {};
233-
}, [isClientReady, options.autoUpdate, optimizely, experimentKey, getCurrentDecision]);
239+
}, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, experimentKey, getCurrentDecision]);
234240

235241
useEffect(
236242
() =>
@@ -297,18 +303,24 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {})
297303

298304
const finalReadyTimeout = options.timeout !== undefined ? options.timeout : timeout;
299305
useEffect(() => {
300-
if (!isClientReady) {
306+
// Subscribe to initialzation promise only
307+
// 1. When client is using Sdk Key, which means the initialization will be asynchronous
308+
// and we need to wait for the promise and update decision.
309+
// 2. When client is using datafile only but client is not ready yet which means user
310+
// was provided as a promise and we need to subscribe and wait for user to become available.
311+
if (optimizely.getIsUsingSdkKey() || !isClientReady) {
301312
subscribeToInitialization(optimizely, finalReadyTimeout, initState => {
302313
setState({
303314
...getCurrentDecision(),
304315
...initState,
305316
});
306317
});
307318
}
308-
}, [isClientReady, finalReadyTimeout, getCurrentDecision, optimizely]);
319+
}, []);
309320

310321
useEffect(() => {
311-
if (options.autoUpdate) {
322+
// Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering.
323+
if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) {
312324
return setupAutoUpdateListeners(optimizely, HookType.FEATURE, featureKey, hooksLogger, () => {
313325
setState(prevState => ({
314326
...prevState,
@@ -317,7 +329,7 @@ export const useFeature: UseFeature = (featureKey, options = {}, overrides = {})
317329
});
318330
}
319331
return (): void => {};
320-
}, [isClientReady, options.autoUpdate, optimizely, featureKey, getCurrentDecision]);
332+
}, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, featureKey, getCurrentDecision]);
321333

322334
return [state.isEnabled, state.variables, state.clientReady, state.didTimeout];
323335
};
@@ -373,18 +385,24 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {})
373385

374386
const finalReadyTimeout = options.timeout !== undefined ? options.timeout : timeout;
375387
useEffect(() => {
376-
if (!isClientReady) {
388+
// Subscribe to initialzation promise only
389+
// 1. When client is using Sdk Key, which means the initialization will be asynchronous
390+
// and we need to wait for the promise and update decision.
391+
// 2. When client is using datafile only but client is not ready yet which means user
392+
// was provided as a promise and we need to subscribe and wait for user to become available.
393+
if (optimizely.getIsUsingSdkKey() || !isClientReady) {
377394
subscribeToInitialization(optimizely, finalReadyTimeout, initState => {
378395
setState({
379396
...getCurrentDecision(),
380397
...initState,
381398
});
382399
});
383400
}
384-
}, [isClientReady, finalReadyTimeout, getCurrentDecision, optimizely]);
401+
}, []);
385402

386403
useEffect(() => {
387-
if (options.autoUpdate) {
404+
// Subscribe to update after first datafile is fetched and readyPromise is resolved to avoid redundant rendering.
405+
if (optimizely.getIsReadyPromiseFulfilled() && options.autoUpdate) {
388406
return setupAutoUpdateListeners(optimizely, HookType.FEATURE, flagKey, hooksLogger, () => {
389407
setState(prevState => ({
390408
...prevState,
@@ -393,7 +411,7 @@ export const useDecision: UseDecision = (flagKey, options = {}, overrides = {})
393411
});
394412
}
395413
return (): void => {};
396-
}, [isClientReady, options.autoUpdate, optimizely, flagKey, getCurrentDecision]);
414+
}, [optimizely.getIsReadyPromiseFulfilled(), options.autoUpdate, optimizely, flagKey, getCurrentDecision]);
397415

398416
return [state.decision, state.clientReady, state.didTimeout];
399417
};

0 commit comments

Comments
 (0)