fix(flagsmith): Fetch identity flags when context changes #1351
Conversation
Signed-off-by: Rodrigo López Dato <rodrigo.lopezdato@flagsmith.com>
Signed-off-by: Rodrigo López Dato <rodrigo.lopezdato@flagsmith.com>
rolodato
force-pushed
the
fix/flagsmith-update-context
branch
from
b6fa0df to
dd18406
Compare
|
One for @kyle-ssg to have a look maybe? I can't assign reviewers for this PR while it's still in draft. |
Signed-off-by: Rodrigo López Dato <rodrigo.lopezdato@flagsmith.com> Co-authored-by: Zaimwa9 <wadii.zaim@flagsmith.com>
rolodato
force-pushed
the
fix/flagsmith-update-context
branch
from
24bec79 to
8ec4423
Compare
|
Hey @rolodato, thanks for the PR. It looks like this PR only includes changes to the docs and a test. If that's intentional, I'm happy to review, but the PR description implies a larger change. |
|
Thanks @beeme1mr - there is actually a behavior change in this PR here, in case you missed it: https://github.com/open-feature/js-sdk-contrib/pull/1351/files#diff-6f058ad4a6f79177acf735cd905843b3a9f0a1f28e1493058d6dc3449a0fd3d2 |
| @@ -52,7 +52,24 @@ export class FlagsmithClientProvider implements Provider { | |||
| ...(context || {}), | |||
| }); | |||
| this.events.emit(ProviderEvents.Stale, { message: 'context has changed' }); | |||
| return isLogout ? this._client.logout() : this._client.getFlags(); | |||
| if (isLogout) { | |||
| return this._client.logout(); | |||
There was a problem hiding this comment.
I see this isn't new behavior but I'm curious why this is necessary. Do you know why the logout would need to be called during initialization?
There was a problem hiding this comment.
It confused me as well for sure. I speculate that this was a shortcut, since this provider implements onContextChange by directly calling initialize: https://github.com/rolodato/js-sdk-contrib/blob/453c70cf829cfee02720e73d78736ddc89d82ce3/libs/providers/flagsmith-client/src/lib/flagsmith-client-provider.ts#L106
A better solution could have been to keep the post-initialization logic in onContextChange, and only have initialization logic in initialize.
After this fix, we'd still need to upgrade the Flagsmith SDK dependency to v9, since this provider uses a quite out of date version. I'd prefer leaving this PR as-is and make the code nicer when we're making that upgrade.
This PR
Use updated traits in the evaluation context whenever it changes.
Various README improvements.
Related Issues
Fixes #1232
Notes
N/A
Follow-up Tasks
Created an issue for the Flagsmith SDK to natively support
Dateobjects: Flagsmith/flagsmith-js-client#329How to test
Added a failing test which is fixed by this PR.