fix (project config manager & Optimizely): Always fulfill onReady Promise 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

Author: mjc1283

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

@@ -28,6 +28,21 @@ var ERROR_MESSAGES = enums.ERROR_MESSAGES;
 
 var MODULE_NAME = 'PROJECT_CONFIG_MANAGER';
 
+/**
+ * Return an error message derived from a thrown value. If the thrown value is
+ * an error, return the error's message property. Otherwise, return a default
+ * provided by the second argument.
+ * @param {*} maybeError
+ * @param {String=} defaultMessage
+ * @return {String}
+ */
+function getErrorMessage(maybeError, defaultMessage) {
+  if (maybeError instanceof Error) {
+    return maybeError.message;
+  }
+  return defaultMessage || 'Unknown error';
+}
+
 /**
  * ProjectConfigManager provides project config objects via its methods
  * getConfig and onUpdate. It uses a DatafileManager to fetch datafiles. It is
@@ -47,7 +62,10 @@ function ProjectConfigManager(config) {
     logger.error(ex);
     this.__updateListeners = [];
     this.__configObj = null;
-    this.__readyPromise = Promise.reject(ex);
+    this.__readyPromise = Promise.resolve({
+      success: false,
+      reason: getErrorMessage(ex, 'Error in initialize'),
+    });
   }
 }
 
@@ -70,7 +88,10 @@ ProjectConfigManager.prototype.__initialize = function(config) {
   if (!config.datafile && !config.sdkKey) {
     this.__configObj = null;
     var datafileAndSdkKeyMissingError = new Error(sprintf(ERROR_MESSAGES.DATAFILE_AND_SDK_KEY_MISSING, MODULE_NAME));
-    this.__readyPromise = Promise.reject(datafileAndSdkKeyMissingError);
+    this.__readyPromise = Promise.resolve({
+      success: false,
+      reason: getErrorMessage(datafileAndSdkKeyMissingError),
+    });
     logger.error(datafileAndSdkKeyMissingError);
     return;
   }
@@ -101,37 +122,71 @@ ProjectConfigManager.prototype.__initialize = function(config) {
     if (this.__validateDatafileOptions(config.datafileOptions)) {
       fns.assign(datafileManagerConfig, config.datafileOptions);
     }
-    if (initialDatafile) {
+    if (initialDatafile && this.__configObj) {
       datafileManagerConfig.datafile = initialDatafile;
     }
     this.datafileManager = new datafileManager.DatafileManager(datafileManagerConfig);
     this.datafileManager.start();
-    this.__readyPromise = this.datafileManager.onReady().then(this.__onDatafileManagerReady.bind(this));
+    this.__readyPromise = this.datafileManager.onReady().then(
+      this.__onDatafileManagerReadyFulfill.bind(this),
+      this.__onDatafileManagerReadyReject.bind(this)
+    );
     this.datafileManager.on('update', this.__onDatafileManagerUpdate.bind(this));
   } else if (this.__configObj) {
-    this.__readyPromise = Promise.resolve();
+    this.__readyPromise = Promise.resolve({
+      success: true,
+    });
   } else {
-    this.__readyPromise = Promise.reject(projectConfigCreationEx);
+    this.__readyPromise = Promise.resolve({
+      success: false,
+      reason: getErrorMessage(projectConfigCreationEx, 'Invalid datafile'),
+    });
   }
 };
 
 /**
- * Respond to datafile manager's onReady promise. When using a datafile manager,
- * ProjectConfigManager's ready promise is based on DatafileManager's ready
- * promise, and this function is used as a then callback. If this function
- * throws (from validation failures), ProjectConfigManager's ready promise is
- * rejected. Otherwise, ProjectConfigManager updates its own project config
- * object from the new datafile, and calls its own registered update listeners.
+ * Respond to datafile manager's onReady promise becoming fulfilled.
+ * If there are validation or parse failures using the datafile provided by
+ * DatafileManager, ProjectConfigManager's ready promise is resolved with an
+ * unsuccessful result. Otherwise, ProjectConfigManager updates its own project
+ * config object from the new datafile, and its ready promise is resolved with a
+ * successful result.
  */
