feat: getExperimentContainer helper function for CMS app integration #123
Conversation
There was a problem hiding this comment.
LGTM once comments expanded a bit, but I'd recommend getting another stamp from @leoromanovsky before merging 👍🏽
| controlContainer, | ||
| ); | ||
| expect(loggerWarnSpy).toHaveBeenCalled(); | ||
| }); |
| await initConfiguration(storage); | ||
| client = new EppoClient(storage); | ||
| client.setIsGracefulFailureMode(true); | ||
| flagExperiment = { |
There was a problem hiding this comment.
This doesn't use any variables set up in beforeEach() should it just be declared as a const up with the containers?
There was a problem hiding this comment.
I generally like to leave anything with Arrays and objects that are referenced by the production code in a before each to prevent against potential mutations that could affect subsequent tests, which then become difficult to diagnose. It's unlikely, but it can happen.
There was a problem hiding this comment.
If you feel strongly though, I can move it!
There was a problem hiding this comment.
Nope that's defensible too!
| expect(loggerWarnSpy).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should default to the control container if an unknown variation is assigned', async () => { |
| ); | ||
| await configurationRequestor.fetchAndStoreConfigurations(); | ||
| } | ||
| import { initConfiguration } from './test-utils'; |
src/client/eppo-client.ts
Outdated
| @@ -68,6 +68,12 @@ export type FlagConfigurationRequestParameters = { | |||
| skipInitialPoll?: boolean; | |||
| }; | |||
|
|
|||
| export interface IFlagExperiment<T> { | |||
There was a problem hiding this comment.
We may want to call this something different that is more clear what it is doing and how it is used
There was a problem hiding this comment.
Is this templatized so in case future CMS inputs take on different shapes?
There was a problem hiding this comment.
We may want to call this something different that is more clear what it is doing and how it is used
Any suggestions? We could call it IContainerExperiment?
Is this templatized so in case future CMS inputs take on different shapes?
Yes, exactly. This is looking forward a bit to subsequent CMS work.
There was a problem hiding this comment.
updated with IContainerExperiment, but let me know if you have another suggestion
src/client/eppo-client.ts
Outdated
| return controlVariation; | ||
| } | ||
| const treatmentVariationIndex = Number.parseInt(assignment.split('-')[1]); | ||
| if (treatmentVariationIndex > treatmentVariations.length) { |
There was a problem hiding this comment.
Should this be >=? (0-based indexing vs 1-based length)
There was a problem hiding this comment.
No because the numbers that are parsed from the key are 1-indexed (e.g. treatment-1, treatment-2, etc)
There was a problem hiding this comment.
I can make this more clear by the naming though. Index does make it seem like it should start with zero.
greghuels
force-pushed
the
greg/FF-3282-contentul-sdk-helper
branch
from
139d20d to
e650c4d
Compare
greghuels
force-pushed
the
greg/FF-3282-contentul-sdk-helper
branch
3 times, most recently
from
5e5ea30 to
10614d6
Compare
greghuels
force-pushed
the
greg/FF-3282-contentul-sdk-helper
branch
from
10614d6 to
04dfca0
Compare
| await initConfiguration(storage); | ||
| client = new EppoClient(storage); | ||
| client.setIsGracefulFailureMode(true); | ||
| flagExperiment = { |
There was a problem hiding this comment.
Nope that's defensible too!
| } | ||
| const treatmentVariationIndex = Number.parseInt(assignment.split('-')[1]); | ||
| if (treatmentVariationIndex > treatmentVariations.length) { | ||
| if (treatmentVariationIndex >= treatmentVariationEntries.length) { |
Fixes: FF-3282
Motivation and Context
This provides a
getExperimentContainerEntryfunction for working with content management systems that integrate with Eppo. For instance, the "Eppo" Contentful app will automatically create a new flag with variations "control", "variation-1", "variation-2", etc. ThegetExperimentContainerEntrywill map the correct CMS container (e.g. "blog post", "header image", etc) given the string assignment returned.How has this been tested?
Unit testing