fix(hooks): Return latest decision values on first call, and re-render in response to decision input changes (#64)

Summary:

This PR addresses two issues with the useExperiment and useFeature hooks:

- Changes to the overrides argument do not trigger a re-render
- Even after the client instance is ready, the first call returns false (for feature enabled status from useFeature) or null (for variation assignment from useExperiment).

A boolean-returning isReady method is added to ReactSDKClient. When isReady returns true, useFeature and useExperiment don't wait for the fulfillment of the onReady promise before calling their respective decision methods.

Arguments that affect decision results (experiment/feature key + overrides) are tracked in state, so we can compare prior and current values. When values change, the decision is recomputed. I followed the pattern described here, treating the decision state as being derived from the arguments.

The effect that subscribes to updates is also changed to depend on the decision inputs. Since override user attributes are an object, I used a ref combined with a custom equality function to ensure that the dependency array receives a new object reference only when the contents of the attributes are different.

Test Plan:

- Manually tested using an example React app. Checked useFeature and useExperiment return values both before & after client became ready. Checked that useFeature and useExperiment trigger re-renders when entity key or override arguments change.
- Unit tests: updated, and added more

Author: mjc1283

.eslintrc.js

@@ -14,6 +14,7 @@ module.exports = {
     node: true,
   },
   extends: [
+    'plugin:react-hooks/recommended',
     'eslint:recommended',
     'plugin:@typescript-eslint/recommended',
     'prettier/@typescript-eslint',

package.json

@@ -53,6 +53,7 @@
     "eslint-config-prettier": "^6.10.0",
     "eslint-plugin-prettier": "^3.1.2",
     "eslint-plugin-react": "^7.19.0",
+    "eslint-plugin-react-hooks": "^4.1.0",
     "jest": "^23.6.0",
     "prettier": "1.19.1",
     "react": "^16.8.0",

src/Experiment.spec.tsx

@@ -51,6 +51,7 @@ describe('<OptimizelyExperiment>', () => {
         id: 'testuser',
         attributes: {},
       },
+      isReady: jest.fn().mockReturnValue(false),
     } as unknown) as ReactSDKClient;
   });
 
@@ -62,7 +63,7 @@ describe('<OptimizelyExperiment>', () => {
   });
 
   describe('when isServerSide prop is false', () => {
-    it('should wait until onReady() is resolved then render result of activate', async () => {
+    it('should wait client is ready then render result of activate', async () => {
       const component = mount(
         <OptimizelyProvider optimizely={optimizelyMock} timeout={100}>
           <OptimizelyExperiment experiment="experiment1">{variation => variation}</OptimizelyExperiment>
@@ -72,7 +73,10 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 100 });
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -94,7 +98,10 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 200 });
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -113,7 +120,10 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 200 });
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -133,7 +143,10 @@ describe('<OptimizelyExperiment>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -198,7 +211,10 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 100 });
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -220,7 +236,10 @@ describe('<OptimizelyExperiment>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -265,7 +284,10 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 100 });
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready: onReady resolving, firing config update notification
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -304,6 +326,9 @@ describe('<OptimizelyExperiment>', () => {
       expect(component.text()).toBe('');
       resolver.resolve({ success: true });
 
+      // Simulate config update update notification firing after datafile becomes available.
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
+
       await optimizelyMock.onReady();
 
       component.update();

src/Feature.spec.tsx

@@ -52,6 +52,7 @@ describe('<OptimizelyFeature>', () => {
         id: 'testuser',
         attributes: {},
       },
+      isReady: jest.fn().mockReturnValue(false),
     } as unknown) as ReactSDKClient;
   });
   it('throws an error when not rendered in the context of an OptimizelyProvider', () => {
@@ -75,7 +76,10 @@ describe('<OptimizelyFeature>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -99,7 +103,10 @@ describe('<OptimizelyFeature>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -123,7 +130,10 @@ describe('<OptimizelyFeature>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -147,7 +157,10 @@ describe('<OptimizelyFeature>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -169,7 +182,10 @@ describe('<OptimizelyFeature>', () => {
 
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
+
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
+      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -194,7 +210,11 @@ describe('<OptimizelyFeature>', () => {
 
         // while it's waiting for onReady()
         expect(component.text()).toBe('');
+
+        // Simulate client becoming ready
+        const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
         resolver.resolve({ success: true });
+        updateFn();
 
         await optimizelyMock.onReady();
 
@@ -204,7 +224,6 @@ describe('<OptimizelyFeature>', () => {
         expect(optimizelyMock.getFeatureVariables).toHaveBeenCalledWith('feature1', undefined, undefined);
         expect(component.text()).toBe('true|bar');
 
-        const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
         // change the return value of activate
         const mockIFE = optimizelyMock.isFeatureEnabled as jest.Mock;
         mockIFE.mockImplementationOnce(() => false);
@@ -236,6 +255,9 @@ describe('<OptimizelyFeature>', () => {
 
         // while it's waiting for onReady()
         expect(component.text()).toBe('');
+
+        // Simulate client becoming ready
+        (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
         resolver.resolve({ success: true });
 
         await optimizelyMock.onReady();
@@ -281,6 +303,8 @@ describe('<OptimizelyFeature>', () => {
         expect(component.text()).toBe('');
         resolver.resolve({ success: false, reason: 'fail', dataReadyPromise: Promise.resolve() });
 
+        // Simulate config update notification firing after datafile fetched
+        (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
         await optimizelyMock.onReady().then(res => res.dataReadyPromise);
 
         component.update();

src/client.ts

@@ -43,6 +43,7 @@ export interface ReactSDKClient extends optimizely.Client {
   onReady(opts?: { timeout?: number }): Promise<any>;
   setUser(userInfo: { id: string; attributes?: { [key: string]: any } }): void;
   onUserUpdate(handler: OnUserUpdateHandler): DisposeFn;
+  isReady(): boolean;
 
   activate(
     experimentKey: string,
@@ -155,6 +156,8 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
   // promise keeping track of async requests for initializing client instance
   private dataReadyPromise: Promise<OnReadyResult>;
 
+  private dataReadyPromiseFulfilled = false;
+
   /**
    * Creates an instance of OptimizelyReactSDKClient.
    * @param {optimizely.Config} [config={}]
@@ -176,6 +179,7 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
     }).then(() => ({ success: true }));
 
     this.dataReadyPromise = Promise.all([this.userPromise, this._client.onReady()]).then(() => {
+      this.dataReadyPromiseFulfilled = true;
       return {
         success: true,
         reason: 'datafile and user resolved',
@@ -233,6 +237,10 @@ class OptimizelyReactSDKClient implements ReactSDKClient {
     };
   }
 
+  isReady(): boolean {
+    return this.dataReadyPromiseFulfilled;
+  }
+
   /**
    * Buckets visitor and sends impression event to Optimizely
    * @param {string} experimentKey