-
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
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. |
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
featureName was misleading in the original code since we accept groupId here
@@ -77,47 +77,48 @@ export default class UnleashClient extends EventEmitter { | |||
feature: FeatureInterface | undefined, | |||
context: Context, | |||
fallback: Function, | |||
): [boolean, VariantDefinition | undefined] { | |||
): StrategyResult { |
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
|
||
return [( | ||
feature.strategies.length > 0 && | ||
feature.strategies?.some((strategySelector): boolean => { |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not super relevant, but is some
guaranteed to traverse the list in order? Or is it just ... most likely to do that?
@@ -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 comment
The 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
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.
Looks sensible to me. I've got two questions that I'd like to get answered, but nothing super major.
Assuming using the group ID over feature name is good, I'm giving this a provisional approval.
|
||
return [( | ||
feature.strategies.length > 0 && | ||
feature.strategies?.some((strategySelector): boolean => { |
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.
Probably not super relevant, but is some
guaranteed to traverse the list in order? Or is it just ... most likely to do that?
src/strategy/strategy.ts
Outdated
@@ -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 | null } | { enabled: false }; |
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 is variant both undefineable and null? Do we treat them differently? That seems subtle and like something that might cause issues down the line. Why not just undefined?
export type StrategyResult = { enabled: true, variant?: Variant | null } | { enabled: false }; | |
export type StrategyResult = { enabled: true, variant?: Variant } | { enabled: false }; |
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.
One of the methods that we call (selecting variant from a list) returns null so we have to handle this
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.
Ah. In that case, could we just make it Variant | null
? Because we allow both it strikes me as significant, so I would expect undefined
and null
to mean different things. If they don't, I would prefer collapsing it into a single value.
Your choice in the end, but the more we can lock it down and be strict about what we return, the easier it'll be to work with later.
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.
After further investigation we can remove null
@thomasheartman Array.some in practice gives me predictable order but I don't think spec can guarantee this. We should have tests that check this. |
@kwasniew Regarding guaranteed order with some: Arrays are ordered collections, so it should be fine. I'm just thinking that some engines might perform some optimizations one the implementation that makes it not predictable (because the function itself doesn't really care about order). But looking back at the code: the order here really doesn't matter, does it? So whether the order is guaranteed or not is irrelevant, yeah? Never mind me, then 😅 |
It's actually guaranteed to be ordered in the language spec! |
@sighphyre good call. I should have checked directly in the spec instead of asking chatgpt :D |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [unleash-client](https://togithub.com/Unleash/unleash-client-node) | [`4.1.0-beta.5` -> `4.1.0`](https://renovatebot.com/diffs/npm/unleash-client/4.1.0-beta.5/4.1.0) | [![age](https://developer.mend.io/api/mc/badges/age/npm/unleash-client/4.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/unleash-client/4.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/unleash-client/4.1.0-beta.5/4.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/unleash-client/4.1.0-beta.5/4.1.0?slim=true)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>Unleash/unleash-client-node (unleash-client)</summary> ### [`v4.1.0`](https://togithub.com/Unleash/unleash-client-node/releases/tag/v4.1.0) [Compare Source](https://togithub.com/Unleash/unleash-client-node/compare/v4.1.0-beta.5...v4.1.0) #### What's Changed - feat: strategy variants by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#482 - fix: remove unused segment fields by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#489 - feat: expose config type by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#491 - fix:fetch lib with its deps by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#492 - fix: force get variant is back by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#493 - refactor: strategy variants inside strategy by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#494 - feat: variant on enabled toggle metrics by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#495 - chore(deps): update dependency nock to v13.3.2 by [@​renovate](https://togithub.com/renovate) in [Unleash/unleash-client-node#488 - chore(deps): update dependency eslint to v8.45.0 by [@​renovate](https://togithub.com/renovate) in [Unleash/unleash-client-node#490 - chore(deps): update typescript-eslint monorepo to v5.62.0 by [@​renovate](https://togithub.com/renovate) in [Unleash/unleash-client-node#483 - chore(deps): update dependency eslint-config-airbnb-typescript to v17.1.0 by [@​renovate](https://togithub.com/renovate) in [Unleash/unleash-client-node#487 - chore(deps): update dependency eslint-plugin-prettier to v5 by [@​renovate](https://togithub.com/renovate) in [Unleash/unleash-client-node#485 - feat: version bump non beta by [@​kwasniew](https://togithub.com/kwasniew) in [Unleash/unleash-client-node#496 **Full Changelog**: Unleash/unleash-client-node@v4.0.2...v4.1.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Enabled. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/Unleash/unleash). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNi4yNC4yIiwidXBkYXRlZEluVmVyIjoiMzYuMjQuMiIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
About the changes
Important files
Discussion points