Skip to content

Commit

Permalink
fix: backward compatibility issues in 5.1.0 and 5.2.0 (#540)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gastonfournier
gastonfournier authored Nov 27, 2023
1 parent f985e51 commit 6bb0463
Show file tree
Hide file tree
Showing 9 changed files with 145 additions and 81 deletions.
29 changes: 20 additions & 9 deletions src/client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@ import { EventEmitter } from 'events';
import { Strategy, StrategyTransportInterface } from './strategy';
import { FeatureInterface } from './feature';
import { RepositoryInterface } from './repository';
import { Variant, VariantDefinition, defaultVariant, selectVariant } from './variant';
import {
Variant,
VariantDefinition,
VariantWithFeatureStatus,
defaultVariant,
selectVariant,
} from './variant';
import { Context } from './context';
import { Constraint, Segment, StrategyResult } from './strategy/strategy';
import { createImpressionEvent, UnleashEvents } from './events';
Expand Down Expand Up @@ -197,7 +203,7 @@ export default class UnleashClient extends EventEmitter {
}
}

getVariant(name: string, context: Context, fallbackVariant?: Variant): Variant {
getVariant(name: string, context: Context, fallbackVariant?: Variant): VariantWithFeatureStatus {
const feature = this.repository.getToggle(name);
const variant = this.resolveVariant(feature, context, true, fallbackVariant);
if (feature?.impressionData) {
Expand All @@ -218,7 +224,11 @@ export default class UnleashClient extends EventEmitter {
// This function is intended to close an issue in the proxy where feature enabled
// state gets checked twice when resolving a variant with random stickiness and
// gradual rollout. This is not intended for general use, prefer getVariant instead
forceGetVariant(name: string, context: Context, fallbackVariant?: Variant): Variant {
forceGetVariant(
name: string,
context: Context,
fallbackVariant?: Variant,
): VariantWithFeatureStatus {
const feature = this.repository.getToggle(name);
return this.resolveVariant(feature, context, true, fallbackVariant);
}
Expand All @@ -228,11 +238,11 @@ export default class UnleashClient extends EventEmitter {
context: Context,
checkToggle: boolean,
fallbackVariant?: Variant,
): Variant {
): VariantWithFeatureStatus {
const fallback = fallbackVariant || defaultVariant;

if (typeof feature === 'undefined') {
return { ...fallback, feature_enabled: false };
return { ...fallback, feature_enabled: false, featureEnabled: false };
}

let featureEnabled = !checkToggle;
Expand All @@ -241,28 +251,29 @@ export default class UnleashClient extends EventEmitter {
featureEnabled = result.enabled;

if (result.enabled && result.variant) {
return { ...result.variant, feature_enabled: featureEnabled };
return { ...result.variant, feature_enabled: featureEnabled, featureEnabled };
}

if (!result.enabled) {
return { ...fallback, feature_enabled: featureEnabled };
return { ...fallback, feature_enabled: featureEnabled, featureEnabled };
}
}

if (!feature.variants || !Array.isArray(feature.variants) || feature.variants.length === 0) {
return { ...fallback, feature_enabled: featureEnabled };
return { ...fallback, feature_enabled: featureEnabled, featureEnabled };
}

const variant: VariantDefinition | null = selectVariant(feature, context);
if (variant === null) {
return { ...fallback, feature_enabled: featureEnabled };
return { ...fallback, feature_enabled: featureEnabled, featureEnabled };
}

return {
name: variant.name,
payload: variant.payload,
enabled: true,
feature_enabled: featureEnabled,
featureEnabled,
};
}
}
151 changes: 85 additions & 66 deletions src/test/client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,16 @@ test('invalid strategy should throw', (t) => {
t.throws(() => new Client(repo, [{}]));
t.throws(() => new Client(repo, [{ name: 'invalid' }]));
t.throws(() => new Client(repo, [{ isEnabled: 'invalid' }]));
t.throws(() => new Client(repo, [{
name: 'valid', isEnabled: () => {
},
}, null]));
t.throws(
() =>
new Client(repo, [
{
name: 'valid',
isEnabled: () => {},
},
null,
]),
);
});

test('should use provided repository', (t) => {
Expand Down Expand Up @@ -111,10 +117,7 @@ test('should use a set of custom strategies', (t) => {
test('should return false a set of custom-false strategies', (t) => {
const repo = {
getToggle() {
return buildToggle('feature', true, [
{ name: 'custom-false' },
{ name: 'custom-false' },
]);
return buildToggle('feature', true, [{ name: 'custom-false' }, { name: 'custom-false' }]);
},
};

Expand Down Expand Up @@ -248,7 +251,8 @@ test('should always return defaultVariant if missing variant', (t) => {
const defaultVariant = {
enabled: false,
name: 'disabled',
feature_enabled: true
feature_enabled: true,
featureEnabled: true,
};
t.deepEqual(result, defaultVariant);

Expand All @@ -259,7 +263,8 @@ test('should always return defaultVariant if missing variant', (t) => {
type: 'string',
value: '',
},
feature_enabled: true
feature_enabled: true,
featureEnabled: true,
};
const result2 = client.getVariant('feature-but-no-variant', {}, fallback);

Expand Down Expand Up @@ -299,33 +304,34 @@ test('should trigger events on isEnabled if impressionData is true', (t) => {
});
client.isEnabled('feature-x', {}, () => false);
t.true(called);

});

test('should trigger events on unsatisfied dependency', (t) => {
let impressionCount = 0;
let recordedWarnings = [];
const repo = {
getToggle(name: string) {
if(name === 'feature-x') {
if (name === 'feature-x') {
return {
name: 'feature-x',
dependencies: [{feature: 'not-feature-x'}],
strategies: [{ name: 'default' }],
dependencies: [{ feature: 'not-feature-x' }],
strategies: [{ name: 'default' }],
variants: [],
impressionData: true,
}
};
} else {
return undefined;
}
},
};
const client = new Client(repo, []);
client.on(UnleashEvents.Impression, () => {
impressionCount++;
}).on(UnleashEvents.Warn, (warning) => {
recordedWarnings.push(warning);
});
client
.on(UnleashEvents.Impression, () => {
impressionCount++;
})
.on(UnleashEvents.Warn, (warning) => {
recordedWarnings.push(warning);
});
client.isEnabled('feature-x', {}, () => false);
client.isEnabled('feature-x', {}, () => false);
t.deepEqual(impressionCount, 2);
Expand All @@ -350,70 +356,83 @@ test('should trigger events on getVariant if impressionData is true', (t) => {
test('should favor strategy variant over feature variant', (t) => {
const repo = {
getToggle() {
return buildToggle('feature-x', true, [
{
name: 'default',
constraints: [],
variants: [{
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
weight: 1000
}],
parameters: {},
},
], [
{
name: 'willBeIgnored',
weight: 100,
payload: {
type: 'string',
value: 'willBeIgnored',
return buildToggle(
'feature-x',
true,
[
{
name: 'default',
constraints: [],
variants: [
{
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
weight: 1000,
},
],
parameters: {},
},
},
], true);
],
[
{
name: 'willBeIgnored',
weight: 100,
payload: {
type: 'string',
value: 'willBeIgnored',
},
},
],
true,
);
},
};
const client = new Client(repo, defaultStrategies);
const enabled = client.isEnabled('feature-x', {}, () => false);
const variant = client.getVariant('feature-x', {});
t.true(enabled);
t.deepEqual(variant, {
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
enabled: true,
feature_enabled: true
},
);
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
enabled: true,
feature_enabled: true,
featureEnabled: true,
});
});


test('should return disabled variant for non-matching strategy variant', (t) => {
const repo = {
getToggle() {
return buildToggle('feature-x', false, [
{
name: 'default',
constraints: [],
variants: [{
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
weight: 1000
}],
parameters: {},
},
], [], true);
return buildToggle(
'feature-x',
false,
[
{
name: 'default',
constraints: [],
variants: [
{
name: 'strategyVariantName',
payload: { type: 'string', value: 'strategyVariantValue' },
weight: 1000,
},
],
parameters: {},
},
],
[],
true,
);
},
};
const client = new Client(repo, defaultStrategies);
const enabled = client.isEnabled('feature-x', {}, () => false);
const variant = client.getVariant('feature-x', {});
t.false(enabled);
t.deepEqual(variant, {
name: 'disabled',
enabled: false,
feature_enabled: false,
},
);
name: 'disabled',
enabled: false,
feature_enabled: false,
featureEnabled: false,
});
});


5 changes: 4 additions & 1 deletion src/test/integration/client-specification.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,10 @@ specs.forEach((testName) => {
instance.on('error', reject);
instance.on('synchronized', () => {
const result = instance.getVariant(testCase.toggleName, testCase.context);
t.deepEqual(result, testCase.expectedResult);
t.deepEqual(result, {
...testCase.expectedResult,
featureEnabled: testCase.expectedResult.feature_enabled
});

instance.destroy();
resolve();
Expand Down
9 changes: 9 additions & 0 deletions src/test/snapshots/client.test.ts.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -22,6 +23,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -34,6 +36,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -48,6 +51,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -60,6 +64,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant1',
payload: {
Expand All @@ -72,6 +77,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -86,6 +92,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -98,6 +105,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant3',
payload: {
Expand All @@ -110,6 +118,7 @@ Generated by [AVA](https://avajs.dev).
{
enabled: true,
featureEnabled: true,
feature_enabled: true,
name: 'variant2',
payload: {
Expand Down
Binary file modified src/test/snapshots/client.test.ts.snap
View file
Binary file not shown.
Loading

0 comments on commit 6bb0463

Please sign in to comment.