launchdarkly / node-server-sdk

LaunchDarkly Server-side SDK for Node
Other
79 stars 65 forks source link

`waitForInitialization()` should resolve with the client #106

Closed rmanalan closed 6 years ago

rmanalan commented 6 years ago

It's best to resolve promises with the object that the user expects. This way in an async/await world, the value resolved can be assigned to a var:

const ldclient = await ldClient.waitForInitialization();
console.log(await ldclient.allFlags(<user>));

In the current version, the above code would yield a null or undefined value for ldclient.

apucacao commented 6 years ago

Hi Rich,

I'm curious: since you already have ldClient, do you need to re-assign it (to ldclient)?

rmanalan commented 6 years ago

@apucacao we're using electrolyte for dependency injection and when I bring in the ldclient, it comes in as a promise... like so:

'use strict';

const ldClient = require('ldclient-node');

module.exports = function (ldSettings, logger) {
  const ldclient = ldClient.init(ldSettings.key, { logger });
  // I would use `waitForIntialization()` here since it returns a promise.
  // However, the current version doesn't resolve a value.
  // https://github.com/launchdarkly/node-client/pull/106
  return new Promise(function (resolve, reject) {
    ldclient.once('ready', function () {
      resolve(ldclient);
    });
    ldclient.once('failed', function (err) {
      reject(err);
    });
  });
};

module.exports['@require'] = ['settings/launchdarkly', 'logger/launchdarkly'];
module.exports['@singleton'] = true;

This can then be injected into other modules like so:

const ldclient = await trello.create('launchDarkly'); // returns ldclient

As you can see in the above code, if I'd used waitForIntialization(), the return value from the trello.create() would have been null.

eli-darkly commented 6 years ago

In general I'm somewhat reluctant to make API design decisions based on what would produce the shortest possible code in one particular framework, but making this change can't hurt.

rmanalan commented 6 years ago

@eli-darkly fwiw, waitForInitialization() isn't documented yet. Also, it's generally good practice to resolve a promise with a value.

eli-darkly commented 6 years ago

It was added to the doc page earlier today, and was already present in index.d.ts (where it is documented as returning a Promise\<Void>). I will update that page once the next release goes out.

eli-darkly commented 6 years ago

BTW, I only realized this after adding a unit test, but this PR was slightly incomplete - it would not resolve with a value unless initComplete was already true. But this has been fixed in the release version of the code.

eli-darkly commented 6 years ago

Released in v5.2.0.