fix: Checking variation in flagvariation map instead of checking it only in experiment#729
Conversation
… config also instead of getting variation from rule
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
Change looks good. We need a unit test covering the case.
| let variation = null; | ||
| variation = experiment.variationKeyMap[variationKey]; |
There was a problem hiding this comment.
Can we add unit test to catch the bug?
msohailhussain
left a comment
msohailhussain
left a comment
There was a problem hiding this comment.
lgtm. please address jae's comments.
There was a problem hiding this comment.
There is an unusual pattern of passing in config from outside, which was introduced here which will make this findValidatedForcedDecision API unusable from outside. Please see if you can find a better way to do it or let me know, we can try to figure something out together
| if (config && forcedDecision) { | ||
| variationKey = forcedDecision.variationKey; | ||
| variation = this.optimizely.getFlagVariationByKey(flagKey, variationKey); | ||
| variation = getFlagVariationByKey(config, flagKey, variationKey); |
There was a problem hiding this comment.
Why did this had to change? this looks like an unusual pattern to pass in config from outside.
| * @return {DecisionResponse<Variation|null>} DecisionResponse object containing valid variation object and decide reasons. | ||
| */ | ||
| findValidatedForcedDecision( | ||
| config: ProjectConfig, |
There was a problem hiding this comment.
This looks like an unusual pattern. why do we need to pass in config from outside when we have access to optimizely instance.
There was a problem hiding this comment.
optimizelyInstance.config may have different version if we try to access it from the optimizely instance. That's why the config we capture in the start of any API, we use it across all methods.
| getFlagVariationByKey(flagKey: string, variationKey: string): OptimizelyVariation | null { | ||
| const configObj = this.projectConfigManager.getConfig(); | ||
| if (!configObj) { | ||
| return null; | ||
| } | ||
|
|
||
| const variations = configObj.flagVariationsMap[flagKey]; | ||
| const result = find(variations, item => item.key === variationKey) | ||
| if (result) { | ||
| return result; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
why remove this and move to project_config and pass that in separately?
There was a problem hiding this comment.
As per my understanding, project_config is used for mapping all datafile properties and their k/v mappings. Optimizely class is not for this purpose. I have not seen any single mapping in this class. That's why removed it and add it at proper location where you can see all other getter and setters.
| if (!variation) { | ||
| variation = getFlagVariationByKey(configObj, feature.key, variationKey); | ||
| } |
There was a problem hiding this comment.
As far as i understand, this was the only change needed here. i am unable to understand why were other changes made at all.
There was a problem hiding this comment.
Secondly, optimizely_instance's access is not available in decide_service, that's why moved to project_config
zashraf1985
left a comment
zashraf1985
left a comment
There was a problem hiding this comment.
Based on @msohailhussain 's explanation and based on the fact that we have done the same in other SDKs, i am approving this. But i have serious concerns in general on the cyclic dependencies we are creating in the SDKs. This is definitely going to come back and bite us later. We need to think about redesigning without these cyclic dependencies later.
@jaeopt @dustin-sier @The-inside-man
jaeopt
left a comment
jaeopt
left a comment
There was a problem hiding this comment.
Question about a test case. Can you check?
packages/optimizely-sdk/lib/optimizely_user_context/index.tests.js
Outdated
Show resolved
Hide resolved
6 participants
Summary
Test plan