fix: feature flags for person and groups and reload flags

[marandaneto]

💡 Motivation and Context

port fixes from PostHog/posthog-android#315

💚 How did you test it?

📝 Checklist

• I reviewed the submitted code.
• I added tests to verify the changes.
• I updated the docs if needed.
• No breaking change or entry added to the changelog.

[marandaneto]

to avoid name clash with the new method

[marandaneto]

people might be calling resetPersonPropertiesForFlags and then reloadFeatureFlags
2 options:
we default reloadFeatureFlags to false to avoid calling reloadFeatureFlags twice
we let it as is since the loadingFeatureFlagsLock and loadingFeatureFlags will do the job and bail out immediatelly on the 2nd call
i chose the later, let me know if thats not ideal.

[ioannisj]

Yeah the only problem I see is that the reloadFeatureFlags callback will never be called.

Maybe not for this PR but we need a way to save the callback if there's another reload request in-flight and call it once the original request finishes

[marandaneto]

not sure i follow, which callback?

[marandaneto]

this is similar to call identify, we just call reloadFeatureFlags and dont allow the user to pass a callback unless they want to relaod it manually passing a callback

[marandaneto]

if thats the case, they could call the method with reloadFeatureFlags: false and then manually reloadFeatureFlags(...)

[marandaneto]

I wonder if we should add a bool parameters to reloadFeatureFlags then that will reset person or group parameters

not sure i understand, mind clarifying?

[ioannisj]

What I mean is maybe we can give people a way to do:

PostHog.shared.reloadFeatureFlags(
  resetPersonProperties: true
) {
  // completion handler
}

Instead of them calling resetPersonPropertiesForFlags and then reloadFeatureFlags

[marandaneto]

i dont think thats a good API
there are so many reasons to reload the flags, in this case the reloadFeatureFlags method would have a bunch of things (reset groups, reset all, etc).
its a much more natural API to eg
resetPersonPropertiesForFlags and flags are automatically reloaded unless opt out
or resetPersonPropertiesForFlags never reloads and the user has to reload manually, it seems a much better UX IMO
its the intention and the side effect and not the other way around if that makes sense

[marandaneto]

i will merge to unblock the fix

[ioannisj]

Yeap agreed. I think a future improvement is saving the callbacks from reloadFeatureFlags calls in memory and calling them whenever flags reload (for whatever reason) so that we make sure that the logic that people attach there always gets executed

[ioannisj]

Lg left a few comments from discussion

[ioannisj]

people might be calling resetPersonPropertiesForFlags and then reloadFeatureFlags

So if they call reloadFeatureFlags with a callback the code will bail out because there's another call to flags by resetPersonPropertiesForFlags right?

[ioannisj]

I wonder if we should add a bool parameters to reloadFeatureFlags then that will reset person or group parameters

[dustinbyrne]

Beat me to it! 😄

The thread lock on reload preventing two /flags calls sounds totally reasonable.