Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Use ITargetingContext when calling GetVariantAsync #484
Use ITargetingContext when calling GetVariantAsync #484
Changes from 5 commits
c870ff5
8ea84fb
ea68517
0bdf279
b7a2af8
e6124f6
19e5520
6bcca05
a308916
5a8bf51
5153e88
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 it required for
TContext
to implementITargetingContext
for this API to work? If so, what's the point to haveTContext
?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.
While
GetVariantAsync
, the first step is to evaluate the feature flag state according to the feature filter.If the user uses a contextual feature filter which requires
TContext
for filter evaluation.TContext
is not necessarilyITargetingContext
. The feature manager will still use it to evaluate whether feature flag is enabled or disabled.And he can also use two variants through
DefaultWhenEnabled
andDefaultWhenDisabled
which don't requireTargetingContext
to do the variant allocation.Previously, we only allow user to pass
TargetingContext
which will make it impossible to use contextual filter.How about this?
If the
TContext
doesn't implementITargetingContext
, the feature manager will still give it another chance to get targeting context from the targeting context accessor.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.
You are talking about an edge case where
ITargetingContext
may not be necessary, but general speaking,ITargetingContext
is required for variant assignment. We should enforce the context to implementITargetingContext
.No, we shouldn't use ambient context in contextual feature evaluation.
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.
ITargetingContext
is a public interface, we cannot modify it to avoid breaking changes.If in the future, we want to add new fields in targeting context, we may create a new interface called IAttributeTargetingContext. If we enforce the context to implement ITargetingContext here, we will constrain ourselves.
TargetingContext
should be sealed. We had an oversight on it. I created an issue #486 Currently, we are in an very uncomfortable spot that we cannot modify anything related to targeting context to avoid potential breaking changes.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 also don't think Variants need to be fully tied to targeting. We don't support it yet- but it seems quite reasonable to want variants without wanting user based allocation.
Making it TContext also aligns it with IsEnabled, which is more consistent. IsEnabled is also able to use Allocation in the same way that GetVariant is. So whatever we decide on- the two interfaces should use the same.