Skip to content

Commit 309fc01

Browse files
mjc1283mjc1283
authored
fix (project config manager): Only call update listener when datafile actually changes (#20)
Summary: This includes two updates to project config manager: - When datafile manager provides a new datafile, only update project config object if that object has a different revision that the current config object - Only call update listeners if the config object actually changed With these two changes, we no longer trigger PROJECT_CONFIG_UPDATE notifications unless the project config actually changed. Previously, when constructed with a valid datafile and sdk key, it would trigger two PROJECT_CONFIG_UPDATE update notifications, even though the project config only changed once (after the fetch) Test plan: Updated unit tests
1 parent e02c569 commit 309fc01

File tree

2 files changed

+144
-85
lines changed

2 files changed

+144
-85
lines changed

‎packages/optimizely-sdk/lib/core/project_config/project_config_manager.js

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,7 @@ ProjectConfigManager.prototype.__onDatafileManagerReady = function() {
131131
logger: logger,
132132
skipJSONValidation: this.skipJSONValidation,
133133
});
134-
this.__configObj = newConfigObj;
135-
this.__updateListeners.forEach(function(listener) {
136-
listener(newConfigObj);
137-
});
134+
this.__handleNewConfigObj(newConfigObj);
138135
};
139136

140137
/**
@@ -156,10 +153,7 @@ ProjectConfigManager.prototype.__onDatafileManagerUpdate = function() {
156153
logger.error(ex);
157154
}
158155
if (newConfigObj) {
159-
this.__configObj = newConfigObj;
160-
this.__updateListeners.forEach(function(listener) {
161-
listener(newConfigObj);
162-
});
156+
this.__handleNewConfigObj(newConfigObj);
163157
}
164158
};
165159

@@ -205,6 +199,27 @@ ProjectConfigManager.prototype.__validateDatafileOptions = function(datafileOpti
205199
return false;
206200
};
207201

202+
/**
203+
* Update internal project config object to be argument object when the argument
204+
* object has a different revision than the current internal project config
205+
* object. If the internal object is updated, call update listeners.
206+
* @param {Object} newConfigObj
207+
*/
208+
ProjectConfigManager.prototype.__handleNewConfigObj = function(newConfigObj) {
209+
var oldConfigObj = this.__configObj;
210+
211+
var oldRevision = oldConfigObj ? oldConfigObj.revision : 'null';
212+
if (oldRevision === newConfigObj.revision) {
213+
return;
214+
}
215+
216+
this.__configObj = newConfigObj;
217+
218+
this.__updateListeners.forEach(function(listener) {
219+
listener(newConfigObj);
220+
});
221+
};
222+
208223
/**
209224
* Returns the current project config object, or null if no project config object
210225
* is available

‎packages/optimizely-sdk/lib/core/project_config/project_config_manager.tests.js

Lines changed: 121 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,18 @@ describe('lib/core/project_config/project_config_manager', function() {
153153
return manager.onReady();
154154
});
155155

156+
it('does not call onUpdate listeners after becoming ready when constructed with a valid datafile and without sdkKey', function() {
157+
var configWithFeatures = testData.getTestProjectConfigWithFeatures();
158+
var manager = new projectConfigManager.ProjectConfigManager({
159+
datafile: configWithFeatures,
160+
});
161+
var onUpdateSpy = sinon.spy();
162+
manager.onUpdate(onUpdateSpy);
163+
return manager.onReady().then(function() {
164+
sinon.assert.notCalled(onUpdateSpy);
165+
});
166+
});
167+
156168
describe('with a datafile manager', function() {
157169
it('passes the correct options to datafile manager', function() {
158170
new projectConfigManager.ProjectConfigManager({
@@ -172,94 +184,126 @@ describe('lib/core/project_config/project_config_manager', function() {
172184
}));
173185
});
174186

175-
it('updates itself when the datafile manager is ready and then emits updates', function() {
176-
var configWithFeatures = testData.getTestProjectConfigWithFeatures();
177-
datafileManager.DatafileManager.returns({
178-
start: sinon.stub(),
179-
stop: sinon.stub(),
180-
get: sinon.stub().returns(configWithFeatures),
181-
on: sinon.stub().returns(function() {}),
182-
onReady: sinon.stub().returns(Promise.resolve())
183-
});
184-
var manager = new projectConfigManager.ProjectConfigManager({
185-
sdkKey: '12345',
186-
});
187-
assert.isNull(manager.getConfig());
188-
return manager.onReady().then(function() {
189-
assert.deepEqual(
190-
manager.getConfig(),
191-
projectConfig.createProjectConfig(configWithFeatures)
192-
);
187+
describe('when constructed with sdkKey and without datafile', function() {
188+
it('updates itself when the datafile manager is ready and then emits updates', function() {
189+
var configWithFeatures = testData.getTestProjectConfigWithFeatures();
190+
datafileManager.DatafileManager.returns({
191+
start: sinon.stub(),
192+
stop: sinon.stub(),
193+
get: sinon.stub().returns(configWithFeatures),
194+
on: sinon.stub().returns(function() {}),
195+
onReady: sinon.stub().returns(Promise.resolve())
196+
});
197+
var manager = new projectConfigManager.ProjectConfigManager({
198+
sdkKey: '12345',
199+
});
200+
assert.isNull(manager.getConfig());
201+
return manager.onReady().then(function() {
202+
assert.deepEqual(
203+
manager.getConfig(),
204+
projectConfig.createProjectConfig(configWithFeatures)
205+
);
193206

194-
var nextDatafile = testData.getTestProjectConfigWithFeatures();
195-
nextDatafile.experiments.push({
196-
key: 'anotherTestExp',
197-
status: 'Running',
198-
forcedVariations: {},
199-
audienceIds: [],
200-
layerId: '253442',
201-
trafficAllocation: [{ entityId: '99977477477747747', endOfRange: 10000 }],
202-
id: '1237847778',
203-
variations: [{ key: 'variation', id: '99977477477747747' }],
207+
var nextDatafile = testData.getTestProjectConfigWithFeatures();
208+
nextDatafile.experiments.push({
209+
key: 'anotherTestExp',
210+
status: 'Running',
211+
forcedVariations: {},
212+
audienceIds: [],
213+
layerId: '253442',
214+
trafficAllocation: [{ entityId: '99977477477747747', endOfRange: 10000 }],
215+
id: '1237847778',
216+
variations: [{ key: 'variation', id: '99977477477747747' }],
217+
});
218+
nextDatafile.revision = '36';
219+
var fakeDatafileManager = datafileManager.DatafileManager.getCall(0).returnValue;
220+
fakeDatafileManager.get.returns(nextDatafile);
221+
var updateListener = fakeDatafileManager.on.getCall(0).args[1];
222+
updateListener({ datafile: nextDatafile });
223+
assert.deepEqual(
224+
manager.getConfig(),
225+
projectConfig.createProjectConfig(nextDatafile)
226+
);
204227
});
205-
var fakeDatafileManager = datafileManager.DatafileManager.getCall(0).returnValue;
206-
fakeDatafileManager.get.returns(nextDatafile);
207-
var updateListener = fakeDatafileManager.on.getCall(0).args[1];
208-
updateListener({ datafile: nextDatafile });
209-
assert.deepEqual(
210-
manager.getConfig(),
211-
projectConfig.createProjectConfig(nextDatafile)
212-
);
213228
});
214-
});
215229

216-
it('calls onUpdate listeners when the datafile manager is ready and emits updates', function() {
217-
datafileManager.DatafileManager.returns({
218-
start: sinon.stub(),
219-
stop: sinon.stub(),
220-
get: sinon.stub().returns(testData.getTestProjectConfigWithFeatures()),
221-
on: sinon.stub().returns(function() {}),
222-
onReady: sinon.stub().returns(Promise.resolve())
223-
});
224-
var manager = new projectConfigManager.ProjectConfigManager({
225-
sdkKey: '12345',
226-
});
227-
var onUpdateSpy = sinon.spy();
228-
manager.onUpdate(onUpdateSpy);
229-
return manager.onReady().then(function() {
230-
sinon.assert.calledOnce(onUpdateSpy);
231-
var fakeDatafileManager = datafileManager.DatafileManager.getCall(0).returnValue;
232-
var updateListener = fakeDatafileManager.on.getCall(0).args[1];
233-
updateListener({ datafile: testData.getTestProjectConfigWithFeatures() });
234-
sinon.assert.calledTwice(onUpdateSpy);
235-
});
236-
});
230+
it('calls onUpdate listeners after becoming ready, and after the datafile manager emits updates', function() {
231+
datafileManager.DatafileManager.returns({
232+
start: sinon.stub(),
233+
stop: sinon.stub(),
234+
get: sinon.stub().returns(testData.getTestProjectConfigWithFeatures()),
235+
on: sinon.stub().returns(function() {}),
236+
onReady: sinon.stub().returns(Promise.resolve())
237+
});
238+
var manager = new projectConfigManager.ProjectConfigManager({
239+
sdkKey: '12345',
240+
});
241+
var onUpdateSpy = sinon.spy();
242+
manager.onUpdate(onUpdateSpy);
243+
return manager.onReady().then(function() {
244+
sinon.assert.calledOnce(onUpdateSpy);
237245

238-
it('rejects its ready promise when the datafile manager emits an invalid datafile', function(done) {
239-
var invalidDatafile = testData.getTestProjectConfig();
240-
delete invalidDatafile['projectId'];
241-
datafileManager.DatafileManager.returns({
242-
start: sinon.stub(),
243-
stop: sinon.stub(),
244-
get: sinon.stub().returns(invalidDatafile),
245-
on: sinon.stub().returns(function() {}),
246-
onReady: sinon.stub().returns(Promise.resolve())
246+
var fakeDatafileManager = datafileManager.DatafileManager.getCall(0).returnValue;
247+
var updateListener = fakeDatafileManager.on.getCall(0).args[1];
248+
var newDatafile = testData.getTestProjectConfigWithFeatures();
249+
newDatafile.revision = '36';
250+
fakeDatafileManager.get.returns(newDatafile);
251+
252+
updateListener({ datafile: newDatafile });
253+
sinon.assert.calledTwice(onUpdateSpy);
254+
});
247255
});
248-
var manager = new projectConfigManager.ProjectConfigManager({
249-
jsonSchemaValidator: jsonSchemaValidator,
250-
sdkKey: '12345',
256+
257+
it('rejects its ready promise when the datafile manager emits an invalid datafile', function(done) {
258+
var invalidDatafile = testData.getTestProjectConfig();
259+
delete invalidDatafile['projectId'];
260+
datafileManager.DatafileManager.returns({
261+
start: sinon.stub(),
262+
stop: sinon.stub(),
263+
get: sinon.stub().returns(invalidDatafile),
264+
on: sinon.stub().returns(function() {}),
265+
onReady: sinon.stub().returns(Promise.resolve())
266+
});
267+
var manager = new projectConfigManager.ProjectConfigManager({
268+
jsonSchemaValidator: jsonSchemaValidator,
269+
sdkKey: '12345',
270+
});
271+
manager.onReady().catch(function() {
272+
done();
273+
});
251274
});
252-
manager.onReady().catch(function() {
253-
done();
275+
276+
it('calls stop on its datafile manager when its stop method is called', function() {
277+
var manager = new projectConfigManager.ProjectConfigManager({
278+
sdkKey: '12345',
279+
});
280+
manager.stop();
281+
sinon.assert.calledOnce(datafileManager.DatafileManager.getCall(0).returnValue.stop);
254282
});
255283
});
256284

257-
it('calls stop on its datafile manager when its stop method is called', function() {
258-
var manager = new projectConfigManager.ProjectConfigManager({
259-
sdkKey: '12345',
285+
describe('when constructed with sdkKey and with a valid datafile', function() {
286+
it('does not call onUpdate listeners after becoming ready', function() {
287+
datafileManager.DatafileManager.returns({
288+
start: sinon.stub(),
289+
stop: sinon.stub(),
290+
get: sinon.stub().returns(testData.getTestProjectConfigWithFeatures()),
291+
on: sinon.stub().returns(function() {}),
292+
onReady: sinon.stub().returns(Promise.resolve())
293+
});
294+
var configWithFeatures = testData.getTestProjectConfigWithFeatures();
295+
var manager = new projectConfigManager.ProjectConfigManager({
296+
datafile: configWithFeatures,
297+
sdkKey: '12345',
298+
});
299+
var onUpdateSpy = sinon.spy();
300+
manager.onUpdate(onUpdateSpy);
301+
return manager.onReady().then(function() {
302+
// Datafile is the same as what it was constructed with, so should
303+
// not have called update listener
304+
sinon.assert.notCalled(onUpdateSpy);
305+
});
260306
});
261-
manager.stop();
262-
sinon.assert.calledOnce(datafileManager.DatafileManager.getCall(0).returnValue.stop);
263307
});
264308
});
265309
});

0 commit comments

Comments
 (0)