refactor: strategy variants inside strategy #494

kwasniew · 2023-07-19T11:50:57Z

About the changes

explicitly modelling strategy result as a discriminative union. We're discriminating on the enabled key.
strategy variant calculation moved to the strategy instead of being spread between client and variant
using strategy groupId for variant distribution instead of using feature name. groupId can be set to the feature name though. This change allows to set the same groupId across variants in different features strategies

Important files

Discussion points

sonatype-lift · 2023-07-19T11:51:01Z

Sonatype Lift is retiring

Sonatype 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.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

github-actions · 2023-07-19T11:52:13Z

coverage: 91.233% (-0.06%) from 91.295% when pulling 2680fdb on refactor-strategy-variants into 548c9af on main.

kwasniew · 2023-07-19T11:55:02Z

src/variant.ts

@@ -77,7 +77,7 @@ function findOverride(variants: VariantDefinition[],
 }

 export function selectVariantDefinition(
-  featureName: string,
+  groupId: string,


featureName was misleading in the original code since we accept groupId here

kwasniew · 2023-07-19T11:55:57Z

src/client.ts

@@ -77,47 +77,48 @@ export default class UnleashClient extends EventEmitter {
    feature: FeatureInterface | undefined,
    context: Context,
    fallback: Function,
-  ): [boolean, VariantDefinition | undefined] {
+  ): StrategyResult {


Proper union type introduced here

kwasniew · 2023-07-19T11:56:32Z

src/client.ts


-    return [(
-      feature.strategies.length > 0 &&
+    feature.strategies?.some((strategySelector): boolean => {


I left some in the impl since we want the early exit/short-circuit semantics

Probably not super relevant, but is some guaranteed to traverse the list in order? Or is it just ... most likely to do that?

kwasniew · 2023-07-19T12:00:15Z

src/strategy/strategy.ts

@@ -215,4 +218,31 @@ export class Strategy {
  ) {
    return this.checkConstraints(context, constraints) && this.isEnabled(parameters, context);
  }
+
+  getResult(parameters: any,


this is the main change - everything strategy variant related is located in one class

…factor-strategy-variants

thomasheartman

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.

thomasheartman · 2023-07-19T12:38:21Z

src/client.ts


-    return [(
-      feature.strategies.length > 0 &&
+    feature.strategies?.some((strategySelector): boolean => {


Probably not super relevant, but is some guaranteed to traverse the list in order? Or is it just ... most likely to do that?

thomasheartman · 2023-07-19T12:41:02Z

src/strategy/strategy.ts

@@ -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 };


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?

Suggested change

export type StrategyResult = { enabled: true, variant?: Variant | null } | { enabled: false };

export type StrategyResult = { enabled: true, variant?: Variant } | { enabled: false };

One of the methods that we call (selecting variant from a list) returns null so we have to handle this

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.

After further investigation we can remove null

kwasniew · 2023-07-19T12:53:01Z

@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.

thomasheartman · 2023-07-19T13:41:10Z

@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 😅

sighphyre · 2023-07-19T14:33:22Z

@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.

It's actually guaranteed to be ordered in the language spec!

https://262.ecma-international.org/5.1/#sec-15.4.4.17

kwasniew · 2023-07-19T18:40:51Z

@sighphyre good call. I should have checked directly in the spec instead of asking chatgpt :D
For future readers of the PR: "some calls callbackfn once for each element present in the array, in ascending order,"

[![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. --- - [ ] 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).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

refactor: strategy variants inside strategy

ea46c56

refactor: strategy variants inside strategy

578f79b

kwasniew commented Jul 19, 2023

View reviewed changes

refactor: strategy variants inside strategy

1b2984e

kwasniew commented Jul 19, 2023

View reviewed changes

kwasniew requested review from thomasheartman, sighphyre, andreas-unleash and Tymek July 19, 2023 11:58

kwasniew commented Jul 19, 2023

View reviewed changes

kwasniew added 2 commits July 19, 2023 14:03

Merge branch 'main' of github.com:Unleash/unleash-client-node into re…

bb4a8b0

…factor-strategy-variants

refactor: strategy variants inside strategy

aabbb8e

thomasheartman previously approved these changes Jul 19, 2023

View reviewed changes

fix: remove extra null in type

2680fdb

kwasniew dismissed thomasheartman’s stale review via 2680fdb July 20, 2023 06:20

kwasniew merged commit ef5d59c into main Jul 20, 2023
5 checks passed

kwasniew deleted the refactor-strategy-variants branch July 20, 2023 06:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: strategy variants inside strategy #494

refactor: strategy variants inside strategy #494

kwasniew commented Jul 19, 2023 •

edited

sonatype-lift bot commented Jul 19, 2023

github-actions bot commented Jul 19, 2023 •

edited

kwasniew Jul 19, 2023

kwasniew Jul 19, 2023

kwasniew Jul 19, 2023

thomasheartman Jul 19, 2023

kwasniew Jul 19, 2023

thomasheartman left a comment

thomasheartman Jul 19, 2023

thomasheartman Jul 19, 2023

kwasniew Jul 19, 2023

thomasheartman Jul 19, 2023

kwasniew Jul 20, 2023

kwasniew commented Jul 19, 2023

thomasheartman commented Jul 19, 2023

sighphyre commented Jul 19, 2023

kwasniew commented Jul 19, 2023

	export type StrategyResult = { enabled: true, variant?: Variant \| null } \| { enabled: false };
	export type StrategyResult = { enabled: true, variant?: Variant } \| { enabled: false };

refactor: strategy variants inside strategy #494

refactor: strategy variants inside strategy #494

Conversation

kwasniew commented Jul 19, 2023 • edited

About the changes

Important files

Discussion points

sonatype-lift bot commented Jul 19, 2023

Sonatype Lift is retiring

github-actions bot commented Jul 19, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasheartman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwasniew commented Jul 19, 2023

thomasheartman commented Jul 19, 2023

sighphyre commented Jul 19, 2023

kwasniew commented Jul 19, 2023

kwasniew commented Jul 19, 2023 •

edited

github-actions bot commented Jul 19, 2023 •

edited