search

Flagsmith / flagsmith-js-client

Javascript Client for Flagsmith. Ship features with confidence using feature flags and remote config. Host yourself or use our hosted version at https://www.flagsmith.com/
https://www.flagsmith.com/
BSD 3-Clause "New" or "Revised" License
60 stars 38 forks source link

Clear cache when calling logout() #175

Closed strugee closed 1 year ago

strugee commented 1 year ago

I'm currently integrating Flagsmith into my company's frontend React app. I wanted to automatically identify users to Flagsmith when they log in, and subsequently unidentify them with logout() when they log out. However, I discovered that in order to log users out, our app currently does a request to the backend and then does window.location.replace('/'), which triggers an actual page navigation event (as opposed to a pushState() React routing event). Importantly, we also have local storage caching enabled.

The problem is that this races the request kicked off by getFlags() in the logout method. This means that (if the right request wins the race):

  1. Our app kicks off the backend request to log the user out.
  2. logout() gets called.
  3. The Flagsmith SDK kicks off a request to refresh the flags.
  4. The backend request from step 1 completes and, in the same tick, a page navigation is triggered. This navigation event aborts the request from step 3.
  5. The page navigation completes and the frontend app is booted again to render the homepage. Flagsmith loads and uses cached flag values which are now incorrect because they were originally retrieved when the user was identified to Flagsmith.

As such it would be nice if logout() could automatically invalidate these cache entries - it's difficult to imagine a use case where you'd want to keep them (maybe if you just never used segment overrides or something like that I guess?) but if there's concerns there could also be just a "clear cache" method so the SDK consumer could manually do this themselves.

Note that this isn't a priority for us since we've determined that for our particular use case this doesn't matter, but it does seem suboptimal!

kyle-ssg commented 1 year ago

Hi @strugee, your usecase makes sense, the overall idea of logout is that storage is naturally replaced by the results of getFlags. Logout can clear storage however it is worth noting that cross platform storage is async.

It sounds like regardless, the solution here is to make logout async. Would your case be resolved if you could await flagsmith.logout and it resolves when the cache is populated by environment flags? This would mean the next session would not have to await an initial getFlags when cache is enabled.

strugee commented 1 year ago

Sorry for the delay here - I totally thought I responded. Yeah, that sounds like it would resolve my use case!

kyle-ssg commented 1 year ago

Hi, actually looking at the SDK what I said is already true, flagsmith.logout is async as defined by the types and awaits flagsmith.getFlags() https://github.com/Flagsmith/flagsmith-js-client/blob/main/flagsmith-core.ts#L690

Please feel free to reopen if awaiting logout doesn't work for you

strugee commented 1 year ago

The problem with awaiting logout() is that it incurs an additional request/response cycle of latency to the logout action, whereas otherwise the page would just immediately refresh and render the logged-out view.

But again, quoting from the description:

Note that this isn't a priority for us since we've determined that for our particular use case this doesn't matter, but it does seem suboptimal!

strugee commented 1 year ago

I guess if no one else has complained it's probably fine 🤷