refactor: new ConfigurationStore #256
Conversation
This integrates better with editor tools.
| @@ -0,0 +1,5 @@ | |||
| { | |||
| format: configResponse.format, | ||
| }; | ||
|
|
||
| if ( |
There was a problem hiding this comment.
We check to see if the set of bandit models we've already loaded satisfies the models referenced by the latest flag config. Limits calls to fetch the bandits to once daily (bandit model recomputation cycle)
| } | ||
|
|
||
| /** @internal */ | ||
| type ConfigurationWire = { |
There was a problem hiding this comment.
heads up, IConfigurationWire is already defined here
There was a problem hiding this comment.
Yes. I tried using it but ran into it having readonly fields, so I couldn't construct it like this:
const wire: ConfigurationWire = {
version: 1,
};
if (this.flags) {
wire.config = {
...this.flags,
response: JSON.stringify(this.flags.response),
};
}
// ...It's also quite a bit more convoluted with braided strings and class implementation, etc.
So my idea is to have the simplest definition here, just for sanity check—it's not exported and is not used for anything but Configuration.toString()/Configuration.fromString(). (The design idea is that it's just a string representation of Configuration.)
There was a problem hiding this comment.
I guess the BaseEppoClient would actually provide this functionality.
There was a problem hiding this comment.
Yeah, ConfigurationRequestor is basically doing the same thing now with the new fetchConfiguration() method (when paired with Configuration.toString())
|
|
||
| /** @internal | ||
| * | ||
| * Returns flag configuration for the given flag key. Obfuscation is |
| @@ -14,7 +15,7 @@ import { | |||
| Allocation, | |||
| Split, | |||
| VariationType, | |||
| ConfigDetails, | |||
| ); | ||
| const configFormat = flagsConfig?.response.format; | ||
| const obfuscated = configFormat !== FormatEnum.SERVER; |
There was a problem hiding this comment.
Should isObfuscated be a method on Configuration so we're not leaking the FormatEnum details?
There was a problem hiding this comment.
isObfuscated() is oversimplification in a sense that Configuration contains multiple pieces (flags, bandits, precomputed), where each could be obfuscated, not obfuscated, or simply missing. So a single isObfuscated() method doesn't cut it.
It also spreads this oversimplified model in programmers' brains, so we're more likely to miss some edge cases. I would rather have us working with raw data instead of half-baked abstractions. It may not be as pretty (initially) but at least it's obvious what's going on
re leaking details: my philosophy is that SDKs are in the know. Evaluation is highly dependent on flags response type, so there's no sense hiding it. When talking about leaking details, the only meaningful distinction for me is user–SDK. Configuration is exposing full server responses but is marking them @internal for this reason
There was a problem hiding this comment.
re FormatEnum specifically: it's currently mixing formats from two independent and unrelated configuration pieces (flags and precomputed), that actually have different rules for determining obfuscation
Next refactoring step is splitting it into FlagsConfigurationFormat and PrecomputedConfigurationFormat. And then we can have a proper isObfuscated(FlagsConfigurationFormat): boolean helper to encode this piece of knowledge
Longer-term still, we shall add isObfuscated field to flags configuration
| return undefined; | ||
| } | ||
| return { | ||
| response, |
There was a problem hiding this comment.
great steps towards passing and checking the eTag in this SDK
| obfuscated: | ||
| this.getConfiguration()?.getFlagsConfiguration()?.response.format === FormatEnum.CLIENT, |
There was a problem hiding this comment.
obfuscated computation should move somewhere centralized
WIP changes
TODO: