search

launchdarkly / js-sdk-common

Code shared between all LaunchDarkly client-side JS-based SDKs
Other
4 stars 27 forks source link

Re-bootstrap on identify #64

Open jscheid opened 2 years ago

jscheid commented 2 years ago

Is your feature request related to a problem? Please describe.

When calling identify and the (server-generated) all_flags_state for the new user is available, I'd like these flags to be made available immediately (through React hooks etc.), not wait for a server round-trip.

The same reason as why you support bootstrapping at client initialization, only later in the client lifecycle.

Currently the flag defaults are used until the round-trip to your server completes.

Describe the solution you'd like

A new (optional) argument flags to identify would be great.

Describe alternatives you've considered

I've tried hacking around this in various ways but couldn't find a solution.

eli-darkly commented 2 years ago

I'm not sure I understand the request - it's not clear to me what you mean by "the (server-generated) all_flags_state for the new user is available." Available from where?

It might help if you could provide more of a step-by-step scenario of what you would have the application do to use such a feature.

jscheid commented 2 years ago

Thanks for getting back to me. Sure, this is what it looks like from the application's perspective currently, if I understand correctly:

  1. Initialize and bootstrap LDClient in secure mode for user A a. Thanks to bootstapping, if ldClient.variation("foo") is called now immediately (synchronously) it returns the correct value for user A
  2. After some time, switch to user B a. Fetch user data and secure hash from our server b. Call ldClient.identify(user, hash) c. Your client starts polling your server for updated flags for user B d. Because polling takes a moment, if ldClient.variation("foo") is called now immediately (synchronously) it will still return user A's flags (or maybe default flags, I'm not sure. Anyway, it wouldn't know user B's flags at this point, would it?) e. (Time passes) f. Response for request in step (d) returns and now user B's flags are available

Since we are talking to our server in step 2.a anyway, and flag evaluation on the server is fast because your server SDK has the full rule set, we might as well get user B's flags at this point (by evaluating all_flags_state on the server). We could then pass those into the call to identify in step 2.b, just as we did for user A during bootstrap, and that way the call to variation in step 2.d would return the correct flags for user B.

Does this make more sense? Am I missing something obvious?

eli-darkly commented 2 years ago

I see. I wasn't clear on the part where you're making a call to your server, but it makes sense that you would need to do that if a secure hash is involved.

So, you're right that currently there's no way to do this. It's a somewhat atypical use case so we would need to think about a non-confusing way to describe this optional behavior in the API docs, but it could be a desirable feature. I'll discuss it with the team.

In the meantime, just to clarify one particular point...

d. Because polling takes a moment, if ldClient.variation("foo") is called now immediately (synchronously) it will still return user A's flags

That's true, but it's unclear to me whether you're saying you need to be able to call it immediately. I mean, I understand the desire to make things faster by not doing an HTTP request here if you can avoid it. But, for the specific concern of "how do I know when a call to variation will give me the new values instead of the old ones", there is already a defined behavior for that: the identify call is an asynchronous operation that is considered "done" as soon as the new flags have been received.

That's why it's defined (see API docs) as returning a Promise or optionally taking an onDone callback. In other words, if you do either of the following, then the variation call is guaranteed to return the value for the new user:

  ldClient.identify(user, hash, () => {
    const newValue = ldClient.variation("foo");
    // do something with this value
  });

  // or:
  await ldClient.identify(user, hash);
  const newValue = ldClient.variation("foo");
  // do something with this value

That may not be practical in your application, but I wanted to clarify what the available semantics are.

jscheid commented 2 years ago

Thanks for your thoughtful response.

I understand that we can wait for the promise but would like to avoid either of the following two issues if possible:

We can either wait for the promise and have the first issue, or not wait for it and have the second. The ability to re-bootstrap would avoid both. I hope this answers your question?

I'm a little surprised you consider this an atypical use case. Wouldn't other users of the identify call have similar concerns?

eli-darkly commented 2 years ago

Well, that is why I said "I understand the desire to make things faster by not doing an HTTP request here if you can avoid it." Again, the second part of my response was only intended to clarify how identify works.

I'm a little surprised you consider this an atypical use case. Wouldn't other users of the identify call have similar concerns?

Not everyone uses secure mode, and not everyone tries to re-generate flag values in a server-side endpoint when switching users. I'm not saying there's anything wrong with your use case, just that there's more than one way to use identify and we don't want to make people think they need to pre-compute the flags.

jscheid commented 2 years ago

Ah ok, gotcha. But wouldn't this be an improvement for people not using secure mode as well?

eli-darkly commented 2 years ago

It might be something they want to do, and it might not. It is certainly not something people need to do, and it's not what people are doing now. So, again, we will just need to think about how to document it clearly. That is really all I'm saying. I've tried to make it clear that I think this is a reasonable feature request, but please understand that we do not want to just unexpectedly change how people must use the SDK, because we have many existing customers and they have many different ways they would like to do things. I'd like to move on from that point now if possible.

eli-darkly commented 2 years ago

Also— again, just to clarify how the SDK works, not to say that your desire to make things faster is unreasonable— I'd like to address this part:

Having a period of time, no matter how short, during which flags are wrong. In a benign case this can cause flicker (going by an example you're using somewhere in the docs) but it can also mean, for example, doing some more expensive operation needlessly, such as loading an otherwise unneeded async Webpack chunk or setting up comms with a third party service only to tear it down again.

With any operation in JavaScript code that involves I/O, there is always some period during which you don't yet have the new information. Even in your suggested scenario where you're precomputing the flags on the back end, making that request to the back end is an asynchronous operation. So there is inherently, no matter what, going to be "a period of time during which flags are wrong." It may be possible to make that a shorter period of time by only calling your own endpoint, but that's not going to get rid of potential issues like you just mentioned (flicker, loading something unnecessary, etc.). Those are just matters of correctly designing your code with asyncness in mind, so that you are choosing to do those things at a time when it makes sense to do them, regardless of whether that means "after I've called my endpoint and then called identify and waited for identify to complete", or just "after I've called my endpoint and then called identify". I hope that makes sense.

jscheid commented 2 years ago

I'd like to move on from that point now if possible.

Yep let's move on. But just to be clear I'm definitely not suggesting changing any defaults or forcing any new behavior on your other users. An optional new parameter for identify is all we'd need.

So there is inherently, no matter what, going to be "a period of time during which flags are wrong."

Sure. Thanks for clarifying. I'm just trying to avoid adding latency to the authentication process needlessly.

louis-launchdarkly commented 2 years ago

Hello @jscheid, we have discussed this issue and think this request is an enhancement. We have put this onto our backlog. We will provide another update when we start coding the change.

Filed internally as 151055