fix: properly handle flag resolver variants (#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 | undefined` to `Variant` where
we use the `getDefaultVariant()` helper method to return us a [default
variant](55274e4953/src/variant.ts (L37-L42)).

frontend/src/utils/variants.ts

@@ -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;
         if (variant.payload.type === PayloadType.JSON) {
             return JSON.parse(variant.payload.value) as T;
         }

src/lib/types/experimental.ts

@@ -1,5 +1,6 @@
 import { Variant, PayloadType } from 'unleash-client';
 import { parseEnvVarBoolean } from '../util';
+import { getDefaultVariant } from 'unleash-client/lib/variant';
 
 export type IFlagKey =
     | 'anonymiseEventLog'
@@ -98,7 +99,7 @@ export const defaultExperimentalOptions: IExperimentalOptions = {
     flags,
     externalResolver: {
         isEnabled: (): boolean => false,
-        getVariant: () => undefined,
+        getVariant: () => getDefaultVariant(),
     },
 };
 
@@ -114,16 +115,10 @@ export interface IFlagContext {
 export interface IFlagResolver {
     getAll: (context?: IFlagContext) => IFlags;
     isEnabled: (expName: IFlagKey, context?: IFlagContext) => boolean;
-    getVariant: (
-        expName: IFlagKey,
-        context?: IFlagContext,
-    ) => Variant | undefined;
+    getVariant: (expName: IFlagKey, context?: IFlagContext) => Variant;
 }
 
 export interface IExternalFlagResolver {
     isEnabled: (flagName: IFlagKey, context?: IFlagContext) => boolean;
-    getVariant: (
-        flagName: IFlagKey,
-        context?: IFlagContext,
-    ) => Variant | undefined;
+    getVariant: (flagName: IFlagKey, context?: IFlagContext) => Variant;
 }

src/lib/util/flag-resolver.test.ts

@@ -2,6 +2,7 @@ import { PayloadType } from 'unleash-client';
 import { defaultExperimentalOptions, IFlagKey } from '../types/experimental';
 import FlagResolver, { getVariantValue } from './flag-resolver';
 import { IExperimentalOptions } from '../types/experimental';
+import { getDefaultVariant } from 'unleash-client/lib/variant';
 
 test('should produce empty exposed flags', () => {
     const resolver = new FlagResolver(defaultExperimentalOptions);
@@ -30,7 +31,7 @@ test('should use external resolver for dynamic flags', () => {
                 return true;
             }
         },
-        getVariant: () => undefined,
+        getVariant: () => getDefaultVariant(),
     };
 
     const config = {
@@ -50,7 +51,7 @@ test('should not use external resolver for enabled experiments', () => {
         isEnabled: () => {
             return false;
         },
-        getVariant: () => undefined,
+        getVariant: () => getDefaultVariant(),
     };
 
     const config = {
@@ -70,7 +71,7 @@ test('should load experimental flags', () => {
         isEnabled: () => {
             return false;
         },
-        getVariant: () => undefined,
+        getVariant: () => getDefaultVariant(),
     };
 
     const config = {
@@ -91,7 +92,7 @@ test('should load experimental flags from external provider', () => {
                 return true;
             }
         },
-        getVariant: () => undefined,
+        getVariant: () => getDefaultVariant(),
     };
 
     const config = {
@@ -121,6 +122,7 @@ test('should support variant flags', () => {
             if (name === 'extraFlag') {
                 return variant;
             }
+            return getDefaultVariant();
         },
     };
 
@@ -131,22 +133,33 @@ test('should support variant flags', () => {
 
     const resolver = new FlagResolver(config as IExperimentalOptions);
 
-    expect(resolver.getVariant('someFlag' as IFlagKey)).toBe(undefined);
-    expect(resolver.getVariant('otherFlag' as IFlagKey)).toBe(undefined);
+    expect(resolver.getVariant('someFlag' as IFlagKey)).toStrictEqual(
+        getDefaultVariant(),
+    );
+    expect(resolver.getVariant('otherFlag' as IFlagKey)).toStrictEqual(
+        getDefaultVariant(),
+    );
     expect(resolver.getVariant('extraFlag' as IFlagKey)).toStrictEqual(variant);
 });
 
 test('should expose an helper to get variant value', () => {
+    expect(
+        getVariantValue({
+            enabled: true,
+            name: 'variant-A',
+        }),
+    ).toBe('variant-A');
+
     expect(
         getVariantValue({
             enabled: true,
             name: 'variant',
             payload: {
                 type: PayloadType.STRING,
-                value: 'variant-A',
+                value: 'variant-B',
             },
         }),
-    ).toBe('variant-A');
+    ).toBe('variant-B');
 
     expect(
         getVariantValue({

src/lib/util/flag-resolver.ts

@@ -7,6 +7,7 @@ import {
     IFlagResolver,
     IFlagKey,
 } from '../types/experimental';
+import { getDefaultVariant } from 'unleash-client/lib/variant';
 
 export default class FlagResolver implements IFlagResolver {
     private experiments: IFlags;
@@ -22,13 +23,16 @@ export default class FlagResolver implements IFlagResolver {
         const flags: IFlags = { ...this.experiments };
 
         Object.keys(flags).forEach((flagName: IFlagKey) => {
-            if (!this.experiments[flagName]) {
-                if (typeof flags[flagName] === 'boolean') {
+            const flag = flags[flagName];
+            if (typeof flag === 'boolean') {
+                if (!flag) {
                     flags[flagName] = this.externalResolver.isEnabled(
                         flagName,
                         context,
                     );
-                } else {
+                }
+            } else {
+                if (!flag?.enabled) {
                     flags[flagName] = this.externalResolver.getVariant(
                         flagName,
                         context,
@@ -49,10 +53,10 @@ export default class FlagResolver implements IFlagResolver {
         return this.externalResolver.isEnabled(expName, context);
     }
 
-    getVariant(expName: IFlagKey, context?: IFlagContext): Variant | undefined {
+    getVariant(expName: IFlagKey, context?: IFlagContext): Variant {
         const exp = this.experiments[expName];
         if (exp) {
-            if (typeof exp === 'boolean') return undefined;
+            if (typeof exp === 'boolean') return getDefaultVariant();
             else return exp;
         }
         return this.externalResolver.getVariant(expName, context);
@@ -62,7 +66,8 @@ export default class FlagResolver implements IFlagResolver {
 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;
         if (variant.payload.type === PayloadType.JSON) {
             return JSON.parse(variant.payload.value) as T;
         }

src/server-dev.ts

@@ -2,7 +2,6 @@ import { start } from './lib/server-impl';
 import { createConfig } from './lib/create-config';
 import { LogLevel } from './lib/logger';
 import { ApiTokenType } from './lib/types/models/api-token';
-import { PayloadType } from 'unleash-client';
 
 process.nextTick(async () => {
     try {
@@ -41,23 +40,6 @@ process.nextTick(async () => {
                         responseTimeWithAppNameKillSwitch: false,
                         variantMetrics: true,
                         strategyImprovements: true,
-                        messageBanner: {
-                            name: 'message-banner',
-                            enabled: true,
-                            payload: {
-                                type: PayloadType.JSON,
-                                value: `{
-                                    "message": "**New message banner!** Check out this new feature.",
-                                    "variant": "secondary",
-                                    "sticky": true,
-                                    "link": "dialog",
-                                    "linkText": "What is this?",
-                                    "plausibleEvent": "message_banner",
-                                    "dialog": "![Message Banner](https://www.getunleash.io/logos/unleash_pos.svg)\\n## Message Banner 🎉\\n**New message banner!**\\n\\nCheck out this new feature:\\n\\n- Get the latest announcements\\n- Get warnings about your Unleash instance\\n\\nYou can read more about it on our newest [blog post](https://www.getunleash.io/blog).",
-                                    "icon": "none"
-                                }`,
-                            },
-                        },
                     },
                 },
                 authentication: {