fix: properly handle flag resolver variants #3808
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| @@ -18,7 +18,8 @@ export interface Variant { | |||
| export const getVariantValue = <T = string>( | |||
| variant: Variant | undefined | |||
| ): T | undefined => { | |||
| if (variant?.payload !== undefined) { | |||
| if (variant?.enabled) { | |||
| if (!variant.payload) return variant.name as T; | |||
There was a problem hiding this comment.
Is this correct? Is the value of the variant the name if the payload is not defined?
There was a problem hiding this comment.
That was the intention, yes. In case you don't specify a payload, the only thing you can use to identify the variant and runtime control, effectively its value in the perspective of this helper, turns out to be its name.
There was a problem hiding this comment.
In case it helps visualizing what I mean in our UI:
We didn't specify a payload, so the variant name acts as our value.
This means that we could easily do something like:
const variant = getVariantValue(uiConfig.flags.ourVariant)
if (variant === 'variant-A') {
// Do cool thing
}| if (typeof flags[flagName] === 'boolean') { | ||
| const flag = flags[flagName]; | ||
| if (typeof flag === 'boolean') { | ||
| if (!flag) { |
There was a problem hiding this comment.
Wouldn't this prevent the flag to be evaluated again? If it's turned true once, it seems you can't disable it because you'll skip the isEnabled evaluation
There was a problem hiding this comment.
If I understood the original reasoning, our flag here is the default value we get from either experimental or server-dev, not the current state of the flag. I.e. the initial value.
What we're saying here is, unless one those two places above override the flag by setting it as something truthy (e.g. env variable), we want to fetch it from the external resolver.
There was a problem hiding this comment.
Ok, I saw it now, and flags is a local variable of this method
| } else { | ||
| } | ||
| } else { | ||
| if (!flag?.enabled) { |
There was a problem hiding this comment.
Should we still resolve getVariant even if it's not enabled? It would give us the default variant
There was a problem hiding this comment.
Same as above, if the initial value is falsy (not overridden be either experimental or server-dev to be truthy), we want to see what our external resolver has to say about it.
It may not be the default variant in this case. We may have a falsy initial value, but our external resolver (e.g. Unleash hosted) saying that this flag should be enabled, etc.
https://linear.app/unleash/issue/2-1880/fix-flag-resolver-getvariant-behavior Fixes the flag resolver `getVariant` behavior when there's a variant object set in `experimental` - The flag resolver should call the external resolver `getVariant` when not overridden to be true, even if set as variant object in `experimental`. Related: #3808
Variants were not being properly handled in the
flag-resolver: The fact that the default value of the variant is not falsy made it so we never asked the external flag resolver for the value.This also moves the logic from
Variant | undefinedtoVariantwhere we use thegetDefaultVariant()helper method to return us a default variant.