feat: React native datafile manager that supports Datafile caching. #430

zashraf1985 · 2020-03-23T06:27:41Z

Summary

This overrides existing HttpPollingDatafileManager to support Datafile Caching for React Native. Browser and Node implementations can also override the same methods to support datafile caching.

Test plan

Unit Tests Pending

coveralls · 2020-03-23T06:39:16Z

Coverage remained the same at 97.222% when pulling b0fd21f on zeeshan/react-native-datafile-manager into 24fe5d6 on master.

mjc1283 · 2020-03-23T20:58:39Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

+      this.hasCachedDatafile()
+        .then((hasCachedDatafile: Boolean) => {
+          if (hasCachedDatafile) {
+            logger.debug('Found datafile in cache')
+            this.getCachedDatafile().then((datafile: any) => {
+              logger.debug('Using datafile from cache')
+              this.currentDatafile = datafile
+              this.resolveReadyPromise()
+            })
+          }
+        })


A few comments on this area:

The presence or absence of datafile should not determine whether caching is in effect; rather it should be sdkKey driving the usage of cache.

We should use the start method to kick off the cache request. Although it's not very consequential given how datafile manager is used within optimizely-sdk, to maintan some internal coherence, I was thinking that start and stop should be controlling all async I/O requests and timers. Prior to start being called, the datafile manager instance is inert: no timers or I/O requests.

Network datafile should always be set after cache datafile, so cache requests need to be coordinated with network requests to avoid race conditions. I would try some refactoring so that the initialization involving both cache and network requests are easier to follow.

mjc1283 · 2020-03-23T22:54:29Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

+
+  // Override in the child class to add cache implementation
+  protected addToCache(datafile: any): Promise<void> {
+    return Promise.resolve()
+  }
+
+  // Override in the child class to add cache implementation
+  protected hasCachedDatafile(): Promise<Boolean> {
+    return Promise.resolve(false)
+  }
+
+  // Override in the child class to add cache implementation
+  protected getCachedDatafile(): Promise<any> {
+    return Promise.resolve()
+  }


Rather than using inheritance and overrides, I suggest we allow passing in a cache object as part of DatafileManagerConfig when constructing HttpPollingDatafileManager. If none is passed in, we can skip the cache reading/writing, or fill in a no-op cache implementation as a default, like you did here.

React Native datafile manager can provide its own default cache object using the getConfigDefaults method override.

mjc1283 · 2020-03-23T22:56:12Z

packages/datafile-manager/src/index.react_native.ts

+
+export * from './datafileManager'
+export { default as HttpPollingDatafileManager } from './reactNativeDatafileManager'
+export { default as StaticDatafileManager } from './staticDatafileManager'


