feat: modify datafile managers to use datafile strings instead of objects (#542)

* feat: modify datafile managers to work with datafile string instead of object

* fix: revert/discard unneeded changes that were accidentally pushed

* test: update tests to comply with change in datafile format

* fix: updated overlooked test case to ensure that TestDatafileManager was correctly created with a datafile string

* style: fix linting

* docs: update CHANGELOG unreleased changes

* refactor: modify cache implementations to store values as a string instead of 'any' type and update corresponding tests

* docs: update CHANGELOG unreleased changes to include changes to cache implementation

Author: pthompson127

packages/datafile-manager/CHANGELOG.md

@@ -7,6 +7,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 Changes that have landed but are not yet released.
 
+### Changed
+
+- Modified datafile manager to accept, process, and return the datafile's string representation instead of the datafile object.
+- Remove JSON parsing of response received from datafile fetch request
+  - Responsibility of validating the datafile now solely belongs to the project config manager
+- Modified React Native async storage cache and persistent value cache implementation to store strings instead of objects as values.
+
 ## [0.7.0] - July 28, 2020
 
 ### Changed

packages/datafile-manager/__test__/httpPollingDatafileManager.spec.ts

@@ -69,11 +69,11 @@ class TestDatafileManager extends HttpPollingDatafileManager {
 }
 
 const testCache: PersistentKeyValueCache = {
-  get(key: string): Promise<any | null> {
-    let val = null;
+  get(key: string): Promise<string> {
+    let val = '';
     switch (key) {
       case 'opt-datafile-keyThatExists':
-        val = { name: 'keyThatExists' };
+        val = JSON.stringify({ name: 'keyThatExists' });
         break;
     }
     return Promise.resolve(val);
@@ -109,11 +109,11 @@ describe('httpPollingDatafileManager', () => {
 
   describe('when constructed with sdkKey and datafile and autoUpdate: true,', () => {
     beforeEach(() => {
-      manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: true });
+      manager = new TestDatafileManager({ datafile: JSON.stringify({ foo: 'abcd' }), sdkKey: '123', autoUpdate: true });
     });
 
     it('returns the passed datafile from get', () => {
-      expect(manager.get()).toEqual({ foo: 'abcd' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'abcd' });
     });
 
     it('after being started, fetches the datafile, updates itself, and updates itself again after a timeout', async () => {
@@ -134,28 +134,30 @@ describe('httpPollingDatafileManager', () => {
       manager.start();
       expect(manager.responsePromises.length).toBe(1);
       await manager.responsePromises[0];
-      expect(manager.get()).toEqual({ foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
       updateFn.mockReset();
 
       await advanceTimersByTime(300000);
 
       expect(manager.responsePromises.length).toBe(2);
       await manager.responsePromises[1];
       expect(updateFn).toBeCalledTimes(1);
-      expect(updateFn).toBeCalledWith({
-        datafile: { fooz: 'barz' },
-      });
-      expect(manager.get()).toEqual({ fooz: 'barz' });
+      expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"fooz": "barz"}' });
+      expect(JSON.parse(manager.get())).toEqual({ fooz: 'barz' });
     });
   });
 
   describe('when constructed with sdkKey and datafile and autoUpdate: false,', () => {
     beforeEach(() => {
-      manager = new TestDatafileManager({ datafile: { foo: 'abcd' }, sdkKey: '123', autoUpdate: false });
+      manager = new TestDatafileManager({
+        datafile: JSON.stringify({ foo: 'abcd' }),
+        sdkKey: '123',
+        autoUpdate: false,
+      });
     });
 
     it('returns the passed datafile from get', () => {
-      expect(manager.get()).toEqual({ foo: 'abcd' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'abcd' });
     });
 
     it('after being started, fetches the datafile, updates itself once, but does not schedule a future update', async () => {
@@ -167,7 +169,7 @@ describe('httpPollingDatafileManager', () => {
       manager.start();
       expect(manager.responsePromises.length).toBe(1);
       await manager.responsePromises[0];
-      expect(manager.get()).toEqual({ foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
       expect(getTimerCount()).toBe(0);
     });
   });
@@ -179,7 +181,7 @@ describe('httpPollingDatafileManager', () => {
 
     describe('initial state', () => {
       it('returns null from get before becoming ready', () => {
-        expect(manager.get()).toBeNull();
+        expect(manager.get()).toEqual('');
       });
     });
 
@@ -205,28 +207,7 @@ describe('httpPollingDatafileManager', () => {
         });
         manager.start();
         await manager.onReady();
-        expect(manager.get()).toEqual({ foo: 'bar' });
-      });
-
-      it('does not update if the response body is not valid json', async () => {
-        manager.queuedResponses.push(
-          {
-            statusCode: 200,
-            body: '{"foo" "',
-            headers: {},
-          },
-          {
-            statusCode: 200,
-            body: '{"foo": "bar"}',
-            headers: {},
-          }
-        );
-        manager.start();
-        await manager.onReady();
-        await advanceTimersByTime(1000);
-        expect(manager.responsePromises.length).toBe(2);
-        await manager.responsePromises[1];
-        expect(manager.get()).toEqual({ foo: 'bar' });
+        expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
       });
 
       describe('live updates', () => {
@@ -275,22 +256,22 @@ describe('httpPollingDatafileManager', () => {
 
           manager.start();
           await manager.onReady();
-          expect(manager.get()).toEqual({ foo: 'bar' });
+          expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
           expect(updateFn).toBeCalledTimes(0);
 
           await advanceTimersByTime(1000);
           await manager.responsePromises[1];
           expect(updateFn).toBeCalledTimes(1);
-          expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo2: 'bar2' } });
-          expect(manager.get()).toEqual({ foo2: 'bar2' });
+          expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"foo2": "bar2"}' });
+          expect(JSON.parse(manager.get())).toEqual({ foo2: 'bar2' });
 
           updateFn.mockReset();
 
           await advanceTimersByTime(1000);
           await manager.responsePromises[2];
           expect(updateFn).toBeCalledTimes(1);
-          expect(updateFn.mock.calls[0][0]).toEqual({ datafile: { foo3: 'bar3' } });
-          expect(manager.get()).toEqual({ foo3: 'bar3' });
+          expect(updateFn.mock.calls[0][0]).toEqual({ datafile: '{"foo3": "bar3"}' });
+          expect(JSON.parse(manager.get())).toEqual({ foo3: 'bar3' });
         });
 
         describe('when the update interval time fires before the request is complete', () => {
@@ -351,15 +332,15 @@ describe('httpPollingDatafileManager', () => {
 
           manager.start();
           await manager.onReady();
-          expect(manager.get()).toEqual({ foo: 'bar' });
+          expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
 
           advanceTimersByTime(1000);
 
           expect(manager.responsePromises.length).toBe(2);
           manager.stop();
           await manager.responsePromises[1];
           // Should not have updated datafile since manager was stopped
-          expect(manager.get()).toEqual({ foo: 'bar' });
+          expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
         });
 
         it('calls abort on the current request if there is a current request when stop is called', async () => {
@@ -399,7 +380,7 @@ describe('httpPollingDatafileManager', () => {
           // Trigger the update, should fetch the next response which should succeed, then we get ready
           advanceTimersByTime(1000);
           await manager.onReady();
-          expect(manager.get()).toEqual({ foo: 'bar' });
+          expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
         });
 
         describe('newness checking', () => {
@@ -424,7 +405,7 @@ describe('httpPollingDatafileManager', () => {
 
             manager.start();
             await manager.onReady();
-            expect(manager.get()).toEqual({ foo: 'bar' });
+            expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
             // First response promise was for the initial 200 response
             expect(manager.responsePromises.length).toBe(1);
             // Trigger the queued update
@@ -434,7 +415,7 @@ describe('httpPollingDatafileManager', () => {
             await manager.responsePromises[1];
             // Since the response was 304, updateFn should not have been called
             expect(updateFn).toBeCalledTimes(0);
-            expect(manager.get()).toEqual({ foo: 'bar' });
+            expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
           });
 
           it('sends if-modified-since using the last observed response last-modified', async () => {
@@ -559,7 +540,7 @@ describe('httpPollingDatafileManager', () => {
       });
       manager.start();
       await manager.onReady();
-      expect(manager.get()).toEqual({ foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
     });
 
     it('does not schedule a live update after ready', async () => {
@@ -659,9 +640,9 @@ describe('httpPollingDatafileManager', () => {
       manager.on('update', updateFn);
       manager.start();
       await manager.onReady();
-      expect(manager.get()).toEqual({ name: 'keyThatExists' });
+      expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' });
       await advanceTimersByTime(50);
-      expect(manager.get()).toEqual({ name: 'keyThatExists' });
+      expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' });
       expect(updateFn).toBeCalledTimes(0);
     });
 
@@ -676,10 +657,10 @@ describe('httpPollingDatafileManager', () => {
       manager.on('update', updateFn);
       manager.start();
       await manager.onReady();
-      expect(manager.get()).toEqual({ name: 'keyThatExists' });
+      expect(JSON.parse(manager.get())).toEqual({ name: 'keyThatExists' });
       expect(updateFn).toBeCalledTimes(0);
       await advanceTimersByTime(50);
-      expect(manager.get()).toEqual({ foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
       expect(updateFn).toBeCalledTimes(1);
     });
 
@@ -693,8 +674,9 @@ describe('httpPollingDatafileManager', () => {
       manager.start();
       await manager.onReady();
       await advanceTimersByTime(50);
-      expect(manager.get()).toEqual({ foo: 'bar' });
-      expect(cacheSetSpy).toBeCalledWith('opt-datafile-keyThatExists', { foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
+      expect(cacheSetSpy.mock.calls[0][0]).toEqual('opt-datafile-keyThatExists');
+      expect(JSON.parse(cacheSetSpy.mock.calls[0][1])).toEqual({ foo: 'bar' });
     });
   });
 
@@ -721,7 +703,7 @@ describe('httpPollingDatafileManager', () => {
       manager.start();
       await advanceTimersByTime(50);
       await manager.onReady();
-      expect(manager.get()).toEqual({ foo: 'bar' });
+      expect(JSON.parse(manager.get())).toEqual({ foo: 'bar' });
       expect(updateFn).toBeCalledTimes(0);
     });
   });

packages/datafile-manager/__test__/reactNativeAsyncStorageCache.spec.ts

@@ -24,46 +24,28 @@ describe('reactNativeAsyncStorageCache', () => {
   });
 
   describe('get', function() {
-    it('should return correct object when item is found in cache', function() {
-      return cacheInstance.get('keyThatExists').then(v => expect(v).toEqual({ name: 'Awesome Object' }));
+    it('should return correct string when item is found in cache', function() {
+      return cacheInstance.get('keyThatExists').then(v => expect(JSON.parse(v)).toEqual({ name: 'Awesome Object' }));
     });
 
-    it('should return null if item is not found in cache', function() {
-      return cacheInstance.get('keyThatDoesNotExist').then(v => expect(v).toBeNull());
-    });
-
-    it('should reject promise error if string has an incorrect JSON format', function() {
-      return cacheInstance
-        .get('keyWithInvalidJsonObject')
-        .catch(() => 'exception caught')
-        .then(v => {
-          expect(v).toEqual('exception caught');
-        });
+    it('should return empty string if item is not found in cache', function() {
+      return cacheInstance.get('keyThatDoesNotExist').then(v => expect(v).toEqual(''));
     });
   });
 
   describe('set', function() {
     it('should resolve promise if item was successfully set in the cache', function() {
       const testObj = { name: 'Awesome Object' };
-      return cacheInstance.set('testKey', testObj);
-    });
-
-    it('should reject promise if item was not set in the cache because of json stringifying error', function() {
-      const testObj: any = { name: 'Awesome Object' };
-      testObj.myOwnReference = testObj;
-      return cacheInstance
-        .set('testKey', testObj)
-        .catch(() => 'exception caught')
-        .then(v => expect(v).toEqual('exception caught'));
+      return cacheInstance.set('testKey', JSON.stringify(testObj));
     });
   });
 
   describe('contains', function() {
-    it('should return true if object with key exists', function() {
+    it('should return true if value with key exists', function() {
       return cacheInstance.contains('keyThatExists').then(v => expect(v).toBeTruthy());
     });
 
-    it('should return false if object with key does not exist', function() {
+    it('should return false if value with key does not exist', function() {
       return cacheInstance.contains('keyThatDoesNotExist').then(v => expect(v).toBeFalsy());
     });
   });

packages/datafile-manager/src/datafileManager.ts

@@ -16,7 +16,7 @@
 import PersistentKeyValueCache from './persistentKeyValueCache';
 
 export interface DatafileUpdate {
-  datafile: object;
+  datafile: string;
 }
 
 export interface DatafileUpdateListener {
@@ -31,14 +31,14 @@ interface Managed {
 }
 
 export interface DatafileManager extends Managed {
-  get: () => object | null;
+  get: () => string;
   on: (eventName: string, listener: DatafileUpdateListener) => () => void;
   onReady: () => Promise<void>;
 }
 
 export interface DatafileManagerConfig {
   autoUpdate?: boolean;
-  datafile?: object;
+  datafile?: string;
   sdkKey: string;
   updateInterval?: number;
   urlTemplate?: string;