Skip to content
This repository has been archived by the owner on May 30, 2024. It is now read-only.

waitForInitialization() should resolve with the client #106

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

rmanalan
Copy link
Contributor

@rmanalan rmanalan commented Aug 1, 2018

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
Copy link
Contributor

apucacao commented Aug 1, 2018

Hi Rich,

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

@rmanalan
Copy link
Contributor Author

rmanalan commented Aug 1, 2018

@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
Copy link
Contributor

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.

@eli-darkly eli-darkly merged commit 2c4879d into launchdarkly:master Aug 1, 2018
@rmanalan
Copy link
Contributor Author

rmanalan commented Aug 1, 2018

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

@eli-darkly
Copy link
Contributor

eli-darkly commented Aug 1, 2018

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 eli-darkly mentioned this pull request Aug 1, 2018
@eli-darkly
Copy link
Contributor

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
Copy link
Contributor

Released in v5.2.0.

eli-darkly added a commit that referenced this pull request Nov 14, 2018
 Factor out caching and update queue from redis store for use in dynamo store
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants