-
Notifications
You must be signed in to change notification settings - Fork 71
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
refactor: strategy variants inside strategy #494
Changes from all commits
ea46c56
578f79b
1b2984e
bb4a8b0
aabbb8e
2680fdb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,10 @@ import { FeatureInterface } from './feature'; | |
import { RepositoryInterface } from './repository'; | ||
import { | ||
Variant, VariantDefinition, | ||
getDefaultVariant, selectVariant, selectVariantDefinition, | ||
getDefaultVariant, selectVariant, | ||
} from './variant'; | ||
import { Context } from './context'; | ||
import { Constraint, Segment } from './strategy/strategy'; | ||
import { Constraint, Segment, StrategyResult } from './strategy/strategy'; | ||
import { createImpressionEvent, UnleashEvents } from './events'; | ||
|
||
interface BooleanMap { | ||
|
@@ -58,7 +58,7 @@ export default class UnleashClient extends EventEmitter { | |
|
||
isEnabled(name: string, context: Context, fallback: Function): boolean { | ||
const feature = this.repository.getToggle(name); | ||
const [enabled] = this.isFeatureEnabled(feature, context, fallback); | ||
const { enabled } = this.isFeatureEnabled(feature, context, fallback); | ||
if (feature?.impressionData) { | ||
this.emit( | ||
UnleashEvents.Impression, | ||
|
@@ -77,47 +77,48 @@ export default class UnleashClient extends EventEmitter { | |
feature: FeatureInterface | undefined, | ||
context: Context, | ||
fallback: Function, | ||
): [boolean, VariantDefinition | undefined] { | ||
): StrategyResult { | ||
if (!feature) { | ||
return [fallback(), undefined]; | ||
return { enabled: fallback() }; | ||
} | ||
|
||
if (!feature || !feature.enabled) { | ||
return [false, undefined]; | ||
return { enabled: false }; | ||
} | ||
|
||
if (!Array.isArray(feature.strategies)) { | ||
const msg = `Malformed feature, strategies not an array, is a ${typeof feature.strategies}`; | ||
this.emit(UnleashEvents.Error, new Error(msg)); | ||
return [false, undefined]; | ||
return { enabled: false }; | ||
} | ||
|
||
if (feature.strategies.length === 0) { | ||
return [feature.enabled, undefined]; | ||
return { enabled: feature.enabled }; | ||
} | ||
|
||
let featureVariant = undefined; | ||
let strategyResult = { enabled: false }; | ||
|
||
return [( | ||
feature.strategies.length > 0 && | ||
feature.strategies?.some((strategySelector): boolean => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I left some in the impl since we want the early exit/short-circuit semantics There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not super relevant, but is |
||
const strategy = this.getStrategy(strategySelector.name); | ||
if (!strategy) { | ||
this.warnOnce(strategySelector.name, feature.name, feature.strategies); | ||
return false; | ||
} | ||
const constraints = this.yieldConstraintsFor(strategySelector); | ||
const result = | ||
strategy.getResult(strategySelector.parameters, | ||
context, | ||
constraints, | ||
strategySelector.variants); | ||
|
||
feature.strategies.some((strategySelector): boolean => { | ||
const strategy = this.getStrategy(strategySelector.name); | ||
if (!strategy) { | ||
this.warnOnce(strategySelector.name, feature.name, feature.strategies); | ||
return false; | ||
} | ||
const constraints = this.yieldConstraintsFor(strategySelector); | ||
const enabled = | ||
strategy.isEnabledWithConstraints(strategySelector.parameters, context, constraints); | ||
const variantParam = strategySelector?.variants; | ||
if (result.enabled) { | ||
strategyResult = result; | ||
return true; | ||
} | ||
return false; | ||
}); | ||
|
||
if (enabled && Array.isArray(variantParam) && variantParam.length > 0) { | ||
featureVariant = selectVariantDefinition(feature.name, variantParam, context); | ||
} | ||
return enabled; | ||
}) | ||
), featureVariant]; | ||
return strategyResult; | ||
} | ||
|
||
* yieldConstraintsFor( | ||
|
@@ -187,24 +188,19 @@ export default class UnleashClient extends EventEmitter { | |
return fallback; | ||
} | ||
|
||
let enabled = true; | ||
let strategyVariant = undefined; | ||
if (checkToggle) { | ||
[enabled, strategyVariant] = this.isFeatureEnabled(feature, context, () => | ||
fallbackVariant ? fallbackVariant.enabled : false, | ||
); | ||
} | ||
const result = this.isFeatureEnabled(feature, context, () => !!fallbackVariant?.enabled); | ||
|
||
if (strategyVariant) { | ||
return { | ||
name: strategyVariant.name, | ||
payload: strategyVariant.payload, | ||
enabled: !checkToggle || enabled, | ||
}; | ||
if (result.enabled && result.variant) { | ||
return result.variant; | ||
} | ||
|
||
if (!result.enabled) { | ||
return fallback; | ||
} | ||
} | ||
|
||
if (!enabled || | ||
!feature.variants || | ||
if (!feature.variants || | ||
!Array.isArray(feature.variants) || | ||
feature.variants.length === 0) { | ||
return fallback; | ||
|
@@ -218,7 +214,7 @@ export default class UnleashClient extends EventEmitter { | |
return { | ||
name: variant.name, | ||
payload: variant.payload, | ||
enabled: !checkToggle || enabled, | ||
enabled: true, | ||
}; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,14 @@ | ||
import { gt as semverGt, lt as semverLt, eq as semverEq, clean as cleanSemver } from 'semver'; | ||
import { Context } from '../context'; | ||
import { resolveContextValue } from '../helpers'; | ||
import { VariantDefinition } from '../variant'; | ||
import { selectVariantDefinition, Variant, VariantDefinition } from '../variant'; | ||
|
||
export interface StrategyTransportInterface { | ||
name: string; | ||
parameters: any; | ||
constraints: Constraint[]; | ||
segments?: number[]; | ||
variants?: VariantDefinition[] | ||
variants?: VariantDefinition[]; | ||
} | ||
|
||
export interface Constraint { | ||
|
@@ -68,7 +68,7 @@ const StringOperator = (constraint: Constraint, context: Context) => { | |
contextValue = contextValue?.toLocaleLowerCase(); | ||
} | ||
|
||
if(typeof contextValue !== 'string') { | ||
if (typeof contextValue !== 'string') { | ||
return false; | ||
} | ||
|
||
|
@@ -166,6 +166,9 @@ operators.set(Operator.DATE_BEFORE, DateOperator); | |
operators.set(Operator.SEMVER_EQ, SemverOperator); | ||
operators.set(Operator.SEMVER_GT, SemverOperator); | ||
operators.set(Operator.SEMVER_LT, SemverOperator); | ||
|
||
export type StrategyResult = { enabled: true, variant?: Variant } | { enabled: false }; | ||
|
||
export class Strategy { | ||
public name: string; | ||
|
||
|
@@ -215,4 +218,31 @@ export class Strategy { | |
) { | ||
return this.checkConstraints(context, constraints) && this.isEnabled(parameters, context); | ||
} | ||
|
||
getResult(parameters: any, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the main change - everything strategy variant related is located in one class |
||
context: Context, | ||
constraints: IterableIterator<Constraint | undefined>, | ||
variants?: VariantDefinition[]): StrategyResult { | ||
const enabled = | ||
this.isEnabledWithConstraints(parameters, context, constraints); | ||
|
||
if (enabled && Array.isArray(variants) && variants.length > 0) { | ||
const variantDefinition = | ||
selectVariantDefinition(parameters.groupId, variants, context); | ||
return variantDefinition ? { | ||
enabled: true, | ||
variant: { | ||
name: variantDefinition.name, | ||
enabled: true, | ||
payload: variantDefinition.payload, | ||
}, | ||
} : { enabled: true }; | ||
} | ||
|
||
if (enabled) { | ||
return { enabled: true }; | ||
} | ||
|
||
return { enabled: false }; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -77,7 +77,7 @@ function findOverride(variants: VariantDefinition[], | |
} | ||
|
||
export function selectVariantDefinition( | ||
featureName: string, | ||
groupId: string, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. featureName was misleading in the original code since we accept groupId here |
||
variants: VariantDefinition[], | ||
context: Context, | ||
): VariantDefinition | null { | ||
|
@@ -92,7 +92,7 @@ export function selectVariantDefinition( | |
|
||
const { stickiness } = variants[0]; | ||
|
||
const target = normalizedValue(getSeed(context, stickiness), featureName, totalWeight); | ||
const target = normalizedValue(getSeed(context, stickiness), groupId, totalWeight); | ||
|
||
let counter = 0; | ||
const variant = variants.find((v: VariantDefinition): VariantDefinition | undefined => { | ||
|
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.
Proper union type introduced here