From 98a6cd05c6a76107200b4e4360940a8fc2daf4ce Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Fri, 5 Aug 2022 11:09:55 +0200 Subject: [PATCH] Feat(#1873): return 'unknown' for application hostname strategies (#1889) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hostname strategy will not work correctly with the playground because it depends on external state. In its constructor, it tries to query the environment or use the os.hostname function to determine what its current hostname is. This means that no matter what the user does in the playground, they can’t affect the results of this strategy. It’s also unlikely that it will be true. And if it is, it probably won’t be true for their clients. In theory, we could accept a hostname property on the Unleash context and use the provided hostname in the address. However, I’m afraid that it’ll make users think that they can impact the hostname strategy by setting the property on their context, when that doesn’t do anything outside of the playground. It would also make the playground evaluate things differently from a regular SDK and I’m not sure that that’s something we want. Instead, this change to the API makes the feature evaluate to 'unknown' or `false` (depending on constraints). --- src/lib/util/feature-evaluator/client.ts | 18 +++++++++-- src/lib/util/offline-unleash-client.test.ts | 36 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/lib/util/feature-evaluator/client.ts b/src/lib/util/feature-evaluator/client.ts index a538786fc2..71210cd81d 100644 --- a/src/lib/util/feature-evaluator/client.ts +++ b/src/lib/util/feature-evaluator/client.ts @@ -84,9 +84,21 @@ export default class UnleashClient { const strategies = feature.strategies.map( (strategySelector): PlaygroundStrategySchema => { - const strategy = - this.getStrategy(strategySelector.name) ?? - this.getStrategy('unknown'); + const getStrategy = () => { + // the application hostname strategy relies on external + // variables to calculate its result. As such, we can't + // evaluate it in a way that makes sense. So we'll + // use the 'unknown' strategy instead. + if (strategySelector.name === 'applicationHostname') { + return this.getStrategy('unknown'); + } + return ( + this.getStrategy(strategySelector.name) ?? + this.getStrategy('unknown') + ); + }; + + const strategy = getStrategy(); const segments = strategySelector.segments diff --git a/src/lib/util/offline-unleash-client.test.ts b/src/lib/util/offline-unleash-client.test.ts index 1beca88188..0181d69378 100644 --- a/src/lib/util/offline-unleash-client.test.ts +++ b/src/lib/util/offline-unleash-client.test.ts @@ -278,6 +278,42 @@ describe('offline client', () => { ); }); + it(`returns '${playgroundStrategyEvaluation.unknownResult}' for the application hostname strategy`, async () => { + const name = 'toggle-name'; + const context = { appName: 'client-test' }; + + const client = await offlineUnleashClient({ + features: [ + { + strategies: [ + { + name: 'applicationHostname', + constraints: [], + }, + ], + stale: false, + enabled: true, + name, + type: 'experiment', + variants: [], + }, + ], + context, + logError: console.log, + }); + + const result = client.isEnabled(name, context); + + result.strategies.forEach((strategy) => + expect(strategy.result.enabled).toEqual( + playgroundStrategyEvaluation.unknownResult, + ), + ); + expect(result.result).toEqual( + playgroundStrategyEvaluation.unknownResult, + ); + }); + it('returns strategies in the order they are provided', async () => { const featureName = 'featureName'; const strategies = [