-ProjectConfigManager.prototype.__onDatafileManagerReady = function() {
+ProjectConfigManager.prototype.__onDatafileManagerReadyFulfill = function() {
   var newDatafile = this.datafileManager.get();
-  var newConfigObj = projectConfig.tryCreatingProjectConfig({
-    datafile: newDatafile,
-    jsonSchemaValidator: this.jsonSchemaValidator,
-    logger: logger,
-    skipJSONValidation: this.skipJSONValidation,
-  });
+  var newConfigObj;
+  try {
+    newConfigObj = projectConfig.tryCreatingProjectConfig({
+      datafile: newDatafile,
+      jsonSchemaValidator: this.jsonSchemaValidator,
+      logger: logger,
+      skipJSONValidation: this.skipJSONValidation,
+    });
+  } catch (ex) {
+    logger.error(ex);
+    return {
+      success: false,
+      reason: getErrorMessage(ex),
+    };
+  }
   this.__handleNewConfigObj(newConfigObj);
+  return {
+    success: true,
+  };
+};
+
+/**
+ * Respond to datafile manager's onReady promise becoming rejected.
+ * When DatafileManager's onReady promise is rejected, there is no possibility
+ * of obtaining a datafile. In this case, ProjectConfigManager's ready promise
+ * is fulfilled with an unsuccessful result.
+ * @param {Error} err
+ */
+ProjectConfigManager.prototype.__onDatafileManagerReadyReject = function(err) {
+  return {
+    success: false,
+    reason: getErrorMessage(err, 'Failed to become ready'),
+  };
 };
 
 /**

packages/optimizely-sdk/lib/core/project_config/project_config_manager.tests.js

@@ -55,19 +55,21 @@ describe('lib/core/project_config/project_config_manager', function() {
     logging.resetLogger();
   });
 
-  it('should throw an error if neither datafile nor sdkKey are passed into the constructor', function(done) {
+  it('should call the error handler and fulfill onReady with an unsuccessful result if neither datafile nor sdkKey are passed into the constructor', function() {
     var manager = new projectConfigManager.ProjectConfigManager({
       skipJSONValidation: true,
     });
     sinon.assert.calledOnce(globalStubErrorHandler.handleError);
     var errorMessage = globalStubErrorHandler.handleError.lastCall.args[0].message;
     assert.strictEqual(errorMessage, sprintf(ERROR_MESSAGES.DATAFILE_AND_SDK_KEY_MISSING, 'PROJECT_CONFIG_MANAGER'));
-    manager.onReady().catch(function() {
-      done();
+    return manager.onReady().then(function(result) {
+      assert.include(result, {
+        success: false,
+      });
     });
   });
 
-  it('should throw an error if the datafile JSON is malformed', function(done) {
+  it('should call the error handler and fulfill onReady with an unsuccessful result if the datafile JSON is malformed', function() {
     var invalidDatafileJSON = 'abc';
     var manager = new projectConfigManager.ProjectConfigManager({
       datafile: invalidDatafileJSON,
@@ -76,12 +78,14 @@ describe('lib/core/project_config/project_config_manager', function() {
     sinon.assert.calledOnce(globalStubErrorHandler.handleError);
     var errorMessage = globalStubErrorHandler.handleError.lastCall.args[0].message;
     assert.strictEqual(errorMessage, sprintf(ERROR_MESSAGES.INVALID_DATAFILE_MALFORMED, 'CONFIG_VALIDATOR'));
-    manager.onReady().catch(function() {
-      done();
+    return manager.onReady().then(function(result) {
+      assert.include(result, {
+        success: false,
+      });
     });
   });
 
-  it('should throw an error if the datafile is not valid', function(done) {
+  it('should call the error handler and fulfill onReady with an unsuccessful result if the datafile is not valid', function() {
     var invalidDatafile = testData.getTestProjectConfig();
     delete invalidDatafile['projectId'];
     var manager = new projectConfigManager.ProjectConfigManager({
@@ -91,21 +95,25 @@ describe('lib/core/project_config/project_config_manager', function() {
     sinon.assert.calledOnce(globalStubErrorHandler.handleError);
     var errorMessage = globalStubErrorHandler.handleError.lastCall.args[0].message;
     assert.strictEqual(errorMessage, sprintf(ERROR_MESSAGES.INVALID_DATAFILE, 'JSON_SCHEMA_VALIDATOR', 'projectId', 'is missing and it is required'));
-    manager.onReady().catch(function() {
-      done();
+    return manager.onReady().then(function(result) {
+      assert.include(result, {
+        success: false,
+      });
     });
   });
 
-  it('should throw an error if the datafile version is not supported', function(done) {
+  it('should call the error handler and fulfill onReady with an unsuccessful result if the datafile version is not supported', function() {
     var manager = new projectConfigManager.ProjectConfigManager({
       datafile: testData.getUnsupportedVersionConfig(),
       jsonSchemaValidator: jsonSchemaValidator,
     });
     sinon.assert.calledOnce(globalStubErrorHandler.handleError);
     var errorMessage = globalStubErrorHandler.handleError.lastCall.args[0].message;
     assert.strictEqual(errorMessage, sprintf(ERROR_MESSAGES.INVALID_DATAFILE_VERSION, 'CONFIG_VALIDATOR', '5'));
-    manager.onReady().catch(function() {
-      done();
+    return manager.onReady().then(function(result) {
+      assert.include(result, {
+        success: false,
+      });
     });
   });
 
@@ -141,7 +149,7 @@ describe('lib/core/project_config/project_config_manager', function() {
     });
   });
 
-  it('should return a valid datafile from getConfig and resolve onReady', function() {
+  it('should return a valid datafile from getConfig and resolve onReady with a successful result', function() {
     var configWithFeatures = testData.getTestProjectConfigWithFeatures();
     var manager = new projectConfigManager.ProjectConfigManager({
       datafile: configWithFeatures,
@@ -150,7 +158,11 @@ describe('lib/core/project_config/project_config_manager', function() {
       manager.getConfig(),
       projectConfig.createProjectConfig(configWithFeatures)
     );
-    return manager.onReady();
+    return manager.onReady().then(function(result) {
+      assert.include(result, {
+        success: true,
+      });
+    });
   });
 
   it('does not call onUpdate listeners after becoming ready when constructed with a valid datafile and without sdkKey', function() {
@@ -185,7 +197,7 @@ describe('lib/core/project_config/project_config_manager', function() {
     });
 
     describe('when constructed with sdkKey and without datafile', function() {
-      it('updates itself when the datafile manager is ready and then emits updates', function() {
+      it('updates itself when the datafile manager is ready, fulfills its onReady promise with a successful result, and then emits updates', function() {
         var configWithFeatures = testData.getTestProjectConfigWithFeatures();
         datafileManager.DatafileManager.returns({
           start: sinon.stub(),
@@ -198,7 +210,10 @@ describe('lib/core/project_config/project_config_manager', function() {
           sdkKey: '12345',
         });
         assert.isNull(manager.getConfig());
-        return manager.onReady().then(function() {
+        return manager.onReady().then(function(result) {
+          assert.include(result, {
+            success: true,
+          });
           assert.deepEqual(
             manager.getConfig(),
             projectConfig.createProjectConfig(configWithFeatures)
@@ -290,7 +305,7 @@ describe('lib/core/project_config/project_config_manager', function() {
         });
       });
 
-      it('rejects its ready promise when the datafile manager emits an invalid datafile', function(done) {
+      it('fulfills its ready promise with an unsuccessful result when the datafile manager emits an invalid datafile', function() {
         var invalidDatafile = testData.getTestProjectConfig();
         delete invalidDatafile['projectId'];
         datafileManager.DatafileManager.returns({
@@ -304,8 +319,29 @@ describe('lib/core/project_config/project_config_manager', function() {
           jsonSchemaValidator: jsonSchemaValidator,
           sdkKey: '12345',
         });
-        manager.onReady().catch(function() {
-          done();
+        return manager.onReady().then(function(result) {
+          assert.include(result, {
+            success: false,
+          });
+        });
+      });
+
+      it('fullfils its ready promise with an unsuccessful result when the datafile manager onReady promise rejects', function() {
+        datafileManager.DatafileManager.returns({
+          start: sinon.stub(),
+          stop: sinon.stub(),
+          get: sinon.stub().returns(null),
+          on: sinon.stub().returns(function() {}),
+          onReady: sinon.stub().returns(Promise.reject(new Error('Failed to become ready')))
+        });
+        var manager = new projectConfigManager.ProjectConfigManager({
+          jsonSchemaValidator: jsonSchemaValidator,
+          sdkKey: '12345',
+        });
+        return manager.onReady().then(function(result) {
+          assert.include(result, {
+            success: false,
+          });
         });
       });
 
@@ -318,8 +354,8 @@ describe('lib/core/project_config/project_config_manager', function() {
       });
     });
 
-    describe('when constructed with sdkKey and with a valid datafile', function() {
-      it('does not call onUpdate listeners after becoming ready', function() {
+    describe('when constructed with sdkKey and with a valid datafile object', function() {
+      it('fulfills its onReady promise with a successful result, and does not call onUpdate listeners after becoming ready', function() {
         datafileManager.DatafileManager.returns({
           start: sinon.stub(),
           stop: sinon.stub(),
@@ -334,7 +370,37 @@ describe('lib/core/project_config/project_config_manager', function() {
         });
         var onUpdateSpy = sinon.spy();
         manager.onUpdate(onUpdateSpy);
-        return manager.onReady().then(function() {
+        return manager.onReady().then(function(result) {
+          assert.include(result, {
+            success: true,
+          });
+          // Datafile is the same as what it was constructed with, so should
+          // not have called update listener
+          sinon.assert.notCalled(onUpdateSpy);
+        });
+      });
+    });
+
+    describe('when constructed with sdkKey and with a valid datafile string', function() {
+      it('fulfills its onReady promise with a successful result, and does not call onUpdate listeners after becoming ready', function() {
+        datafileManager.DatafileManager.returns({
+          start: sinon.stub(),
+          stop: sinon.stub(),
+          get: sinon.stub().returns(testData.getTestProjectConfigWithFeatures()),
+          on: sinon.stub().returns(function() {}),
+          onReady: sinon.stub().returns(Promise.resolve())
+        });
+        var configWithFeatures = testData.getTestProjectConfigWithFeatures();
+        var manager = new projectConfigManager.ProjectConfigManager({
+          datafile: JSON.stringify(configWithFeatures),
+          sdkKey: '12345',
+        });
+        var onUpdateSpy = sinon.spy();
+        manager.onUpdate(onUpdateSpy);
+        return manager.onReady().then(function(result) {
+          assert.include(result, {
+            success: true,
+          });
           // Datafile is the same as what it was constructed with, so should
           // not have called update listener
           sinon.assert.notCalled(onUpdateSpy);

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

@@ -859,14 +859,24 @@ Optimizely.prototype.close = function() {
 };
 
 /**
- * Returns a Promise that resolves when this instance is ready to use (meaning
+ * Returns a Promise that fulfills when this instance is ready to use (meaning
  * it has a valid datafile), or has failed to become ready within a period of
  * time (configurable by the timeout property of the options argument). If a
  * valid datafile was provided in the constructor, the instance is immediately
  * ready. If an sdkKey was provided, a manager will be used to fetch a datafile,
  * and the returned promise will resolve if that fetch succeeds or fails before
  * the timeout. The default timeout is 30 seconds, which will be used if no
  * timeout is provided in the argument options object.
+ * The returned Promise is fulfilled with a result object containing these
+ * properties:
+ *    - success (boolean): True if this instance is ready to use with a valid
+ *                         datafile, or false if this instance failed to become
+ *                         ready.
+ *    - reason (string=):  If success is false, this is a string property with
+ *                         an explanatory message. Failure could be due to
+ *                         expiration of the timeout, network errors,
+ *                         unsuccessful responses, datafile parse errors, or
+ *                         datafile validation errors.
  * @param  {Object=}          options
  * @param  {number|undefined} options.timeout
  * @return {Promise}
@@ -881,7 +891,10 @@ Optimizely.prototype.onReady = function(options) {
   }
   var timeoutPromise = new Promise(function(resolve) {
     setTimeout(function() {
-      resolve();
+      resolve({
+        success: false,
+        reason: sprintf('onReady timeout expired after %s ms', timeout),
+      });
     }, timeout);
   });
   return Promise.race([this.__readyPromise, timeoutPromise]);