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

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

Closed
davidlivingrooms opened this issue Oct 24, 2018 · 5 comments
Closed

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

davidlivingrooms opened this issue Oct 24, 2018 · 5 comments

Comments

@davidlivingrooms
Copy link

davidlivingrooms commented Oct 24, 2018

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

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

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

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

davidlivingrooms commented Oct 24, 2018

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

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.

eli-darkly added a commit that referenced this issue Feb 22, 2019
bump jest version to stop vulnerability warning
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants