Skip to content

Commit a8a2026

Browse files
rzaneclaude
rzane
and
claude
authored
chore: Refactor operation classes to functions (#88)
In this PR, I'm looking to make the code a little bit simpler. These are patterns that I'm looking to remove here: 1. Type-validation at runtime. This is overly defensive, and complicates testing. Most of these type checks happen in functions that are not public. We can rely on TypeScript to alert us to these misconfigurations. 2. Using classes when a function would suffice. The only remaining classes are `Visitor` and `Assignment`. These are both mutable objects and are exposed via the public API. This PR does not change the public API. Please review this PR with "hide whitespace" enabled, since that's the majority of the diff in test files. /no-platform --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent c657ada commit a8a2026

31 files changed

+979
-1886
lines changed

‎src/abConfiguration.test.ts‎

Lines changed: 72 additions & 121 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import ABConfiguration from './abConfiguration';
1+
import { getABVariants } from './abConfiguration';
22
import Visitor from './visitor';
33
import { createConfig } from './test-utils';
44
import type { Config } from './config';
@@ -28,160 +28,111 @@ function createVisitor(config: Config) {
2828
return visitor;
2929
}
3030

31-
describe('ABConfiguration', () => {
32-
it('requires a splitName', () => {
31+
describe('getABVariants()', () => {
32+
it('logs an error if the split does not have exactly two variants', () => {
3333
const config = setupConfig();
3434
const visitor = createVisitor(config);
3535

36-
expect(() => {
37-
// @ts-expect-error Testing missing required property
38-
new ABConfiguration({ trueVariant: 'red', visitor: visitor });
39-
}).toThrow('must provide splitName');
40-
});
41-
42-
it('requires an trueVariant', () => {
43-
const config = setupConfig();
44-
const visitor = createVisitor(config);
45-
46-
expect(() => {
47-
new ABConfiguration({ splitName: 'button_color', visitor: visitor });
48-
}).toThrow('must provide trueVariant');
49-
});
36+
getABVariants({
37+
splitName: 'element',
38+
trueVariant: 'water',
39+
visitor: visitor
40+
});
5041

51-
it('requires a visitor', () => {
52-
expect(() => {
53-
// @ts-expect-error Testing missing required property
54-
new ABConfiguration({ splitName: 'button_color', trueVariant: 'red' });
55-
}).toThrow('must provide visitor');
42+
expect(visitor.logError).toHaveBeenCalledWith('A/B for element configures split with more than 2 variants');
5643
});
5744

58-
it('allows a null trueVariant', () => {
59-
const config = setupConfig();
45+
it('does not log an error if the split registry is not loaded', () => {
46+
const config = createConfig();
6047
const visitor = createVisitor(config);
6148

62-
expect(() => {
63-
new ABConfiguration({
64-
splitName: 'button_color',
65-
// @ts-expect-error Testing null value
66-
trueVariant: null,
67-
visitor: visitor
68-
});
69-
}).not.toThrow();
49+
getABVariants({
50+
splitName: 'element',
51+
trueVariant: 'water',
52+
visitor
53+
});
54+
55+
expect(visitor.logError).not.toHaveBeenCalled();
7056
});
7157

72-
describe('#getVariants()', () => {
73-
it('logs an error if the split does not have exactly two variants', () => {
58+
describe('true variant', () => {
59+
it('accepts `true` as a fallback value', () => {
7460
const config = setupConfig();
75-
const visitor = createVisitor(config);
76-
const abConfiguration = new ABConfiguration({
77-
splitName: 'element',
78-
trueVariant: 'water',
79-
visitor: visitor
61+
const variants = getABVariants({
62+
splitName: 'button_color',
63+
trueVariant: 'true',
64+
visitor: createVisitor(config)
8065
});
8166

82-
abConfiguration.getVariants();
83-
84-
expect(visitor.logError).toHaveBeenCalledWith('A/B for element configures split with more than 2 variants');
67+
expect(variants.true).toBe('true');
8568
});
8669

87-
it('does not log an error if the split registry is not loaded', () => {
88-
const config = createConfig();
89-
90-
const visitor = createVisitor(config);
91-
const abConfiguration = new ABConfiguration({
92-
splitName: 'element',
93-
trueVariant: 'water',
94-
visitor
70+
it('is true if only one variant in the split', () => {
71+
const config = setupConfig();
72+
const variants = getABVariants({
73+
splitName: 'new_feature',
74+
trueVariant: 'true',
75+
visitor: createVisitor(config)
9576
});
9677

97-
abConfiguration.getVariants();
98-
99-
expect(visitor.logError).not.toHaveBeenCalled();
78+
expect(variants.true).toBe('true');
10079
});
10180

102-
describe('true variant', () => {
103-
it('is true if null was passed in during instantiation', () => {
104-
const config = setupConfig();
105-
const abConfiguration = new ABConfiguration({
106-
splitName: 'button_color',
107-
// @ts-expect-error Testing null value
108-
trueVariant: null,
109-
visitor: createVisitor(config)
110-
});
111-
112-
expect(abConfiguration.getVariants().true).toBe('true');
81+
it('is whatever was passed in', () => {
82+
const config = setupConfig();
83+
const variants = getABVariants({
84+
splitName: 'button_color',
85+
trueVariant: 'red',
86+
visitor: createVisitor(config)
11387
});
11488

115-
it('is true if only one variant in the split', () => {
116-
const config = setupConfig();
117-
const abConfiguration = new ABConfiguration({
118-
splitName: 'new_feature',
119-
// @ts-expect-error Testing null value
120-
trueVariant: null,
121-
visitor: createVisitor(config)
122-
});
89+
expect(variants.true).toBe('red');
90+
});
91+
});
12392

124-
expect(abConfiguration.getVariants().true).toBe('true');
93+
describe('false variant', () => {
94+
it('is the variant of the split that is not the true_variant', () => {
95+
const config = setupConfig();
96+
const variants = getABVariants({
97+
splitName: 'button_color',
98+
trueVariant: 'red',
99+
visitor: createVisitor(config)
125100
});
126101

127-
it('is whatever was passed in during instantiation', () => {
128-
const config = setupConfig();
129-
const abConfiguration = new ABConfiguration({
130-
splitName: 'button_color',
131-
trueVariant: 'red',
132-
visitor: createVisitor(config)
133-
});
134-
135-
expect(abConfiguration.getVariants().true).toBe('red');
136-
});
102+
expect(variants.false).toBe('blue');
137103
});
138104

139-
describe('false variant', () => {
140-
it('is the variant of the split that is not the true_variant', () => {
141-
const config = setupConfig();
142-
const abConfiguration = new ABConfiguration({
143-
splitName: 'button_color',
144-
trueVariant: 'red',
145-
visitor: createVisitor(config)
146-
});
147-
148-
expect(abConfiguration.getVariants().false).toBe('blue');
105+
it('is false when there is no split_registry', () => {
106+
const config = createConfig();
107+
const variants = getABVariants({
108+
splitName: 'button_color',
109+
trueVariant: 'red',
110+
visitor: createVisitor(config)
149111
});
150112

151-
it('is false when there is no split_registry', () => {
152-
const config = createConfig();
153-
154-
const abConfiguration = new ABConfiguration({
155-
splitName: 'button_color',
156-
trueVariant: 'red',
157-
visitor: createVisitor(config)
158-
});
113+
expect(variants.false).toBe('false');
114+
});
159115

160-
expect(abConfiguration.getVariants().false).toBe('false');
116+
it('is always the same if the split has more than two variants', () => {
117+
const config = setupConfig();
118+
const variants = getABVariants({
119+
splitName: 'element',
120+
trueVariant: 'earth',
121+
visitor: createVisitor(config)
161122
});
162123

163-
it('is always the same if the split has more than two variants', () => {
164-
const config = setupConfig();
165-
const abConfiguration = new ABConfiguration({
166-
splitName: 'element',
167-
trueVariant: 'earth',
168-
visitor: createVisitor(config)
169-
});
124+
expect(variants.false).toBe('fire');
125+
});
170126

171-
expect(abConfiguration.getVariants().false).toBe('fire');
127+
it('is false if only one variant in the split', () => {
128+
const config = setupConfig();
129+
const variants = getABVariants({
130+
splitName: 'new_feature',
131+
trueVariant: 'true',
132+
visitor: createVisitor(config)
172133
});
173134

174-
it('is false if only one variant in the split', () => {
175-
const config = setupConfig();
176-
const abConfiguration = new ABConfiguration({
177-
splitName: 'new_feature',
178-
// @ts-expect-error Testing null value
179-
trueVariant: null,
180-
visitor: createVisitor(config)
181-
});
182-
183-
expect(abConfiguration.getVariants().false).toBe('false');
184-
});
135+
expect(variants.false).toBe('false');
185136
});
186137
});
187138
});

‎src/abConfiguration.ts‎

Lines changed: 11 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -1,75 +1,22 @@
1+
import { getSplitVariants } from './split';
12
import Visitor from './visitor';
23

3-
export type ABConfigurationOptions = {
4+
type Options = {
45
splitName: string;
5-
trueVariant?: string;
6+
trueVariant: string;
67
visitor: Visitor;
78
};
89

9-
class ABConfiguration {
10-
private _splitName: string;
11-
private _trueVariant?: string;
12-
private _visitor: Visitor;
10+
export function getABVariants({ splitName, trueVariant, visitor }: Options) {
11+
const split = visitor.config.splitRegistry.getSplit(splitName);
12+
const splitVariants = split && getSplitVariants(split);
1313

14-
constructor(options: ABConfigurationOptions) {
15-
if (!options.splitName) {
16-
throw new Error('must provide splitName');
17-
} else if (!options.hasOwnProperty('trueVariant')) {
18-
throw new Error('must provide trueVariant');
19-
} else if (!options.visitor) {
20-
throw new Error('must provide visitor');
21-
}
22-
23-
this._splitName = options.splitName;
24-
this._trueVariant = options.trueVariant;
25-
this._visitor = options.visitor;
26-
}
27-
28-
getVariants() {
29-
const splitVariants = this._getSplitVariants();
30-
if (splitVariants && splitVariants.length > 2) {
31-
this._visitor.logError('A/B for ' + this._splitName + ' configures split with more than 2 variants');
32-
}
33-
34-
return {
35-
true: this._getTrueVariant(),
36-
false: this._getFalseVariant()
37-
};
38-
}
39-
40-
_getTrueVariant() {
41-
return this._trueVariant || 'true';
42-
}
43-
44-
_getFalseVariant() {
45-
const nonTrueVariants = this._getNonTrueVariants();
46-
return Array.isArray(nonTrueVariants) && nonTrueVariants.length !== 0 ? nonTrueVariants.sort()[0] : 'false';
14+
if (splitVariants && splitVariants.length > 2) {
15+
visitor.logError(`A/B for ${splitName} configures split with more than 2 variants`);
4716
}
4817

49-
_getNonTrueVariants() {
50-
const splitVariants = this._getSplitVariants();
18+
const nonTrueVariants = splitVariants?.filter(v => v !== trueVariant) || [];
19+
const falseVariant = nonTrueVariants.length > 0 ? nonTrueVariants.sort()[0] : 'false';
5120

52-
if (splitVariants) {
53-
const trueVariant = this._getTrueVariant();
54-
const trueVariantIndex = splitVariants.indexOf(trueVariant.toString());
55-
56-
if (trueVariantIndex !== -1) {
57-
splitVariants.splice(trueVariantIndex, 1); // remove the true variant
58-
}
59-
60-
return splitVariants;
61-
} else {
62-
return null;
63-
}
64-
}
65-
66-
_getSplit() {
67-
return this._visitor.config.splitRegistry.getSplit(this._splitName);
68-
}
69-
70-
_getSplitVariants() {
71-
return this._getSplit() && this._getSplit().getVariants();
72-
}
21+
return { true: trueVariant, false: falseVariant };
7322
}
74-
75-
export default ABConfiguration;

‎src/analyticsProvider.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import Assignment from './assignment';
1+
import type Assignment from './assignment';
22

33
export interface AnalyticsProvider {
44
trackAssignment(visitorId: string, assignment: Assignment, callback: (value: boolean) => void): void;

‎src/assignment.ts‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -31,14 +31,6 @@ class Assignment {
3131
private _isUnsynced: boolean;
3232

3333
constructor(options: AssignmentOptions) {
34-
if (!options.splitName) {
35-
throw new Error('must provide splitName');
36-
} else if (!options.hasOwnProperty('variant')) {
37-
throw new Error('must provide variant');
38-
} else if (!options.hasOwnProperty('isUnsynced')) {
39-
throw new Error('must provide isUnsynced');
40-
}
41-
4234
this._splitName = options.splitName;
4335
this._variant = options.variant;
4436
this._context = options.context;

0 commit comments

Comments
 (0)