From 4adc977ba03fc994bf75ee14e24a08ad7c054e17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Thu, 18 May 2023 17:04:55 +0100 Subject: [PATCH] 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](https://github.com/Unleash/unleash-client-node/blob/55274e4953164cdfdf14469a7e318147aa4c37cc/src/variant.ts#L37-L42). --- frontend/src/utils/variants.ts | 3 ++- src/lib/types/experimental.ts | 13 ++++--------- src/lib/util/flag-resolver.test.ts | 29 +++++++++++++++++++++-------- src/lib/util/flag-resolver.ts | 17 +++++++++++------ src/server-dev.ts | 18 ------------------ 5 files changed, 38 insertions(+), 42 deletions(-) diff --git a/frontend/src/utils/variants.ts b/frontend/src/utils/variants.ts index 4063b5bb70..d26a6d2238 100644 --- a/frontend/src/utils/variants.ts +++ b/frontend/src/utils/variants.ts @@ -18,7 +18,8 @@ export interface Variant { export const getVariantValue = ( 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; } diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 3954998e0e..5464a8da60 100644 --- a/src/lib/types/experimental.ts +++ b/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; } diff --git a/src/lib/util/flag-resolver.test.ts b/src/lib/util/flag-resolver.test.ts index de9ac4633b..6366e62e9a 100644 --- a/src/lib/util/flag-resolver.test.ts +++ b/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({ diff --git a/src/lib/util/flag-resolver.ts b/src/lib/util/flag-resolver.ts index 180235a684..55afcbfc6d 100644 --- a/src/lib/util/flag-resolver.ts +++ b/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 = ( 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; } diff --git a/src/server-dev.ts b/src/server-dev.ts index c4a757d70d..44bc204350 100644 --- a/src/server-dev.ts +++ b/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: {