fix(hooks): Always recompute decision after resolution of ready promise (#91)

Summary:

Fix the following bug: when client is initialized with datafile, and no SDK key, OptimizelyExperiment and OptimizelyFeature components don't properly render their decisions.

The issue is in the implementation in hooks and the interaction with the client's ready state (via isReady/onReady). When initialized with datafile, the client begins in a non-ready state, but immediately becomes ready on the next tick. But the hook does not recompute the decision when this occurs - it's only recomputing the decision when a CONFIG_UPDATE is fired. CONFIG_UPDATE is never fired when the instance was initialized with datafile only.

To fix, add an effect in the useFeature/useExperiment hooks that recomputes the decision after the client becomes ready.

Test plan:

- Manual testing
- Updated unit tests

Author: mjc1283

src/Experiment.spec.tsx

@@ -76,7 +76,6 @@ describe('<OptimizelyExperiment>', () => {
 
       // 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();
 
@@ -101,7 +100,6 @@ describe('<OptimizelyExperiment>', () => {
 
       // 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();
 
@@ -121,9 +119,8 @@ describe('<OptimizelyExperiment>', () => {
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
 
-      // Simulate client becoming ready: onReady resolving, firing config update notification
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -144,9 +141,8 @@ describe('<OptimizelyExperiment>', () => {
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
 
-      // Simulate client becoming ready: onReady resolving, firing config update notification
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -212,9 +208,8 @@ describe('<OptimizelyExperiment>', () => {
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
 
-      // Simulate client becoming ready: onReady resolving, firing config update notification
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -237,9 +232,8 @@ describe('<OptimizelyExperiment>', () => {
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
 
-      // Simulate client becoming ready: onReady resolving, firing config update notification
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -285,9 +279,8 @@ describe('<OptimizelyExperiment>', () => {
       // while it's waiting for onReady()
       expect(component.text()).toBe('');
 
-      // Simulate client becoming ready: onReady resolving, firing config update notification
+      // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -298,11 +291,11 @@ describe('<OptimizelyExperiment>', () => {
       expect(component.text()).toBe('variationResult');
 
       // capture the OPTIMIZELY_CONFIG_UPDATE function
-      const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
       // change the return value of activate
       const mockActivate = optimizelyMock.activate as jest.Mock;
       mockActivate.mockImplementationOnce(() => 'newVariation');
 
+      const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
       updateFn();
       expect(optimizelyMock.activate).toBeCalledTimes(2);
 
@@ -324,10 +317,9 @@ describe('<OptimizelyExperiment>', () => {
       expect(optimizelyMock.onReady).toHaveBeenCalledWith({ timeout: 100 });
       // while it's waiting for onReady()
       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]();
+      // Simulate client becoming ready
+      resolver.resolve({ success: true });
 
       await optimizelyMock.onReady();
 

src/Feature.spec.tsx

@@ -79,7 +79,6 @@ describe('<OptimizelyFeature>', () => {
 
       // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -106,7 +105,6 @@ describe('<OptimizelyFeature>', () => {
 
       // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -133,7 +131,6 @@ describe('<OptimizelyFeature>', () => {
 
       // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -160,7 +157,6 @@ describe('<OptimizelyFeature>', () => {
 
       // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -185,7 +181,6 @@ describe('<OptimizelyFeature>', () => {
 
       // Simulate client becoming ready
       resolver.resolve({ success: true });
-      (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1]();
 
       await optimizelyMock.onReady();
 
@@ -212,9 +207,7 @@ describe('<OptimizelyFeature>', () => {
         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();
 
@@ -232,6 +225,7 @@ describe('<OptimizelyFeature>', () => {
           foo: 'baz',
         }));
 
+        const updateFn = (optimizelyMock.notificationCenter.addNotificationListener as jest.Mock).mock.calls[0][1];
         updateFn();
 
         component.update();
@@ -257,7 +251,6 @@ describe('<OptimizelyFeature>', () => {
         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();
@@ -304,7 +297,6 @@ describe('<OptimizelyFeature>', () => {
         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/hooks.spec.tsx

@@ -147,20 +147,24 @@ describe('hooks', () => {
     });
 
     it('should respect the timeout option passed', async () => {
-      activateMock.mockReturnValue('12345');
+      activateMock.mockReturnValue(null);
       readySuccess = false;
+
       const component = Enzyme.mount(
         <OptimizelyProvider optimizely={optimizelyMock}>
           <MyExperimentComponent options={{ timeout: mockDelay }} />
         </OptimizelyProvider>
       );
       expect(component.text()).toBe('null|false|false'); // initial render
+
       await optimizelyMock.onReady();
       component.update();
       expect(component.text()).toBe('null|false|true'); // when didTimeout
-      readySuccess = true;
-      // Simulate CONFIG_UPDATE notification, causing decision state to recompute
-      notificationListenerCallbacks[0]();
+
+      // Simulate datafile fetch completing after timeout has already passed
+      // Activate now returns a variation
+      activateMock.mockReturnValue('12345');
+      // Wait for completion of dataReadyPromise
       await optimizelyMock.onReady().then(res => res.dataReadyPromise);
       component.update();
       expect(component.text()).toBe('12345|true|true'); // when clientReady
@@ -243,7 +247,7 @@ describe('hooks', () => {
       expect(mockLog).toHaveBeenCalledWith('12345');
     });
 
-    it('should re-render after the client becomes ready and triggers a config update notification', async () => {
+    it('should re-render after the client becomes ready', async () => {
       readySuccess = false;
       let resolveReadyPromise: (result: { success: boolean; dataReadyPromise: Promise<any> }) => void;
       const readyPromise: Promise<any> = new Promise(res => {
@@ -266,10 +270,14 @@ describe('hooks', () => {
       expect(mockLog).toHaveBeenCalledWith(null);
 
       mockLog.mockReset();
+
+      // Simulate datafile fetch completing after timeout has already passed
+      // Activate now returns a variation
       activateMock.mockReturnValue('12345');
-      // Simulate CONFIG_UPDATE notification, causing decision state to recompute
-      notificationListenerCallbacks[0]();
-      resolveReadyPromise!({ success: true, dataReadyPromise: Promise.resolve() });
+      // Wait for completion of dataReadyPromise
+      const dataReadyPromise = Promise.resolve();
+      resolveReadyPromise!({ success: true, dataReadyPromise });
+      await dataReadyPromise;
       component.update();
 
       expect(mockLog).toHaveBeenCalledTimes(1);
@@ -380,22 +388,36 @@ describe('hooks', () => {
     });
 
     it('should respect the timeout option passed', async () => {
-      isFeatureEnabledMock.mockReturnValue(true);
+      isFeatureEnabledMock.mockReturnValue(false);
+      featureVariables = {};
       readySuccess = false;
+
       const component = Enzyme.mount(
         <OptimizelyProvider optimizely={optimizelyMock}>
           <MyFeatureComponent options={{ timeout: mockDelay }} />
         </OptimizelyProvider>
       );
       expect(component.text()).toBe('false|{}|false|false'); // initial render
+
       await optimizelyMock.onReady();
       component.update();
       expect(component.text()).toBe('false|{}|false|true'); // when didTimeout
-      readySuccess = true;
-      // Simulate CONFIG_UPDATE notification, causing decision state to recompute
-      notificationListenerCallbacks[0]();
+
+      // Simulate datafile fetch completing after timeout has already passed
+      // isFeatureEnabled now returns true, getFeatureVariables returns variable values
+      isFeatureEnabledMock.mockReturnValue(true);
+      featureVariables = mockFeatureVariables;
+      // Wait for completion of dataReadyPromise
       await optimizelyMock.onReady().then(res => res.dataReadyPromise);
       component.update();
+
+      // Simulate datafile fetch completing after timeout has already passed
+      // Activate now returns a variation
+      activateMock.mockReturnValue('12345');
+      // Wait for completion of dataReadyPromise
+      await optimizelyMock.onReady().then(res => res.dataReadyPromise);
+      component.update();
+
       expect(component.text()).toBe('true|{"foo":"bar"}|true|true'); // when clientReady
     });
 
@@ -480,7 +502,7 @@ describe('hooks', () => {
       expect(mockLog).toHaveBeenCalledWith(false);
     });
 
-    it('should re-render after the client becomes ready and triggers a config update notification', async () => {
+    it('should re-render after the client becomes ready', async () => {
       readySuccess = false;
       let resolveReadyPromise: (result: { success: boolean; dataReadyPromise: Promise<any> }) => void;
       const readyPromise: Promise<any> = new Promise(res => {
@@ -503,10 +525,14 @@ describe('hooks', () => {
       expect(mockLog).toHaveBeenCalledWith(false);
 
       mockLog.mockReset();
+
+      // Simulate datafile fetch completing after timeout has already passed
+      // isFeatureEnabled now returns true
       isFeatureEnabledMock.mockReturnValue(true);
-      // Simulate CONFIG_UPDATE notification, causing decision state to recompute
-      notificationListenerCallbacks[0]();
-      resolveReadyPromise!({ success: true, dataReadyPromise: Promise.resolve() });
+      // Wait for completion of dataReadyPromise
+      const dataReadyPromise = Promise.resolve();
+      resolveReadyPromise!({ success: true, dataReadyPromise });
+      await dataReadyPromise;
       component.update();
 
       expect(mockLog).toHaveBeenCalledTimes(1);