feat(utils): Add PollingConfigCache and ClientCache #96

spencerwilson-optimizely · 2018-05-14T16:29:03Z

Summary

Add public types PollingConfigCache and ClientCache, both of which are subclasses of a [currently] private type AsyncCache
Move the definition of the createInstance export to lib/optimizely/index.js, in order to resolve a circular-import issue: since ClientCache refers to it, but the entry point modules in turn refer to ClientCache.

How to read this: start with AsyncCache, then move on to the other types. It is a bit like the type exported by the popular async-cache package, but with some differences. This implementation:

Is a class, so can be extended, having methods overridden (like seed, whose semantics are quite different for ClientCache than they are for PollingConfigCache).
Lacks any notions of entry expiration or eviction, since our use case is a small dataset (Full Stack configs). See YAGNI.
Subclasses may spur entry refreshes at their discretion by calling __execRefresh(key).
Supports other JavaScript host environments, like a browser (async-cache depends on process.nextTick).

This AsyncCache is totally generic, so could/should live in another package.

Both the Node.js and browser builds (which reminds me, this PR must not be merged until babel transpilation is introduced) of PollingConfigCache have a functional default means of polling the CDN: request-promise-native and window.fetch, respectively. request-promise-native and its peer dependency "request" are optionalDependencies.

Test plan

Currently some very basic unit tests. Some manual tests have validated basic getAsync use of both types, at-most-one-in-flight behavior, and support for tracking headers so PollingConfigCache can get 304 responses from the CDN.

mikeproeng37

Initial round of feedback. I need to spend more time looking at the cache implementations and maybe play around with them. Will get back to it later this week.

mikeproeng37 · 2018-05-21T17:52:17Z

packages/optimizely-sdk/lib/index.browser.js

 var Optimizely = require('./optimizely');
-
-var MODULE_NAME = 'INDEX';
+var { PollingConfigCache } = require('./utils/config_cache');


Since this is supposed to be an overridable component, I think it should be in the plugins directory as with the rest of the overridable components. Also, I was thinking we can even make this it's own package and later provide an entry point in here that allows users to not include that package (not that we can't do it without making it a separate package). Just drawing inspiration from other open source projects that split up in multiple packages like React and Apollo

mikeproeng37 · 2018-05-21T17:52:47Z

packages/optimizely-sdk/lib/index.browser.js

-
-var MODULE_NAME = 'INDEX';
+var { PollingConfigCache } = require('./utils/config_cache');
+var { ClientCache } = require('./utils/client_cache');

 /**
 * Entry point into the Optimizely Node testing SDK


This comment is no longer valid :)

mikeproeng37 · 2018-05-21T17:53:37Z

packages/optimizely-sdk/lib/index.browser.js

+  return {
+    body: await response.text(),
+    headers: Array.from(response.headers.entries()).reduce((acc, [k, v]) => {
+      acc[k] = v;


Please use proper variable names

mikeproeng37 · 2018-05-21T17:56:16Z

packages/optimizely-sdk/package.json

-  ]
+  ],
+  "optionalDependencies": {
+    "request": "^2.86.0",


@tylerbrandt later introduced this PR that removes this dependency because of vulnerabilities: #98 can we accomplish the same here using just http and https? Perhaps we can even have a promisified wrapper around them

mikeproeng37 · 2018-05-21T17:58:42Z

packages/optimizely-sdk/lib/optimizely/index.js

+  }
+
+  config = fns.assignIn({
+    clientEngine: enums.JAVASCRIPT_CLIENT_ENGINE,


This needs to vary by entrypoint because it lets the backend know whether it is a node or browser client.

mikeproeng37 · 2018-05-21T21:59:08Z

packages/optimizely-sdk/lib/utils/async_cache.js

+   *        Fulfills ASAP in accord with the result of `__onGetAsync` or the given override.
+   *        Rejects on refresh error.
+   */
+  async getAsync(key, override) {


Document override as an optional param. Also, can name it more explicitly, like refreshDirectiveOverride.

mikeproeng37 · 2018-05-21T22:01:46Z

packages/optimizely-sdk/lib/utils/config_cache.js

+/**
+ * A ConfigCache that syncs by polling the CDN.
+ *
+ * Parametrized by which requester to use as a default, since Node.js and browser


Parameterized

mikeproeng37 · 2018-05-21T22:03:35Z

packages/optimizely-sdk/lib/index.browser.js

+/**
+ * The function that PollingConfigCache should use by default to update a config.
+ */
+async function browserRequester(url, headers) {


Maybe move this into utils or even plugins (with the idea that we'll let you configure your own requester in a later version)? Let's try not to bloat the entry point file

mikeproeng37 · 2018-05-21T22:06:42Z

packages/optimizely-sdk/lib/utils/async_cache.js

+        this.__execRefresh(key);
+        return this.get(key);
+
+      case enums.refreshDirectives.YES_AWAIT:


We do need to handle the default case in case of user error and probably default to one of the above and log a warning about it.

mikeproeng37 · 2018-05-21T22:08:59Z

packages/optimizely-sdk/lib/utils/client_cache.js

+   * within this.configCache. _May_ involve losing per-instance state. In particular,
+   * forcedVariations and notificationListeners are lost. TODO: Fix this.
+   */
+  async __refresh(configKey, { client: currentClient, config: currentConfig } = {}) {


please document params

mikeproeng37 · 2019-01-24T22:38:42Z

This is stale, gonna close for now.

spencerwilson-optimizely added 12 commits May 11, 2018 10:49

A few tests and things in place

c97f078

Clarify seed's docstring

f32901d

Delete words

de24a61

s/intervalPeriod/pollPeriod/g

4a9065c

Flesh out __refresh, rename to PollingConfigCache, test seed

b362208

Support conditional requests (i.e., getting 304 responses)

3ee28f7

Implement #on

7fab73c

Split out into LiveCache abstract class

abf2f35

Rename ConfigCache option to onGetAsync

dd140e7

Start tracking client_cache and live_cache

2b70657

Return value/UNCHANGED from __execRefresh

2352327

Rename LiveCache -> AsyncCache

8d153ea

spencerwilson-optimizely changed the title ~~feat(utils): Add ConfigCache and ClientCache~~ feat(utils): Add PollingConfigCache and ClientCache May 14, 2018

spencerwilson-optimizely added 3 commits May 14, 2018 13:17

Require a configCache to make a ClientCache, for PoC

fa29525

Set default requesters for Node.js and browser builds

d74b475

Update comment

b0cf778

spencerwilson-optimizely added the do-not-review label May 15, 2018

Add statusCode to Fetch-based requester

6825f68

spencerwilson-optimizely force-pushed the config-cache branch from 2389a6b to 6825f68 Compare May 15, 2018 20:33

mikeproeng37 suggested changes May 21, 2018

View reviewed changes

mikeproeng37 closed this Jan 24, 2019

mikeproeng37 deleted the config-cache branch February 21, 2019 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(utils): Add PollingConfigCache and ClientCache #96

feat(utils): Add PollingConfigCache and ClientCache #96

Uh oh!

spencerwilson-optimizely commented May 14, 2018 •

edited

Loading

Uh oh!

mikeproeng37 left a comment

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 May 21, 2018

Uh oh!

mikeproeng37 commented Jan 24, 2019

Uh oh!

Uh oh!

feat(utils): Add PollingConfigCache and ClientCache #96

feat(utils): Add PollingConfigCache and ClientCache #96

Uh oh!

Conversation

spencerwilson-optimizely commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

mikeproeng37 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented Jan 24, 2019

Uh oh!

Uh oh!

spencerwilson-optimizely commented May 14, 2018 •

edited

Loading