-
-
Notifications
You must be signed in to change notification settings - Fork 658
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
feat: strategy variants in playground #4281
Conversation
Sonatype Lift is retiringSonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console. |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
package.json
Outdated
@@ -161,7 +161,7 @@ | |||
"stoppable": "^1.1.0", | |||
"ts-toolbelt": "^9.6.0", | |||
"type-is": "^1.6.18", | |||
"unleash-client": "3.21.0", | |||
"unleash-client": "4.1.0-beta.3", |
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.
support for strategy variants that will be used in frontend API but not playground which has its own impl
@@ -24,6 +24,8 @@ export type EvaluatedPlaygroundStrategy = Omit< | |||
|
|||
export type FeatureStrategiesEvaluationResult = { | |||
result: boolean | typeof playgroundStrategyEvaluation.unknownResult; | |||
variant?: Variant; |
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.
when evaluating each strategy we also add variant is resolved to and all possible variants that we chose from
}; | ||
}, | ||
); | ||
|
||
// Feature evaluation | ||
const overallStrategyResult = () => { | ||
const overallStrategyResult = (): [ |
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.
choosing a first matching strategy and its variant/variants
this.isFeatureEnabled(feature, context, () => | ||
fallbackVariant ? fallbackVariant.enabled : false, | ||
).result === true; | ||
const result = this.isFeatureEnabled(feature, context, () => |
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.
based on Node SDK
@@ -127,10 +130,27 @@ export class Strategy { | |||
const overallResult = | |||
constraintResults.result && enabledResult && segmentResults.result; | |||
|
|||
const variantDefinition = variantDefinitions |
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.
based on Node SDK
@@ -51,6 +53,54 @@ export const strategyEvaluationResults = { | |||
description: | |||
'Whether this strategy evaluates to true or not.', | |||
}, | |||
variant: { |
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.
Created inline variant schema that reflects what SDK sees. It's different from the variant in the admin API which has more fields.
variants: { | ||
type: 'array', | ||
description: 'The feature variants.', | ||
items: { $ref: variantSchema.$id }, |
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.
this is full variants since we need to display weights in the playground UI
yarn.lock
Outdated
version "3.21.0" | ||
resolved "https://registry.yarnpkg.com/unleash-client/-/unleash-client-3.21.0.tgz#a31ab30acb42abfb3a21180aa83e4415a3124ec1" | ||
integrity sha512-I7eYhRyOia3oBZ9Tu1v+IlNO+XJgsjcMEO2+j+e4A7LTTKZvGoV8WPfDGGxiMPKBPHNUACkERB3YhCQ9jzTGoQ== | ||
unleash-client@4.1.0-beta.4: |
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.
this also adds strategy variant support for frontend API. no coding needed
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.
LGTM
About the changes
Important files
Discussion points
I'm pretty confident of the change since the property based tests caught many errors as I was doing the refactor