Static datafile manager is not used and actually we should remove it from the codebase (you don't have to do that now). But we should not export it from here, nor create a react native version of it.

mjc1283

Looks good, nice work @zashraf1985. We should add some more unit tests covering the new caching behavior.

mjc1283 · 2020-03-25T20:15:07Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

@@ -91,9 +112,11 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana
      sdkKey,
      updateInterval = DEFAULT_UPDATE_INTERVAL,
      urlTemplate = DEFAULT_URL_TEMPLATE,
+      cache,


Nit: suggest add default here like the other config values:

Suggested change

cache,

cache = new NoOpKeyValueCache()

mjc1283 · 2020-03-25T20:16:33Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

@@ -34,6 +35,24 @@ function isSuccessStatusCode(statusCode: number): boolean {
  return statusCode >= 200 && statusCode < 400
 }

+class NoOpKeyValueCache implements PersistentKeyValueCache {


This can be a single shared object rather than a class, because it has no instance state:

const noOpKeyValueCache: PersistentKeyValueCache { get(...) { ... }, ... };

mjc1283 · 2020-03-27T00:07:36Z

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

-      expect(updateFn).toBeCalledTimes(1)
-      expect(updateFn).toBeCalledWith({
-        datafile: { foo: 'bar' }
-      })


Why is this deleted?

Ok, I see why this behavior changed. We are only emitting update events after ready is resolved. Deleting this is OK.

mjc1283 · 2020-03-27T00:18:05Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

+  setDatafileFromCacheIfAvailable(): void {
+    this.cache.get(this.cacheKey)
+      .then(datafile => {
+        if (!this.isReadyPromiseSettled && datafile) {


The datafile manager can be stopped at any time. When it's stopped, we want to cancel all pending timers, abort requests, and cut short any already-scheduled promise reactions. So let's add a check for this.isStarted in here and return early if it's false (just like other promise reactions in this class).

mjc1283

LGTM, just one comment

mjc1283 · 2020-03-30T19:40:28Z

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

+      await advanceTimersByTime(50)
+      expect(manager.get()).toEqual({name: 'keyThatExists'})
+      expect(updateFn).toBeCalledTimes(0)
+      updateFn.mockReset()


Does calling mockReset matter? updateFn is just thrown away.

you are right. I naively copied without thinking. The places where i copied it from was verifying it twice hence resetting after first use. Pifalls of copy/pasting :)

mjc1283 · 2020-03-30T19:47:06Z

packages/datafile-manager/src/httpPollingDatafileManager.ts

@@ -197,6 +226,8 @@ export default abstract class HttpPollingDatafileManager implements DatafileMana
    if (datafile !== null) {
      logger.info('Updating datafile from response')
      this.currentDatafile = datafile
+      logger.debug('Adding Datafile to Cache')


Let's remove this log message. It will be confusing for users to see this with the no-op cache.

zashraf1985 added 2 commits March 22, 2020 23:20

Added a React native datafile manager that supports caching

b72871f

Added missing file

6eb1773

zashraf1985 marked this pull request as ready for review March 23, 2020 06:39

zashraf1985 requested a review from a team as a code owner March 23, 2020 06:39

optimizely-codereview-bot added the 🤖 4 hrs label Mar 23, 2020

optimizely-codereview-bot requested a review from mjc1283 March 23, 2020 20:04

mjc1283 reviewed Mar 23, 2020

View reviewed changes

zashraf1985 added 3 commits March 25, 2020 12:55

Incorporated review feedback

c5eefdd

Fixed unit tests

8237ae3

minor fix

edf789e

mjc1283 reviewed Mar 27, 2020

View reviewed changes

zashraf1985 added 3 commits March 27, 2020 16:29

incorporated more feedback

8f59ac8

Some tests added. Few more needed

b57991e

Added more unit tests

dbddaee

mjc1283 approved these changes Mar 30, 2020

View reviewed changes

zashraf1985 and others added 2 commits March 30, 2020 13:00

incorporated review feedback

f256afa

Merge branch 'master' into zeeshan/react-native-datafile-manager

b0fd21f

mjc1283 merged commit 645ea8a into master Mar 31, 2020

mjc1283 deleted the zeeshan/react-native-datafile-manager branch March 31, 2020 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: React native datafile manager that supports Datafile caching. #430

feat: React native datafile manager that supports Datafile caching. #430

zashraf1985 commented Mar 23, 2020 •

edited

Loading

coveralls commented Mar 23, 2020 •

edited

Loading

mjc1283 Mar 23, 2020

mjc1283 Mar 23, 2020

mjc1283 Mar 23, 2020

mjc1283 left a comment

mjc1283 Mar 25, 2020

mjc1283 Mar 25, 2020

mjc1283 Mar 27, 2020

mjc1283 Mar 27, 2020

mjc1283 Mar 27, 2020

mjc1283 left a comment

mjc1283 Mar 30, 2020

zashraf1985 Mar 30, 2020

mjc1283 Mar 30, 2020

feat: React native datafile manager that supports Datafile caching. #430

feat: React native datafile manager that supports Datafile caching. #430

Conversation

zashraf1985 commented Mar 23, 2020 • edited Loading

Summary

Test plan

coveralls commented Mar 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjc1283 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zashraf1985 commented Mar 23, 2020 •

edited

Loading

coveralls commented Mar 23, 2020 •

edited

Loading