feat: datafile accessor #535
Conversation
…e string instead of object
…RN) to include sdkKey as well as an object/string datafile
…nput type input (WIP)
…eating a project config
There was a problem hiding this comment.
@pthompson127 I'll DM you to discuss further - I think we could try to make this more similar to the approach in Python SDK, rather than keeping track of the datafile string within project config manager.
| this.experimentsMap = this.__getExperimentsMap(configObj); | ||
| this.featuresMap = this.__getFeaturesMap(configObj, this.experimentsMap); |
There was a problem hiding this comment.
getExperimentsMap, getFeaturesMap, getMergedVariablesMap, etc. don't need to become methods of OptimizelyConfig. We could leave them as free functions, then these two lines could be:
| this.experimentsMap = this.__getExperimentsMap(configObj); | |
| this.featuresMap = this.__getFeaturesMap(configObj, this.experimentsMap); | |
| this.experimentsMap = getExperimentsMap(configObj); | |
| this.featuresMap = getFeaturesMap(configObj, this.experimentsMap); |
| @@ -20,7 +20,7 @@ import { HttpPollingDatafileManager } from '@optimizely/js-sdk-datafile-manager' | |||
| import fns from '../../utils/fns'; | |||
| import { ERROR_MESSAGES } from '../../utils/enums'; | |||
| import projectConfig from '../../core/project_config'; | |||
| import { getOptimizelyConfig } from '../optimizely_config'; | |||
| import optimizelyConfig from '../optimizely_config'; | |||
There was a problem hiding this comment.
Suggest only importing what's needed:
| import optimizelyConfig from '../optimizely_config'; | |
| import { OptimizelyConfig } from '../optimizely_config'; |
then, use OptimizelyConfig directly on line 258.
There was a problem hiding this comment.
@msohailhussain Please try this.
| if (initialDatafile && this.__configObj) { | ||
| datafileManagerConfig.datafile = initialDatafile; | ||
| if (this.__datafileObj && this.__configObj) { | ||
| datafileManagerConfig.datafile = this.__datafileObj; |
There was a problem hiding this comment.
We should pass the string version of the datafile here.
| /** | ||
| * Returns the datafile string | ||
| * @return {string} | ||
| */ | ||
| ProjectConfigManager.prototype.toDatafile = function() { | ||
| return this.__datafileStr; | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't think we want toDatafile on project config manager.
There was a problem hiding this comment.
Looks good overall! Mostly requesting minor changes.
There is one substantial issue: datafile-manager and optimizely-sdk are separate packages, and optimizely-sdk depends on a published version of datafile-manager. So we can't make changes in optimizely-sdk depending on the code in datafile-manager at the same commit. We have to release a new datafile-manager version and update the dependency version in order to bring new functionality in. In order to solve this, we can merge & release the datafile-manager changes first before the optimizely-sdk changes.
| projectConfig.__datafileStr = JSON.stringify(datafile); | ||
|
|
There was a problem hiding this comment.
I found an issue with this code that I suggested. When we already have a string because user passed in string, we will be parsing it and then re-stringify-ing it here. To avoid this unneccessary stringify, let's do this:
export function createProjectConfig(datafileObj, datafileStr=null) {
// ...
projectConfig.__datafileStr = datfileStr === null ? JSON.stringify(datafileObj) : datafileStr;
// ...
}Then, below in tryCreatingProjectConfig:
// ...
var createArgs = [newDatafileObj];
if (typeof config.datafile === 'string') {
// We know config.datafile is a valid JSON datafile string because it was validated above
createArgs.push(config.datafile)
}
var newConfigObj = this.createProjectConfig(...createArgs);
// ...| @@ -539,24 +542,54 @@ export var isFeatureExperiment = function(projectConfig, experimentId) { | |||
| return projectConfig.experimentFeatureMap.hasOwnProperty(experimentId); | |||
| }; | |||
|
|
|||
| /** | |||
| * | |||
There was a problem hiding this comment.
Please add a short description of what toDatafile returns.
| this.__datafile = datafile; | ||
| } | ||
|
|
||
| OptimizelyConfig.prototype.getDatafile = function() { |
There was a problem hiding this comment.
Please add a JSDoc comment with the return type.
| @@ -15,16 +15,6 @@ | |||
| */ | |||
| import { isFeatureExperiment } from '../project_config'; | |||
|
|
|||
| // Get Experiment Ids which are part of rollouts | |||
There was a problem hiding this comment.
I don't think there's any material difference in getRolloutExperimentIds or getFeaturesMap, rather they appear in this diff because they were reordered within the file. Could you restore them to the original order just to confirm that?
| // Note: The TS compiler determines what syntax is accepted. We're using TS version 3.3.3333. | ||
| // This seems to roughly correspond to "2018" for this setting. | ||
| ecmaVersion: 2018, |
There was a problem hiding this comment.
Oops...I thought your branch was based on https://github.com/optimizely/javascript-sdk/tree/mcarroll/optimizely-sdk-typescript, so I suggested adding some newer syntax. But I see now this PR is against master. I suppose the easiest thing now is to wait until we can merge https://github.com/optimizely/javascript-sdk/tree/mcarroll/optimizely-sdk-typescript into master before merging these changes.
We can address this later, as the first step for this PR should be separating out and pushing through the datafile manager changes.
…strings instead of objects in the expected results
|
FSC builds passing link: |
| @@ -739,31 +739,29 @@ describe('lib/core/project_config', function() { | |||
| jsonSchemaValidator: stubJsonSchemaValidator, | |||
| logger: logger, | |||
| }); | |||
| assert.deepEqual(result, configObj); | |||
| assert.deepEqual(result.configObj, configObj); | |||
| }); | |||
|
|
|||
| it('throws an error when validateDatafile throws', function() { | |||
There was a problem hiding this comment.
| it('throws an error when validateDatafile throws', function() { | |
| it('returns an error when validateDatafile throws', function() { |
| }); | ||
| assert.isNotNull(error); | ||
| }); | ||
|
|
||
| it('throws an error when jsonSchemaValidator.validate throws', function() { |
There was a problem hiding this comment.
| it('throws an error when jsonSchemaValidator.validate throws', function() { | |
| it('returns an error when jsonSchemaValidator.validate throws', function() { |
| } | ||
| return this.createProjectConfig(config.datafile); | ||
| var newConfigObj = this.createProjectConfig(...createProjectConfigArgs); |
There was a problem hiding this comment.
Why is this used here? We are not in a method of an object, this is a stand-alone function. I suggest the following change:
| var newConfigObj = this.createProjectConfig(...createProjectConfigArgs); | |
| var newConfigObj = createProjectConfig(...createProjectConfigArgs); |
There was a problem hiding this comment.
This was causing to call real function instead of stubbed function, that's why this is mandatory here.
| @@ -20,7 +20,7 @@ import { HttpPollingDatafileManager } from '@optimizely/js-sdk-datafile-manager' | |||
| import fns from '../../utils/fns'; | |||
| import { ERROR_MESSAGES } from '../../utils/enums'; | |||
| import projectConfig from '../../core/project_config'; | |||
| import { getOptimizelyConfig } from '../optimizely_config'; | |||
| import optimizelyConfig from '../optimizely_config'; | |||
There was a problem hiding this comment.
@msohailhussain Please try this.
| @@ -118,8 +103,8 @@ ProjectConfigManager.prototype.__initialize = function(config) { | |||
| if (this.__validateDatafileOptions(config.datafileOptions)) { | |||
| assign(datafileManagerConfig, config.datafileOptions); | |||
| } | |||
| if (initialDatafile && this.__configObj) { | |||
| datafileManagerConfig.datafile = initialDatafile; | |||
| if (!handleNewDatafileException) { | |||
There was a problem hiding this comment.
I prefer checking this.__configObj here, even though handleNewDatafileException can be used as an indirect signal, we should be direct. An error will be thrown if toDatafile is passed null.
| if (!handleNewDatafileException) { | |
| if (this.__configObj) { |
| @@ -23,7 +23,7 @@ import * as datafileManager from '@optimizely/js-sdk-datafile-manager'; | |||
| import projectConfig from './index'; | |||
| import { ERROR_MESSAGES, LOG_MESSAGES } from '../../utils/enums'; | |||
| import testData from '../../tests/test_data'; | |||
| import * as optimizelyConfig from '../optimizely_config/index'; | |||
| import optimizelyConfig from '../optimizely_config/index'; | |||
There was a problem hiding this comment.
Why this change? For multi-value modules, I prefer using named imports or the namespace import style as much as possible.
| sinon.assert.calledOnce(stubLogHandler.log); | ||
| var logMessage = stubLogHandler.log.args[0][1]; | ||
| assert.strictEqual(logMessage, sprintf(LOG_MESSAGES.VALID_DATAFILE, 'PROJECT_CONFIG')); |
There was a problem hiding this comment.
This message should still be logged, is it not?
| @@ -130,4 +130,6 @@ OptimizelyConfig.prototype.getDatafile = function() { | |||
| return this.__datafile; | |||
| } | |||
|
|
|||
| export default OptimizelyConfig | |||
| export default { | |||
| OptimizelyConfig: OptimizelyConfig | |||
There was a problem hiding this comment.
unable to stub, that's why passing object.
Summary
Add a
getDatafilemethod to theOptimizelyConfigobject, that returns a string version of the datafile currently being used by the instance.toDatafilefunction added in project_config module to encapsulate conversion of project config to datafile stringJSON.parseat most once.Test Plan