-
Notifications
You must be signed in to change notification settings - Fork 9
feat: add eval reasons for JS Bucketing #1085
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
537fdb7 to
dbdc9c5
Compare
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.
Pull Request Overview
This PR introduces evaluation reasons for JS bucketing by adding new enums and updating the types to propagate evaluation details during bucketing and segmentation. Key changes include the removal of the unsupported IP filter, the addition of EVAL_REASONS, EVAL_REASON_DETAILS, and EvalReason types in the SDK API, and extensive updates to segmentation and bucketing logic, including corresponding updates to tests to reflect the new output structure.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| lib/shared/types/src/types/config/models/audience.ts | Removed the unsupported IP filter by deleting the ip field from UserSubType. |
| lib/shared/types/src/types/apis/sdk/clientSDKAPI.ts | Added new enums (DEFAULT_REASONS, EVAL_REASONS, EVAL_REASON_DETAILS) and updated evalReason typings. |
| lib/shared/bucketing/src/segmentation.ts | Refactored evaluateOperator to return an object with result and reasonDetails; updated filter evaluation logic. |
| lib/shared/bucketing/src/bucketing.ts | Updated bucketing functions to return variation decisions along with evalReasons based on rollout and targeting status. |
| lib/shared/bucketing/tests/* | Adjusted test assertions to match the new object shape with evaluation reason details. |
| lib/shared/bucketing-test-data/src/data/testData.ts | Updated country codes to conform with ISO-3166 two-letter formats. |
Comments suppressed due to low confidence (1)
lib/shared/bucketing/src/bucketing.ts:80
- In decideTargetVariation, the lower bound for each variation check is always 0 because previousDistributionIndex is never updated inside the loop. Consider updating previousDistributionIndex at the end of each iteration (e.g. setting previousDistributionIndex = distributionIndex) to accurately reflect each variation's range.
const previousDistributionIndex = 0
| const result = checkStringsFilter(data.country, filter) | ||
| return { | ||
| result, | ||
| reasonDetails: result ? EVAL_REASON_DETAILS.COUNTRY : 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.
why would the result be undefined here?
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.
if result is false, that means the user did not meet the criteria to be bucketed into the feature, therefore we don't want to set the reasonDetails property
ba114c2 to
47d6fd2
Compare
…ides to Feature & Variable response for JS Bucketing shared lib
…e the evaluation response
… 2 character country codes
…ce Match and Custom Data
626cd83 to
7f72e58
Compare
8b53003 to
ee34287
Compare
…and feature types to use eval instead of evalReasons
| deviceModel: (data, filter) => checkStringsFilter(data.deviceModel, filter), | ||
| platform: (data, filter) => checkStringsFilter(data.platform, filter), | ||
| country: (data, filter) => { | ||
| const result = checkStringsFilter(data.country, filter) |
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.
should we check the comparator for the string filters?
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.
or is the idea to let the user know of the field that they are getting evaluated by and not the actual targeting?
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.
the field(s) that the user matched to get bucketed into a given variation
…lReasons field to allow node SDK builds as wasm bucketing still depends on it
evalon the Feature & VariablereasonDetailsif the evaluation is truthyvariable.evalfrom Bucketed User Configurationsvariable.eval?.reasontoreasonfor evaluation reasons (aka if DevCycle provides an evaluation reason, pass it along to the OpenFeature Provider)evalReasonfield to theSDKVariabletype definition to allow NodeJS builds to work, this will be removed during the WASM Bucketing lib changes