-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add listening for context changes #97
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, apart from two comments addressing some race conditions and corner cases
let resolveResult = try await resolve(context: context) | ||
var removedKeys: [String] = [] | ||
if let oldContext = oldContext { | ||
removedKeys = Array(oldContext.asMap().filter { key, _ in !newContext.asMap().keys.contains(key) }.keys) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OF.init -> "context1" = 1
Confidence.putContext -> "context1" = 2
OF.init -> "context2" = "abc"
In this case, the final context won't have context1
but we would expect it, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say no. if you had some context in your OF and now you set it again without, it means you removed it. I'd remove it here too. (keeping the current behaviour)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't Confidence
(line 2) overriding context1
? At this point, that takes precedence
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes but the set of context using open feature api is also an action which happens later, which takes over the priority again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
basically the chain of commands from the confidence perspective is :
add context 1 -> commander : OF
change context 1 -> commander: Confidence API
remove context 1 and add context 2 -> commander OF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see 👍
No description provided.