launchdarkly / node-server-sdk

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

Why does the variation api have to be async? #125

Closed davidlivingrooms closed 5 years ago

davidlivingrooms commented 5 years ago

Hi, this is more of a question. I noticed that other SDKs are synchronous when calling .variation() including the client side javascript library. Is there a functional reason the node version has to be async?

I would like to be able to use the node client like this instead.

client.on('ready', function() {
 console.log("It's now safe to request feature flags");
 var showFeature = client.variation("YOUR_FEATURE_KEY", false);

 if (showFeature) {
   ...
 } else {
   ...
 }
  });

Having thevariation API be async and forcing usage of a callback and promise makes it harder to adopt in my opinion.

eli-darkly commented 5 years ago

The reason is that calling variation may involve doing some I/O. Before evaluating a flag, the client has to get it out of the feature store that holds all the latest known flag data. The default implementation of the feature store is just an in-memory key-value map, but there is also a Redis implementation, and reading from Redis is an async operation - therefore the feature store interface has to support that.

eli-darkly commented 5 years ago

And the reason that that's not the case in the client-side JS SDK is that it always stores its flag data in memory, so it never needs to do I/O for variation.

davidlivingrooms commented 5 years ago

I see, so it’s easier to make it all async even though the in-memory version could technically be synchronous.

Well, it does make it a bit harder in my opinion but it’s really not such a big deal. Looking forward to testing out launchdarkly :). Thanks for your answer.

davidlivingrooms commented 5 years ago

Would it make sense to ever split these out into two apis? So you would call

.variationAsync() when using a store that requires IO. It would take a callback or return a promise.

or just

.variation() for a synchronous version if using the in memory store like most other SDKs. This is likely the large majority of use-cases so giving this synchronous option would make it easier to adopt and pitch to teammates :). Interested in your opinions, thanks.

eli-darkly commented 5 years ago

Well, we can't change the name of the existing async method without breaking things for existing users, but as far adding another method— what I would worry about is that then the logic that uses the service would be very closely tied to the logic that configures the service, that is, if you started developing with an in-memory store and then wanted to migrate to using a Redis store, you would have to rewrite everything quite a bit. But we'll consider it.