-
-
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
fix: frontend variant weights distribution #4347
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 ↗︎
1 Ignored Deployment
|
({ remainingPercentage, variableVariantCount }, variant) => { | ||
if (variant.weight && variant.weightType === weightTypes.FIX) { | ||
remainingPercentage -= Number(variant.weight); | ||
} else { | ||
} | ||
if (variant.weightType === weightTypes.VARIABLE) { |
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.
there is a bugfix here - it was not counting correctly when weight == 0
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.
do we need let?
35fb0b7
to
b56f9ae
Compare
String(remainingPercentage / variableVariantCount) | ||
); | ||
const getPercentage = () => | ||
Math.round(remainingPercentage / variableVariantCount); |
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 variableVariantCount == 1
it will return remainingPercentage
, so there is no special case at the end
|
||
return variants.map(variant => { | ||
if (variant.weightType !== weightTypes.FIX) { | ||
const percentage = getPercentage(); // round "as we go" - clean best effort approach |
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.
Instead of dividing everything at once, calculate "head" and adjust "tail"
({ remainingPercentage, variableVariantCount }, variant) => { | ||
if (variant.weight && variant.weightType === weightTypes.FIX) { | ||
remainingPercentage -= Number(variant.weight); | ||
} else { | ||
} | ||
if (variant.weightType === weightTypes.VARIABLE) { |
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.
do we need let?
@@ -52,7 +52,7 @@ export interface IFeatureVariant { | |||
name: string; | |||
stickiness: string; | |||
weight: number; | |||
weightType: string; | |||
weightType: 'fix' | 'variable'; |
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.
isn't it gonna break openapi schemas?
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.
That's in frontend, before we started generating it from OpenAPI. Something to clean up and refactor
we need |
About the changes
Unit-tested way of distributing weights between variants.