feat: Datafile management #261

mjc1283 · 2019-04-24T17:08:28Z

Summary

Add support for datafile management.

Test plan

Unit tests, compatibility suite, manual testing

Issues

https://optimizely.atlassian.net/browse/OASIS-4122

…polling datafile manager (#1) Summary: Add the datafile-management package Add the implementation of HTTPPollingDatafileManager, an abstract class that handles most datafile manager logic Making the GET request and setting the timeout are left as abstract methods - browser and node datafile manager classes will implement these. Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4308

Summary: Update expected return value of abstract method `makeGetRequest`. Should return a request object with an `abort` method. This will be useful in the Node datafile manager for aborting the request on `stop`. Test plan: Updated unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4309

Summary: Adds `NodeDatafileManager` class, extending `HttpPollingDatafileManager` with Node-specific http requesting logic. Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4309

…comes a method (#4)

…t 1 value per header name (#5) Summary: Change `Headers` type to have at-most 1 value per header name. This is sufficient for the current requirements (which only use If-Modified-Since and Last-Modified), and simplifies the code. We can introduce a richer type if we need to deal with multiple values for headers. Test plan: Existing unit tests. This should be a pure refactor with no behavior change.

Summary: Add `BrowserDatafileManager`, similar to `NodeDatafileManager`: a subclass of `HttpPollingDatafileManager` implementing `makeGetRequest` using `XMLHttpRequest`. The caching described in the design is not yet implemented. Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4310

…s or non-success response status (#7) Summary: Implement an increasing delay for the next request after a response Promise is rejected, or resolved with a non-successful status. If the configured updateInterval is greater than the calculated delay, we just use the updateInterval. Test plan: Added unit tests for new module backoffController. Added unit tests for httpPollingDatafileManager backoff functionality.

…ntry point (#8) Summary: This adds several small, but required, fixes: - Update README - Rename liveUpdates to autoUpdate - Increase minimum allowed update interval to 1000 ms - Increase default update interval to 5 minutes - Introduce abstract method for subclasses to provide default config. Use this for autoUpdate default true in node, false in browser - Add top-level entry points for node (index.node.ts) and browser (index.browser.ts) - package.json fixes: Fix license, move @types/node to dev dependencies, add build script, add browser/node entry points - Remove unused DatafileManagerConfig interface properties - Add missing license header in some files Test plan: Updated affected unit tests

…h objects instead of strings. Add StaticDatafileManager. (#9) Summary: - DatafileManager interface uses objects instead of strings - HttpPollingDatafileManager uses JSON.parse on response bodies - Added StaticDatafileManager as a top level export (will be used in optimizely-sdk) Test plan: Updated unit tests & added new unit tests. Issues: https://optimizely.atlassian.net/browse/OASIS-4384

…rom package.json for initial publish

…lable' (#14) Summary: This changes the behavior of the `onReady` Promise to align with the Java implementation. The `onReady` Promise is now resolved as soon as the datafile manager has any datafile (whether it was passed in or fetched). An update event is always emitted when the datafile changes after `onReady`. Test plan: Updated unit tests

… constructor, add onReady method (#13) Summary: - Import and use datafile manager package. Create datafile manager instance in Optimizely constructor if `sdkKey`. is provided. - Provide `onReady` method, returning a promise based on the datafile manager's `onReady` method, or a fulfilled promise if no `sdkKey` was provided. - Update `isValidInstance` to distinguish between config validity and datafile/project config validity. - Move some validation/creation logic into a new function in the project config module: `tryCreatingProjectConfig` - Return early in top-level methods when `configObj` is not available Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4384

…ager options, and stopping datafile manager (#15) Summary: - Refactor project config management into a ProjectConfigManager class, which uses DatafileManager internally - Update Optimizely to get project config by calling projectConfigManager.getConfig() instead of keeping its own reference to a project config object - Pass through datafileOptions to project config manager (allowing use of autoUpdate and updateInterval) - Add notification center event for new project config - Add an update listener to datafile manager that sends project config update notification - Call this.datafileManager.stop in close method - Add default timeout to onReady method - Update behavior of onReady to reject early when datafile manager emits invalid datafile Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4385 https://optimizely.atlassian.net/browse/OASIS-4386 https://optimizely.atlassian.net/browse/OASIS-4387

Summary: - Only assign current datafile if truthy (replace typeof datafile !== 'undefined' check with datafile check) - Change urlTemplate option to be a sprintf-style format string, like Java's withFormat - Change backoff delay to increase up to 512 seconds after 7 errors, and clean up the backoff implementation a little - Clean up url handling in HttpPollingConfigManager. It was saving more private variables than necessary for data that never changes after the constructor. Test plan: Updated unit tests

Summary: Before this change, we were passing the host property returned from url.parse as the host option going into http.request (or https.request). The host property from url.parse includes the port when the URL contains a port. The problem is, request needs port to be passed as a separate option (not included as part of the host option). To fix, stop passing host, and replace it with hostname: url.hostname (where url.hostname excludes port), and port: url.port. References: https://nodejs.org/api/http.html#http_http_request_url_options_callback https://nodejs.org/api/url.html#url_urlobject_host https://nodejs.org/api/url.html#url_urlobject_hostname https://nodejs.org/api/url.html#url_urlobject_port Test plan: Added a regression test. Manually tested in @mikeng13's prototype datafile management branches for JS-testapp and compatibility suite.

…n unit tests (#19) Summary: With changes for datafile management, we introduced Promise rejections that need to be handled. Add handlers to these expected errors. Test plan: Run unit tests with changes from this branch and master branch (which exits the test process on unhandled rejections)

… actually changes (#20) Summary: This includes two updates to project config manager: - When datafile manager provides a new datafile, only update project config object if that object has a different revision that the current config object - Only call update listeners if the config object actually changed With these two changes, we no longer trigger PROJECT_CONFIG_UPDATE notifications unless the project config actually changed. Previously, when constructed with a valid datafile and sdk key, it would trigger two PROJECT_CONFIG_UPDATE update notifications, even though the project config only changed once (after the fetch) Test plan: Updated unit tests

Summary: Fix dispose function returned from onUpdate of ProjectConfigManager. I forgot to bind the dispose function. Test plan: Added new unit test. Manually tested.

…r to Optimizely config instead of project config (#22) Summary: Notification name and log messages refer to "Optimizely config" instead of "project config". Test plan: Updated unit test

…mise with a result object, instead of sometimes rejecting (#23) Summary: When documenting datafile management, I realized the rationale behind the behavior of the promise returned from Optimizely onReady was inconsistent. It would sometimes fulfill even if the instance was still not ready to be used. Also, I thought it would be better to always fulfill because we attempt to never throw errors, and rejected promises are akin to thrown errors. Before this change, the promise would fulfill either when a valid project config was obtained prior to the timeout, or when the timeout expired, and reject in other cases. With this change, the promise always fulfills with a result object of the form { success: boolean, reason?: string }. When success is true, the instance has a valid project config and is ready to use. When success is false, the instance failed to become ready, and reason explains why. Test plan: Updated unit tests

…of project config and into decision service (#24) Summary: This is a solution for persisting forced variation state when the project config is updated from datafile management. Just as in Java, we moved the forced variation map and forced variation methods into decision service. Test plan: - Existing unit tests are passing with no substantial changes: the changes I made in existing unit tests only update log message assertions to reflect the new location of forced variation methods (previously in project config, now in decision service). - I moved forced variation method tests to the decision service tests file, and updated the method signatures to remove the logger argument (decision service already has a reference to its own logger). - I added a few more unit tests covering cases where a previously set variation or experiment no longer exists on a new project config object.

…all fixes (#25) Summary: Previously, if close was called with pending ready timeouts, those timeouts would remain active. With this change, we clear all pending ready timeouts when close is called. To guard against Promises returned from onReady remaining permanently pending, in close, we resolve any ready promises associated with pending timeouts with unsuccessful results. There are two other small changes in this PR: - Improve documentation comments for Optimzely onReady and ProjectConfigManager onReady - Return false from setForcedVariation when no configObj available (was previously returning null) Test plan: Updated unit tests, existing unit tests should continue passing

Summary: There were a few files that were created/modified during development of datafile management that did not get updated license headers. This fixes those files. Test plan: None. Should be comment-only changes

coveralls · 2019-04-24T17:18:19Z

Coverage decreased (-0.2%) to 97.58% when pulling e268ba5 on mcarroll/datafile-manager-public into 55e2628 on master.

coveralls · 2019-04-24T17:18:19Z

Coverage decreased (-0.2%) to 97.58% when pulling e268ba5 on mcarroll/datafile-manager-public into 55e2628 on master.

coveralls · 2019-04-24T17:18:20Z

Coverage decreased (-0.2%) to 97.58% when pulling e268ba5 on mcarroll/datafile-manager-public into 55e2628 on master.

coveralls · 2019-04-24T17:18:21Z

Coverage decreased (-0.2%) to 97.58% when pulling 2d33de8 on mcarroll/datafile-manager-public into 55e2628 on master.

mikeproeng37

Looks good. I have a few requests.

mikeproeng37 · 2019-04-24T21:15:24Z

packages/datafile-manager/package.json

+  "description": "Optimizely Full Stack Datafile Manager",
+  "license": "Apache-2.0",
+  "engines": {
+    "node": ">=4.0.0"


nit: We technically only support Node 6+ so I don't wanna give the false impression that we officially support 4 (even though it would work)

mikeproeng37 · 2019-04-24T21:17:16Z

packages/optimizely-sdk/CHANGELOG.MD

+  ```js
+  const optimizely = require('@optimizely/optimizely-sdk');
+  const optimizelyClientInstance = optimizely.createInstance({
+    sdkKey: ‘12345’, // Provide the sdkKey of your desired environment here


nit: these ticks/apostrophes seem off, is that intentional?

Whoa, good eye. I think it's because I copied from a google doc.

mikeproeng37 · 2019-04-24T21:21:27Z

packages/optimizely-sdk/lib/utils/json_schema_validator/index.tests.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2016-2019, Optimizely


If this file wasn't touched in 2018 it should be 2016-2017, 2019

mikeproeng37 · 2019-04-24T21:21:50Z

packages/optimizely-sdk/lib/core/project_config/project_config_schema.js

@@ -1,5 +1,5 @@
 /**
- * Copyright 2016-2017, Optimizely
+ * Copyright 2016-2019, Optimizely


If this file wasn't touched in 2018 it should be 2016-2017, 2019

This was only moved, but not changed. Should it be left as 2016-2017?

mikeproeng37

lgtm, just have a few minor comments on it

mikeproeng37 · 2019-04-25T21:17:53Z

packages/optimizely-sdk/CHANGELOG.MD

@@ -7,6 +7,57 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 ## [Unreleased]
 Changes that have landed but are not yet released.

+### New Features
+
+- Added support for automatic datafile management


I think we should also mention in the unreleased notes that the forced variation logic has been moved to the decision service module because that affects where those messages are logged via the MODULE_NAME

mikeproeng37 · 2019-04-25T21:19:07Z

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

+   * @param  {Object} config.skipJSONValidation
+   * @return {Object} Project config object
+   */
+  tryCreatingProjectConfig: function(config) {


We can punt this to a later PR, but I think this factory method should live in a factory class or something instead of being in the project config itself

mikeproeng37 · 2019-04-25T21:21:42Z

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

@@ -67,29 +67,23 @@ function Optimizely(config) {
  this.clientVersion = config.clientVersion || enums.NODE_CLIENT_VERSION;
  this.errorHandler = config.errorHandler;
  this.eventDispatcher = config.eventDispatcher;
-  this.isValidInstance = config.isValidInstance;
+  this.__isOptimizelyConfigValid = config.isValidInstance;


nit: I think this property might be useful to developers. Did we consider exposing __isValidInstance? I know that both mobile SDKs make this public because it is used in one of the initialization scenarios

I hadn't considered this. Sounds good to me if there's a use case for it.

mjc1283 and others added 30 commits March 18, 2019 09:56

feat (datafile manager): Node datafile manager (#3)

8135a51

Summary: Adds `NodeDatafileManager` class, extending `HttpPollingDatafileManager` with Node-specific http requesting logic. Test plan: Unit tests Issues: https://optimizely.atlassian.net/browse/OASIS-4309

fix (datafile manager): Update datafile manager interface: onReady be…

2eec4e8

…comes a method (#4)

Merge branch 'master' into devel

82e4d0a

(fix) Fix travis build (#10)

36d5c55

Merge branch 'master' into devel

6a77bb2

Merge branch 'master' into devel

71954e7

Add missing license headers. Move import statement under license header

0ef64db

Add prepublishOnly script, and remove references to public SDK repo f…

d0b3b77

…rom package.json for initial publish

Merge branch 'master' into devel

b53d102

Fix README, remove unused enum

8adfc7d

Merge branch 'master' into devel

29352c2

Update CHANGELOG.md for v0.2.0

b9c026f

js-sdk-datafile-manager v0.2.0

ec261d7

Update js-sdk-datafile-manager version to 0.2.0

e02c569

fix (project config manager): Fix onUpdate dispose function (#21)

6c1b014

Summary: Fix dispose function returned from onUpdate of ProjectConfigManager. I forgot to bind the dispose function. Test plan: Added new unit test. Manually tested.

Fix umd bundle tests

b6884e9

mjc1283 and others added 12 commits April 16, 2019 10:26

fix (config update): Change notification name and log message to refe…

67105ba

…r to Optimizely config instead of project config (#22) Summary: Notification name and log messages refer to "Optimizely config" instead of "project config". Test plan: Updated unit test

Merge branch 'master' into devel

08f451f

ci(tests): Enable FSC tests to run on this repo. (#26)

0f55be2

Merge branch 'master' into devel

bc34102

Remove TODO comment

3b4de0f

Update license headers on modified files (#27)

ceedcd4

Summary: There were a few files that were created/modified during development of datafile management that did not get updated license headers. This fixes those files. Test plan: None. Should be comment-only changes

Restore travis configuration from master

37134f9

Add datafile management entry in CHANGELOG.md

431ac49

Fix code sample in CHANGELOG.md

e268ba5

mjc1283 requested review from jordangarcia, mikeproeng37 and aliabbasrizvi April 24, 2019 17:08

Matt Carroll added 2 commits April 24, 2019 11:10

Add config update notification to changelog entry

cadfd66

Use proper naming conventions in code samples

4a26bec

mikeproeng37 reviewed Apr 24, 2019

View reviewed changes

Matt Carroll added 4 commits April 24, 2019 21:31

Change node engines value in pacakge.json from >=4.0.0 to >=6.0.0

ae1c70a

Fix quotation marks in CHANGELOG.md code samples

fc74699

Undo year update since the file was only moved, not changed

8b0fdd9

Fix example import in datafile-manager README

f40530f

mikeproeng37 approved these changes Apr 25, 2019

View reviewed changes

Add change entry to CHANGELOG about forced variation log messages

2d33de8

mjc1283 merged commit 8c6e955 into master Apr 26, 2019

mjc1283 deleted the mcarroll/datafile-manager-public branch April 26, 2019 17:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Datafile management #261

feat: Datafile management #261

mjc1283 commented Apr 24, 2019

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019 •

edited

mikeproeng37 left a comment

mikeproeng37 Apr 24, 2019

mikeproeng37 Apr 24, 2019

mjc1283 Apr 25, 2019

mikeproeng37 Apr 24, 2019

mikeproeng37 Apr 24, 2019

mjc1283 Apr 25, 2019

mikeproeng37 left a comment

mikeproeng37 Apr 25, 2019

mikeproeng37 Apr 25, 2019

mikeproeng37 Apr 25, 2019

mjc1283 Apr 26, 2019

feat: Datafile management #261

feat: Datafile management #261

Conversation

mjc1283 commented Apr 24, 2019

Summary

Test plan

Issues

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019

coveralls commented Apr 24, 2019 • edited

mikeproeng37 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

Choose a reason for hiding this comment

mikeproeng37 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

coveralls commented Apr 24, 2019 •

edited