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

Don't call getFlags() if the identity set is the same #177

Closed strugee closed 10 months ago

strugee commented 1 year ago

I want to set up our React app so that I can unconditionally identify to Flagsmith on every authenticated page load. This means that Flagsmith will use the proper identity even if users e.g. refresh the page and therefore enter the app through a route that we wouldn't typically consider an "entry point". The problem with this is that Flagsmith will call getFlags() every time a page navigation occurs, resulting in a lot of unnecessary requests. I could work around this by setting some global state in React, but this is super annoying given the way React thinks about state and I don't really want to bother with it.

It would be much nicer if I could just unconditionally call identify(), on basically every render, and have Flagsmith just do the Right Thing™ and make it a no-op unless the identity actually changed.

dabeeeenster commented 1 year ago

Maybe this should behaviour should be configurable? It would be a major breaking change if we made it default behaviour...

strugee commented 1 year ago

It would be a major breaking change if we made it default behaviour...

Hmm, how so? I mean, I suppose if you're relying on this side effect instead of just calling getFlags() you'd be broken... but why would you do that in the first place?

strugee commented 1 year ago

It's also possible this is the wrong layer to provide this at - it might make more sense to have a wrapper at the Flagsmith React integration layer that used this proposed behavior. The idea being that hey, use this React integration wrapper because it makes it easy to deal with this network side effect thing in the context of a React app.

dabeeeenster commented 1 year ago

It's also possible this is the wrong layer to provide this at - it might make more sense to have a wrapper at the Flagsmith React integration layer that used this proposed behavior. The idea being that hey, use this React integration wrapper because it makes it easy to deal with this network side effect thing in the context of a React app.

Q for @kyle-ssg and @novakzaballa !

kyle-ssg commented 1 year ago

I don't think the React layer is the right place to do it, as the suggested behaviour has nothing to do with React. If we were to make any changes I think it'd be part of the core library.

Making identify skip the API makes sense, the only use-case I think is weird would be if the identity was the same due to local storage cache rather than a previous API response. That's the only part I'd see as a nasty breaking change.

kyle-ssg commented 10 months ago

Reading through this a bit more, I think that we should not pursue with this change. As documented, calling identify fetches flags for the identity and people may expect it to do so regardless of current state.

What's less common is the desired outcome of this ticket, I think this can easily be achieved by wrapping the identify calls in if(flagsmith.identity!=='x')....

If it comes to light that this is a common use case I think we should reconsider of course.