From 733b93c939b0ffbbb591a144ebbd489055256750 Mon Sep 17 00:00:00 2001 From: Richard Vowles Date: Thu, 25 Jan 2024 18:17:47 +1300 Subject: [PATCH 1/2] Listeners, Murmur3 and getX methods see changelog Fixes #196 --- featurehub-javascript-client-sdk/CHANGELOG.md | 16 +- .../app/context_impl.ts | 36 +-- .../app/feature_state_holders.ts | 54 ++++- .../app/strategy_matcher.ts | 2 +- featurehub-javascript-client-sdk/package.json | 2 +- .../test/client_context_spec.ts | 11 +- .../test/ripple_feature_update_spec.ts | 212 ++++++++++++++++++ 7 files changed, 304 insertions(+), 29 deletions(-) create mode 100644 featurehub-javascript-client-sdk/test/ripple_feature_update_spec.ts diff --git a/featurehub-javascript-client-sdk/CHANGELOG.md b/featurehub-javascript-client-sdk/CHANGELOG.md index 1f24028..6420a99 100644 --- a/featurehub-javascript-client-sdk/CHANGELOG.md +++ b/featurehub-javascript-client-sdk/CHANGELOG.md @@ -1,10 +1,18 @@ +#### 1.3.3 +- listeners were not being fired when added to contexts that matched strategies. (bugfix) +- all the getX methods on the Context now have defaults, so you can say fhContext.getFlag("feature", false) and if it isn't set or doesn't exist, it will return false. This is an optional field so it doesn't break existing code. (feature) +- set murmur3 hash seed value to 0 to be consistent with all SDK apis (bugfix). + #### 1.3.2 - Delete feature returns version zero which was being prevented from action by 1.3.1 + #### 1.3.1 - Edge case where a feature is retired and then unretired immediately, this _can_ cause the feature to stay deleted. + #### 1.3.0 - This surfaces a fully statically typed API for Typescript clients who enforce it. Of particular interest -is the places where `undefined` can be returned. +is the places where `undefined` can be returned. + #### 1.2.0 - Support for localstorage in a browser to cache the features - EdgeFeatureHubConfig will now hold onto only a single context for server evaluated keys. Once created @@ -14,22 +22,27 @@ happen that is under its control. This ensures the React SDK for example will on - removed the alternative log silencing method - added meta-tag support for browsers and a new FeatureHub object to access them - updated documentation + #### 1.1.7 - Support multiple attribute values per custom evaluated key. - Support a .value method on all SDKs (contributed by Daniel Sanchez (@thedanchez)) - fix an issue with Fastly support (contextSha=0 query parameter). This is the minimum SDK version required for Fastly support. + #### 1.1.6 - Resolve issue with cache control header [Github issues](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/23) - Change to interfaces instead of classes for models, and match the OpenAPI current definition - Add support for deregistering feature and readiness listeners (ready for React SDK) - Deprecate the readyness listener for a readiness listener - Add support for stale environments [GitHub PR](https://github.com/featurehub-io/featurehub-javascript-sdk/pull/78) + #### 1.1.5 - Support for adding custom headers [GitHub issues](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/32), [GitHub PR](https://github.com/featurehub-io/featurehub-javascript-sdk/pull/44) - Fix bug in catch and release mode when feature value would not update if another feature state was changed [GitHub PR](https://github.com/featurehub-io/featurehub-javascript-sdk/pull/70) + #### 1.1.4 - Bump dependencies - Fix bug in percentage calculation where it did not use value from context if specifying your own percentage attributes + #### 1.1.3 - Fix a bug when deleted features are not picked up on polling SDK requests [GitHub issue](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/20) @@ -37,6 +50,7 @@ happen that is under its control. This ensures the React SDK for example will on #### 1.1.2 - Fix a bug related to Catch & Release mode [GitHub issue](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/9) + #### 1.1.1 - Provided additional getters to get feature values and properties [GitHub issue](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/4). If using Typescript, please upgrade your project to typescript v4.3+ - Updated links to examples and documentation as the SDK has been split into a separate repository diff --git a/featurehub-javascript-client-sdk/app/context_impl.ts b/featurehub-javascript-client-sdk/app/context_impl.ts index 7dfc727..d17d996 100644 --- a/featurehub-javascript-client-sdk/app/context_impl.ts +++ b/featurehub-javascript-client-sdk/app/context_impl.ts @@ -111,29 +111,39 @@ export abstract class BaseClientContext implements ClientContext { return this.feature(name).isSet(); } - getNumber(name: string): number | undefined { - return this.feature(name).getNumber(); + getNumber(name: string, def?: number): number | undefined { + const fsh = this.feature(name); + return fsh.isSet() ? fsh.getNumber() : def; } - getString(name: string): string | undefined { - return this.feature(name).getString(); + getString(name: string, def?: string): string | undefined { + const fsh = this.feature(name); + return fsh.isSet() ? fsh.getString() : def; } - getJson(name: string): any | undefined { - const val = this.feature(name).getRawJson(); - return val === undefined ? undefined : JSON.parse(val); + getJson(name: string, def?: any): any | undefined { + const fsh = this.feature(name); + if (fsh.isSet()) { + const val = fsh.getRawJson(); + return JSON.parse(val!); + } else { + return def; + } } - getRawJson(name: string): string | undefined { - return this.feature(name).getRawJson(); + getRawJson(name: string, def?: string): string | undefined { + const fsh = this.feature(name); + return fsh.isSet() ? fsh.getRawJson() : def; } - getFlag(name: string): boolean | undefined { - return this.feature(name).getFlag(); + getFlag(name: string, def?: boolean): boolean | undefined { + const fsh = this.feature(name); + return fsh.isSet() ? fsh.getBoolean() : def; } - getBoolean(name: string): boolean | undefined { - return this.feature(name).getBoolean(); + getBoolean(name: string, def?: boolean): boolean | undefined { + const fsh = this.feature(name); + return fsh.isSet() ? fsh.getBoolean() : def; } abstract build(): Promise; diff --git a/featurehub-javascript-client-sdk/app/feature_state_holders.ts b/featurehub-javascript-client-sdk/app/feature_state_holders.ts index c1163b4..cc7468c 100644 --- a/featurehub-javascript-client-sdk/app/feature_state_holders.ts +++ b/featurehub-javascript-client-sdk/app/feature_state_holders.ts @@ -3,11 +3,21 @@ import { FeatureState, FeatureValueType } from './models'; import { ClientContext } from './client_context'; import { InternalFeatureRepository } from './internal_feature_repository'; import { ListenerUtils } from './listener_utils'; +import {fhLog} from "./feature_hub_config"; + +interface ListenerTracker { + listener: FeatureListener; + holder: FeatureStateHolder; +} + +interface ListenerOriginal { + value: any; +} export class FeatureStateBaseHolder implements FeatureStateHolder { protected internalFeatureState: FeatureState | undefined; protected _key: string; - protected listeners: Map = new Map(); + protected listeners: Map = new Map(); protected _repo: InternalFeatureRepository; protected _ctx: ClientContext | undefined; // eslint-disable-next-line no-use-before-define @@ -76,9 +86,13 @@ export class FeatureStateBaseHolder implements FeatureStateHolder { const pos = ListenerUtils.newListenerKey(this.listeners); if (this._ctx !== undefined) { - this.listeners.set(pos, () => listener(this)); + this.listeners.set(pos, { + listener: () => listener(this), holder: this + } ); } else { - this.listeners.set(pos, listener); + this.listeners.set(pos, { + listener: listener, holder: this + }); } return pos; @@ -127,13 +141,33 @@ export class FeatureStateBaseHolder implements FeatureStateHolder { const existingValue = this._getValue(); const existingLocked = this.locked; + // capture all the original values of the listeners + const listenerValues: Map = new Map(); + this.listeners.forEach((value, key) => { + listenerValues.set(key, { + value: value.holder.value + }) + }); + this.internalFeatureState = fs; - const changed = existingLocked !== this.featureState()?.l || existingValue !== this._getValue(fs?.type); + // the lock changing is not part of the contextual evaluation of values changing, and is constant across all listeners. + const changedLocked = existingLocked !== this.featureState()?.l; + // did at least the default value change, even if there are no listeners for the state? + let changed = changedLocked || existingValue !== this._getValue(fs?.type); - if (changed) { - this.notifyListeners(); - } + this.listeners.forEach((value, key) => { + const original = listenerValues.get(key); + if (changedLocked || original?.value !== value.holder.value) { + changed = true; + + try { + value.listener(value.holder); + } catch (e) { + fhLog.error(`Failed to trigger listener`, e); + } + } + }); return changed; } @@ -163,13 +197,9 @@ export class FeatureStateBaseHolder implements FeatureStateHolder { } triggerListeners(feature: FeatureStateHolder): void { - this.notifyListeners(feature); - } - - protected notifyListeners(feature?: FeatureStateHolder): void { this.listeners.forEach((l) => { try { - l(feature || this); + l.listener(feature || this); } catch (e) { // } // don't care diff --git a/featurehub-javascript-client-sdk/app/strategy_matcher.ts b/featurehub-javascript-client-sdk/app/strategy_matcher.ts index c83f980..e66a402 100644 --- a/featurehub-javascript-client-sdk/app/strategy_matcher.ts +++ b/featurehub-javascript-client-sdk/app/strategy_matcher.ts @@ -21,7 +21,7 @@ export class Murmur3PercentageCalculator implements PercentageCalculator { private readonly MAX_PERCENTAGE = 1000000; public determineClientPercentage(percentageText: string, featureId: string): number { - const result = murmur3(percentageText + featureId); + const result = murmur3(percentageText + featureId, 0); return Math.floor(result / Math.pow(2, 32) * this.MAX_PERCENTAGE); } } diff --git a/featurehub-javascript-client-sdk/package.json b/featurehub-javascript-client-sdk/package.json index 42d6211..da08997 100644 --- a/featurehub-javascript-client-sdk/package.json +++ b/featurehub-javascript-client-sdk/package.json @@ -1,6 +1,6 @@ { "name": "featurehub-javascript-client-sdk", - "version": "1.3.2", + "version": "1.3.3", "description": "FeatureHub client/browser SDK", "author": "info@featurehub.io", "sideEffects": false, diff --git a/featurehub-javascript-client-sdk/test/client_context_spec.ts b/featurehub-javascript-client-sdk/test/client_context_spec.ts index 8d50603..66c1358 100644 --- a/featurehub-javascript-client-sdk/test/client_context_spec.ts +++ b/featurehub-javascript-client-sdk/test/client_context_spec.ts @@ -1,7 +1,7 @@ import { EdgeService, FeatureEnvironmentCollection, - FeatureState, + FeatureState, FeatureStateHolder, FeatureValueType, LocalClientContext, StrategyAttributeCountryName, @@ -11,6 +11,8 @@ import { import { Substitute, Arg, SubstituteOf } from '@fluffy-spoon/substitute'; import { ClientEvalFeatureContext, ServerEvalFeatureContext, InternalFeatureRepository } from '../app'; import { expect } from 'chai'; +import {FeatureStateBaseHolder} from "../app/feature_state_holders"; +import {server} from "sinon"; describe('Client context should be able to encode as expected', () => { let repo: SubstituteOf; @@ -58,6 +60,13 @@ describe('Client context should be able to encode as expected', () => { await serverContext.userKey('DJElif') .sessionKey('VirtualBurningMan1').build(); edge.received(1).close(); + const fhSubst = Substitute.for>(); + repo.feature('joy').returns(fhSubst); + fhSubst.isSet().returns(false); + expect(serverContext.getFlag("joy", true)).to.be.true; + expect(serverContext.getString("joy", "loopypro")).to.eq("loopypro"); + expect(serverContext.getJson('joy', {x:1})).to.deep.eq({x:1}); + expect(serverContext.getRawJson('joy', 'raw-json')).to.eq('raw-json'); }); it('the client context should not trigger a context change or a not ready', async () => { diff --git a/featurehub-javascript-client-sdk/test/ripple_feature_update_spec.ts b/featurehub-javascript-client-sdk/test/ripple_feature_update_spec.ts new file mode 100644 index 0000000..2f87eb7 --- /dev/null +++ b/featurehub-javascript-client-sdk/test/ripple_feature_update_spec.ts @@ -0,0 +1,212 @@ +import { + AnalyticsCollector, + BaseClientContext, + ClientContext, + FeatureRolloutStrategy, + FeatureRolloutStrategyAttribute, + FeatureState, + FeatureStateHolder, + FeatureStateValueInterceptor, + FeatureValueType, + InterceptorValueMatch, + InternalFeatureRepository, + PostLoadNewFeatureStateAvailableListener, + Readyness, + ReadynessListener, + RolloutStrategyAttributeConditional, + RolloutStrategyFieldType, + SSEResultState +} from "../app"; +import {Arg, Substitute, SubstituteOf} from "@fluffy-spoon/substitute"; +import {FeatureStateBaseHolder} from "../app/feature_state_holders"; +import {expect} from "chai"; +import {Applied, ApplyFeature} from "../app/strategy_matcher"; + +class TestingContext extends BaseClientContext { + build(): Promise { + throw new Error("Method not implemented."); + } + + feature(name: string): FeatureStateHolder { + throw new Error("Method not implemented."); + } + + close(): void { + throw new Error("Method not implemented."); + } + + constructor(repository: InternalFeatureRepository) { + super(repository); + } +} + +class FakeInternalRepository implements InternalFeatureRepository { + private applier = new ApplyFeature(); + + notReady(): void { + throw new Error("Method not implemented."); + } + + notify(state: SSEResultState, data: any) { + throw new Error("Method not implemented."); + } + + valueInterceptorMatched(key: string): InterceptorValueMatch { + return new InterceptorValueMatch(undefined); + } + + apply(strategies: FeatureRolloutStrategy[], key: string, featureValueId: string, context: ClientContext): Applied { + return this.applier.apply(strategies, key, featureValueId, context); + } + + readyness: Readyness = Readyness.Ready; + catchAndReleaseMode: boolean = false; + + logAnalyticsEvent(action: string, other?: Map, ctx?: ClientContext) { + throw new Error("Method not implemented."); + } + + hasFeature(key: string): FeatureStateHolder { + throw new Error("Method not implemented."); + } + + feature(key: string): FeatureStateHolder { + throw new Error("Method not implemented."); + } + + getFeatureState(key: string): FeatureStateHolder { + throw new Error("Method not implemented."); + } + + release(disableCatchAndRelease?: boolean): Promise { + throw new Error("Method not implemented."); + } + + simpleFeatures(): Map { + throw new Error("Method not implemented."); + } + + getFlag(key: string): boolean { + throw new Error("Method not implemented."); + } + + getString(key: string): string { + throw new Error("Method not implemented."); + } + + getJson(key: string): string { + throw new Error("Method not implemented."); + } + + getNumber(key: string): number { + throw new Error("Method not implemented."); + } + + isSet(key: string): boolean { + throw new Error("Method not implemented."); + } + + addValueInterceptor(interceptor: FeatureStateValueInterceptor): void { + throw new Error("Method not implemented."); + } + + addReadynessListener(listener: ReadynessListener): number { + throw new Error("Method not implemented."); + } + + addReadinessListener(listener: ReadynessListener, ignoreNotReadyOnRegister?: boolean): number { + throw new Error("Method not implemented."); + } + + removeReadinessListener(listener: number | ReadynessListener) { + throw new Error("Method not implemented."); + } + + addAnalyticCollector(collector: AnalyticsCollector): void { + throw new Error("Method not implemented."); + } + + addPostLoadNewFeatureStateAvailableListener(listener: PostLoadNewFeatureStateAvailableListener): number { + throw new Error("Method not implemented."); + } + + removePostLoadNewFeatureStateAvailableListener(listener: number | PostLoadNewFeatureStateAvailableListener) { + throw new Error("Method not implemented."); + } + +} + + +describe('When checking for listeners triggering on strategy changes', () => { + let repo: FakeInternalRepository; + let applied: ApplyFeature; + + beforeEach(() => { + repo = new FakeInternalRepository(); + }); + + it('should ripple listener changes down', () => { + const key = 'feature-value'; + const f1 = new FeatureStateBaseHolder(repo, key); + const ctx = new TestingContext(repo).attributeValue('testing', 'x'); + + let listener1Result: any = undefined; + let listener2Result: any = undefined; + let listener1TriggerCounter = 0; + let listener2TriggerCounter = 0; + + const listener1 = f1.addListener((ft1) => { + listener1Result = ft1; + listener1TriggerCounter++; + }); + + const listener2 = f1.withContext(ctx).addListener((ft1) => { + listener2Result = ft1; + listener2TriggerCounter++; + }); + + const feature1 = { + id: '1', key: key, l: false, + version: 1, type: FeatureValueType.Boolean, + value: false, + strategies: [{ + id: 'abcd', value: true, attributes: [{ + conditional: RolloutStrategyAttributeConditional.Equals, + fieldName: 'testing', + type: RolloutStrategyFieldType.String, + values: ['x'] + } as FeatureRolloutStrategyAttribute] + } as FeatureRolloutStrategy] + } as FeatureState; + + f1.setFeatureState(feature1); + + expect(listener1Result).to.not.be.undefined; + expect(listener2Result).to.not.be.undefined; + let l1Result = listener1Result as FeatureStateHolder; + let l2Result = listener2Result as FeatureStateHolder; + expect(l1Result?.flag).to.be.false; + expect(l2Result?.flag).to.be.true; + expect(listener2TriggerCounter).to.eq(1); + expect(listener1TriggerCounter).to.eq(1); + + const featureV2 = { + id: '1', key: key, l: false, + version: 2, type: FeatureValueType.Boolean, + value: false, + strategies: [{ + id: 'abcd', value: false, attributes: [{ + conditional: RolloutStrategyAttributeConditional.Equals, + fieldName: 'testing', + type: RolloutStrategyFieldType.String, + values: ['y'] + } as FeatureRolloutStrategyAttribute] + } as FeatureRolloutStrategy] + } as FeatureState; + + f1.setFeatureState(featureV2); + expect(listener2TriggerCounter).to.eq(2); // this should trigger again as the strategy changed + expect(listener1TriggerCounter).to.eq(1); + expect(listener2Result.flag).to.be.false; + }); +}); \ No newline at end of file From 7b81fb68b8fe59baa0459ec3b309654975518253 Mon Sep 17 00:00:00 2001 From: Irina Southwell Date: Fri, 26 Jan 2024 17:34:23 +1300 Subject: [PATCH 2/2] Update featurehub-javascript-client-sdk/CHANGELOG.md --- featurehub-javascript-client-sdk/CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/featurehub-javascript-client-sdk/CHANGELOG.md b/featurehub-javascript-client-sdk/CHANGELOG.md index 6420a99..a33aa8d 100644 --- a/featurehub-javascript-client-sdk/CHANGELOG.md +++ b/featurehub-javascript-client-sdk/CHANGELOG.md @@ -1,5 +1,5 @@ #### 1.3.3 -- listeners were not being fired when added to contexts that matched strategies. (bugfix) +- listeners were not being fired when added to contexts that matched strategies. [bugfix](https://github.com/featurehub-io/featurehub-javascript-sdk/issues/196) - all the getX methods on the Context now have defaults, so you can say fhContext.getFlag("feature", false) and if it isn't set or doesn't exist, it will return false. This is an optional field so it doesn't break existing code. (feature) - set murmur3 hash seed value to 0 to be consistent with all SDK apis (bugfix).