Skip to content
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

Merged
merged 8 commits into from
Apr 25, 2024
Merged

Conversation

vahidlazio
Copy link
Contributor

No description provided.

@vahidlazio vahidlazio changed the title feat: [WIP]add listening for context changes feat: [WIP] add listening for context changes Apr 22, 2024
@vahidlazio vahidlazio marked this pull request as ready for review April 24, 2024 14:43
@vahidlazio vahidlazio changed the title feat: [WIP] add listening for context changes feat: add listening for context changes Apr 24, 2024
Copy link
Member

@fabriziodemaria fabriziodemaria left a 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

Sources/Confidence/Confidence.swift Outdated Show resolved Hide resolved
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)
Copy link
Member

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?

Copy link
Contributor Author

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)

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see 👍

@vahidlazio vahidlazio merged commit 0d1cefd into main Apr 25, 2024
3 checks passed
@fabriziodemaria fabriziodemaria deleted the reconciling-context branch April 25, 2024 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants