feat: added notification support for json variable #253
Conversation
mikeproeng37
left a comment
mikeproeng37
left a comment
There was a problem hiding this comment.
Just a comment, will leave for @mjc1283 to approve
| convertedValue, e = strconv.ParseBool(variableValue) | ||
| case entities.JSON: | ||
| convertedValue = map[string]string{} | ||
| e = json.Unmarshal([]byte(variableValue), &convertedValue) |
There was a problem hiding this comment.
I'm wondering now, if we should lazy unmarshal and cache the map in the variable itself the first time it is ever accessed and from there on we can just refer to that value. Seems like this could potentially be a costly operation to repeat
| convertedValue, e = strconv.ParseBool(variableValue) | ||
| case entities.JSON: | ||
| convertedValue = map[string]string{} | ||
| e = json.Unmarshal([]byte(variableValue), &convertedValue) |
| convertedValue, e = strconv.ParseBool(variableValue) | ||
| case entities.JSON: | ||
| convertedValue = map[string]string{} | ||
| e = json.Unmarshal([]byte(variableValue), &convertedValue) |
There was a problem hiding this comment.
Should we pass the OptimizelyJSON object to the notification, rather than the JSON string? And apply memoization to creating the OptimizelyJSON as @mikeng13 suggested?
There was a problem hiding this comment.
memoization is already done on OptimizelyJSON, the thing is that GetFeatureDecision does not know anything about OptimizelyJSON. This whole notification logic will need to be restructured.
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
Requesting changes for the memoization - @pawels-optimizely let me know if that's reasonable.
mjc1283
left a comment
mjc1283
left a comment
There was a problem hiding this comment.
Discussed with @pawels-optimizely . We think the memoization should be done, but it requires more investigation to be done separately.
Summary