-
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
fix: Removes flag resolving from Confidence child instances #149
Conversation
@@ -3,7 +3,7 @@ import Foundation | |||
/** | |||
Sends events to Confidence. Contextual data is appended to each event | |||
*/ | |||
public protocol ConfidenceEventSender: Contextual { | |||
public protocol ConfidenceEventSender: ConfidenceContextProvider { |
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.
Would removing Contextual not be a breaking change? I am not sure whether it's necessary to remove 🤔
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.
Answering here for @vahidlazio as well: the problem I found with Contextual
is that withContext returns a Self, and if it's Confidence
implementing such protocol the return type can only be Confidence (while we want ConfidenceEventSender
instead)
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 don't think that removing Contextual
is a breaking change, since final users never operate with such type directly: they either have a Confidence instance or a ConfidenceEventSender instance
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 think my issue with this is that removing Contextual then means this structure doesn't align with Android/JS anymore.
Right, so since Confidence doesn't confirm to EventSender, it won't be able to return one when calling withContext. I think that makes sense 👍 We should document this somewhere, but that's not necessarily part of this PR
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 think my issue with this is that removing Contextual then means this structure doesn't align with Android/JS anymore.
True, I am also not very about that. But as long as this is internals, it's less of a problem and we can always change it without breaking backwards compat with final users
Right, so since Confidence doesn't confirm to EventSender, it won't be able to return one when calling withContext.
Even if it would conform to EventSender, it seems like the return type Self
only accepts the type of the implementing class Confidence
31c02cf
to
bac7fbc
Compare
The plan is to enable this back once we have support for it in the caching layer.