launchdarkly / react-client-sdk

LaunchDarkly Client-side SDK for React.js
Other
86 stars 68 forks source link

Flag status is lost during `identify` #60

Closed adamyonk closed 3 years ago

adamyonk commented 3 years ago

Describe the bug If I bootstrap LDProvider with flags and then later identify a user, flags seem to get reverted to their defaults while the identify request is in flight, causing a flash of incorrect flag state.

To reproduce Initialize LaunchDarkly with LDProvider HOC, use useFlags hook in components, later ldClient.identify a user.

Expected behavior The flag state should remain as bootstrapped until the identify request completes.

SDK version 2.21.0

Language version, developer tools TypeScript 4.0.3

OS/platform any

bwoskow-ld commented 3 years ago

Hi @adamyonk,

I'm not quite sure what you're seeing -- here's why:

  1. When bootstrapping values, what you're effectively doing is populating the internal flag store's initial contents. This allows the SDK's internal flag store to return the bootstrapped values when evaluating a flag, until the flag store is updated (step 2).
  2. Upon the SDK getting a response for the initially configured user, the flag store is updated with the user's flag values as known to the LaunchDarkly service. At this point in time the bootstrapped values are no longer used.
  3. At a later time, if/when identify is called, the user and flag store remain unchanged until the LaunchDarkly service returns data for the newly identified user. At that time the process is similar to what is described in step 2.

If you're seeing something different when calling identify, please expand on what you are seeing -- it should be neither bootstrapped values nor default values. Some sample code to reproduce the problem would be great, too.

Thanks, @bwoskow-ld

adamyonk commented 3 years ago

@bwoskow-ld thanks so much for the context! I think this must be what's happening:

  1. We kick off the LDProvider with an anonymous user and bootstrapped flags (from localStorage)
  2. We make a request to get our logged in user's information (this is a static site)
  3. The initial LD identification request for the anonymous user resolves and resets the flags (because some flags are off by default)
  4. The request for user information resolves, then we identify the user with LD and when that identification request resolves the flags return to their correct state

So our flag state is correct between 1 and 3 and between 3 and 4 there is a flash of incorrect flag state. If that is the case, is there some way to "override" the flag state with the bootstrapped values for the user while they are anonymous until they're identified?

eli-darkly commented 3 years ago

(Chiming in here as one of the SDK maintainers) I think there may be some mixed signals here as to what the preconditions are. The initial issue description said "if you bootstrap LDProvider with flags" which I assumed to mean that the code is actually providing flag values via the bootstrap option. But the latest comment mentions "bootstrapped flags (from localStorage)", implying that the bootstrap option is being set to "localstorage" and the code is not providing values. The SDK's behavior in those two modes is fairly different: in particular, the local storage mode makes a follow-up request to LaunchDarkly, which the explicit-values mode does not do (and, in the local storage mode, there's no guarantee that there are any initial values in local storage, whereas in the explicit-values mode there are always initial values).

In the local storage mode, I think the explanation in the latest comment might be correct, but if so there's something going on that doesn't match how I remember the logic. I would have thought that if an identify request were kicked off while a previous request for a different user's flags was still in flight, the previous request should be completely discarded— in other words, identify(user1) followed immediately by identify(user2) should not cause a flash-of-state, and similarly there should not be one in this case if step 2 really did happen before step 3. But I'll have to review the code further to verify that.

(Btw, this is an example of why a code example is generally preferable to a high-level description like "Initialize LaunchDarkly".)

eli-darkly commented 3 years ago

@adamyonk, if it's possible to try a change like this without disrupting your application, I wonder if you could verify whether this still happens if you set the useReport option to true. If it does not still happen, then I bet I know what the issue is and how we should fix it (I think that the "forget about a previous pending request when a new request is made" logic I mentioned above might only work if the requests have the same URL, and in the default HTTP GET mode the request URLs for different users are different, whereas in REPORT mode they're the same).

adamyonk commented 3 years ago

I'm sorry, that was a confusing inclusion of "localStorage"! I meant that we are caching last known flag values manually in localStorage and using that object for bootstrap and NOT the bootstrap: 'localStorage' mode. I added the useReport option and I'm still seeing a flash of incorrect flag state.

const flags = JSON.parse(localStorage.getItem('flags'));
const ldConfig = {
  clientSideID,
  user: {
    anonymous: true,
    key: 'anon',
  },
  reactOptions: {
    useCamelCaseFlagKeys: false,
  },
  options: {
    bootstrap: flags,
    useReport: true,
  },
};

ReactDOM.render(
  <LDProvider {...ldConfig}>
     <App ...
  </LDProvider>,
  document.getElementById('root'),
);

Then later in the application lifecycle, after a user info request resolves, we do:

ldClient?.identify(userInfo)
adamyonk commented 3 years ago

Some more context, if I comment out our eventual ldClient.identify call, the flags are correct momentarily immediately upon page load (I assume because they're using the bootstrapped values), and then they flash to the anonymous state (I assume because the LDProvider sends an identify request for the anonymous user?) and then the flags stay that way (anonymous).

eli-darkly commented 3 years ago

@adamyonk OK, thanks for clarifying. I'd like to double-check to make sure I've got it right: In your earlier comment with the 4 steps of what might be happening, you had step 2 ("We make a request to get our logged in user's information") coming before step 3 ("The initial LD identification request for the anonymous user resolves"). I had assumed that the "request" in step 2 meant an HTTP request to LaunchDarkly for a logged-in user's flags, which would be triggered by an identify call. But now I see that in step 4 you wrote "then we identify the user with LD". So, am I right that whatever kind of request step 2 was talking about, it did not involve any LaunchDarkly calls?

My working theory is now a little different. As I mentioned earlier, if you explicit provide a map of values in the bootstrap option then it is not supposed to make a follow-up call to get values for the user as it would if you did an identify; it should just use the values. However, the default behavior in the React SDK is to enable streaming updates. Streaming updates are obtained through a different mechanism than the polling request that identify uses, and I think it's possible that if you provide bootstrap values and also enable streaming, it does assume that you want a follow-up request to be made.

If that's what's happening... I can see how that wouldn't be desirable in your use case. But I'd have to think about what the logic would need to be to prevent that, while still supporting other people's use cases where they're providing bootstrapped values that match the configured flag default values and they do want to be notified of updates.

I will review the code, but there are two ways to check my theory in the meantime. One is to look at the browser debugger and see if there is an extra HTTP request to a LaunchDarkly endpoint happening prior to your identify call. If there is one, and its URL path starts with /sdx/evalx, then that is a polling request which would mean my theory is wrong. If instead the URL path starts with /eval, then it's a streaming request, which fits my theory. Another way would be to set streaming: false in the SDK options and see if that stops the flash.

eli-darkly commented 3 years ago

Probably the reason this hasn't come up before is that we hadn't really envisioned using the bootstrap option to provide values that deliberately do not correspond to any flag values that would be returned by LaunchDarkly for an anonymous user; in a typical use case, the bootstrap values either match what you expect the default values to be, or are being used in a context where you don't want the front end to make any connection to LD at all (so streaming updates would not be enabled). But it sounds to me like you do want to be able to get updates, once you have identified with a non-anonymous user... right?

adamyonk commented 3 years ago

So, am I right that whatever kind of request step 2 was talking about, it did not involve any LaunchDarkly calls?

Yes that is correct, I mean that we are getting our logged in user info from our database.

check my theory in the meantime

I disabled the identify code so that all that is happening related to LD is I am initializing the LDProvider HOC with the above configuration, and I see these requests matching LaunchDarkly.com:

If I set streaming: false I only see:

And I no longer see the flash.

But it sounds to me like you do want to be able to get updates, once you have identified with a non-anonymous user... right?

Ideally, yes. It seems like if we could wait to turn on streaming until the first identify that would solve our use-case.

adamyonk commented 3 years ago

Would another approach be to cache the user's key in localStorage rather than the user's last known flags, kick off LDProvider with that user key and bootstrap: 'localStorage'? Or is that not how localStorage mode works?

eli-darkly commented 3 years ago

No, that's not how it works. It would expect the flag values to be cached there, from a previous instantiation of the SDK in the same browser for the same user. The assumption in that mode is that the application knows what the user's key is, and that the cached data in local storage is used as the last-known-good values so it can get through startup quickly when it hasn't yet heard back from LaunchDarkly.

eli-darkly commented 3 years ago

It seems like if we could wait to turn on streaming until the first identify that would solve our use-case.

If that's not a hassle then I'd recommend it as the approach that most clearly indicates what you want the SDK's behavior to be.

The React SDK, when it initializes the underlying JS SDK, does not modify the streaming option, so by default that option is unset. The JS SDK's behavior in that case is that it enables streaming if and only if there is a change event listener— and the React SDK provides such a listener, so normally streaming would be on. But if you explicitly set streaming: false, it will not enable streaming until you explicitly tell it to with ldClient.setStreaming(true). At that point, it will start a stream connection to LD based on whatever the user properties currently are. So you would want to do it after the identify has completed (as indicated by a callback or a .then), since until it's received the flag values for the user the SDK doesn't consider the current user to have changed.

eli-darkly commented 3 years ago

Let me know if that makes sense. It may be that the SDK's current behavior is correct but that we should try to address this scenario better in the documentation.

adamyonk commented 3 years ago

@eli-darkly I think that is exactly what we're wanting, thank you for your patience and for helping me to understand that!

We were motivated to use LDProvider over asyncWithLDProvider to start React rendering as soon as possible after page load, but because we're a static site, we don't have any identifying user data until later. It may be something that other folks building a static site run into, so it may be a scenario worth discussing in the docs.