From 111adccc51cb84a721e1c9e26c019538cba8f17e Mon Sep 17 00:00:00 2001 From: b-ma Date: Thu, 3 Oct 2024 18:04:51 +0200 Subject: [PATCH] refactor: remove context from SharedState#set --- src/common/SharedState.js | 50 ++++++++++++++----------------- src/common/logger.js | 2 +- src/server/SharedStatePrivate.js | 18 ++++++----- tests/misc/deprecated.spec.js | 11 +++++++ tests/states/SharedState.spec.js | 37 ++++------------------- tests/states/StateManager.spec.js | 5 ++-- 6 files changed, 52 insertions(+), 71 deletions(-) diff --git a/src/common/SharedState.js b/src/common/SharedState.js index 7cc30cff..5677d2d7 100644 --- a/src/common/SharedState.js +++ b/src/common/SharedState.js @@ -33,8 +33,6 @@ export const kSharedStatePromiseStore = Symbol('soundworks:shared-state-promise- * applied to the state. * @param {Object} oldValues - Key / value pairs of the updated params before * the updates has been applied to the state. - * @param {Mixed} [context=null] - Optionnal context object that has been passed - * with the values updates in the `set` call. */ /** @@ -136,27 +134,27 @@ ${JSON.stringify(initValues, null, 2)}`); this[kSharedStatePromiseStore] = new PromiseStore(this.constructor.name); // add listener for state updates - this.#client.transport.addListener(`${UPDATE_RESPONSE}-${this.#id}-${this.#remoteId}`, async (reqId, updates, context) => { - const updated = await this.#commit(updates, context, true, true); + this.#client.transport.addListener(`${UPDATE_RESPONSE}-${this.#id}-${this.#remoteId}`, async (reqId, updates) => { + const updated = await this.#commit(updates, true, true); this[kSharedStatePromiseStore].resolve(reqId, updated); }); // retrieve values but do not propagate to subscriptions - this.#client.transport.addListener(`${UPDATE_ABORT}-${this.#id}-${this.#remoteId}`, async (reqId, updates, context) => { - const updated = await this.#commit(updates, context, false, true); + this.#client.transport.addListener(`${UPDATE_ABORT}-${this.#id}-${this.#remoteId}`, async (reqId, updates) => { + const updated = await this.#commit(updates, false, true); this[kSharedStatePromiseStore].resolve(reqId, updated); }); - this.#client.transport.addListener(`${UPDATE_NOTIFICATION}-${this.#id}-${this.#remoteId}`, async (updates, context) => { + this.#client.transport.addListener(`${UPDATE_NOTIFICATION}-${this.#id}-${this.#remoteId}`, async (updates) => { // https://github.com/collective-soundworks/soundworks/issues/18 // // # note: 2002-10-03 // - // `setTimeout(async () => this.#commit(updates, context, true, false));` + // `setTimeout(async () => this.#commit(updates, true, false));` // appears to be the only way to push the update commit in the next event // cycle so that `attach` can resolve before the update notification is // actually dispatched. The alternative: - // `Promise.resolve().then(() => this.#commit(updates, context, true, false))`` + // `Promise.resolve().then(() => this.#commit(updates, true, false))`` // does not behave as expected... // // However this breaks the reliability of: @@ -170,7 +168,7 @@ ${JSON.stringify(initValues, null, 2)}`); // ``` // which is far more important than the edge case reported in the issue // therefore this wont be fixed for now - this.#commit(updates, context, true, false); + this.#commit(updates, true, false); }); // --------------------------------------------- @@ -302,7 +300,7 @@ ${JSON.stringify(initValues, null, 2)}`); } } - async #commit(updates, context, propagate = true, initiator = false) { + async #commit(updates, propagate = true, initiator = false) { const newValues = {}; const oldValues = {}; @@ -336,7 +334,7 @@ ${JSON.stringify(initValues, null, 2)}`); if (propagate && Object.keys(newValues).length > 0) { this.#onUpdateCallbacks.forEach(listener => { - promises.push(listener(newValues, oldValues, context)); + promises.push(listener(newValues, oldValues)); }); } @@ -409,26 +407,23 @@ ${JSON.stringify(initValues, null, 2)}`); * ``` * * @param {object} updates - Key / value pairs of updates to apply to the state. - * @param {mixed} [context=null] - Optionnal contextual object that will be propagated - * alongside the updates of the state. The context is valid only for the - * current call and will be passed as third argument to all update listeners. * @returns {Promise} A promise to the (coerced) updates. * * @example * const state = await client.stateManager.attach('globals'); * const updates = await state.set({ myParam: Math.random() }); */ - async set(updates, context = null) { + async set(updates) { if (this.#detached) { return; } - if (!isPlainObject(updates)) { - throw new TypeError(`[SharedState] State "${this.#className}": state.set(updates[, context]) should receive an object as first parameter`); + if (isPlainObject(arguments[0]) && isPlainObject(arguments[1])) { + logger.deprecated('SharedState.set(updates, context)', 'a regular parameter set with `event=true` behavior', '4.0.0-alpha.29'); } - if (context !== null && !isPlainObject(context)) { - throw new TypeError(`[SharedState] State "${this.#className}": state.set(updates[, context]) should receive an object as second parameter`); + if (!isPlainObject(updates)) { + throw new TypeError(`[SharedState] State "${this.#className}": state.set(updates) should receive an object as first parameter`); } const newValues = {}; @@ -502,7 +497,7 @@ ${JSON.stringify(initValues, null, 2)}`); // propagate immediate params if changed if (propagateNow) { - this.#onUpdateCallbacks.forEach(listener => listener(newValues, oldValues, context)); + this.#onUpdateCallbacks.forEach(listener => listener(newValues, oldValues)); } // check if we can resolve immediately or if we need to go through network @@ -521,7 +516,7 @@ ${JSON.stringify(initValues, null, 2)}`); // go through server-side normal behavior return new Promise((resolve, reject) => { const reqId = this[kSharedStatePromiseStore].add(resolve, reject, 'SharedState#set', forwardParams); - this.#client.transport.emit(`${UPDATE_REQUEST}-${this.#id}-${this.#remoteId}`, reqId, updates, context); + this.#client.transport.emit(`${UPDATE_REQUEST}-${this.#id}-${this.#remoteId}`, reqId, updates); }); } @@ -719,10 +714,10 @@ ${JSON.stringify(initValues, null, 2)}`); * @param {sharedStateOnUpdateCallback} callback * Callback to execute when an update is applied on the state. * @param {Boolean} [executeListener=false] - Execute the callback immediately - * with current state values. (`oldValues` will be set to `{}`, and `context` to `null`) + * with current state values. Note that `oldValues` will be set to `{}`. * @returns {sharedStateDeleteOnUpdateCallback} * @example - * const unsubscribe = state.onUpdate(async (newValues, oldValues, context) => { + * const unsubscribe = state.onUpdate(async (newValues, oldValues) => { * for (let [key, value] of Object.entries(newValues)) { * switch (key) { * // do something @@ -738,8 +733,8 @@ ${JSON.stringify(initValues, null, 2)}`); if (executeListener === true) { const currentValues = this.getValues(); - // filter `event: true` parameters from currentValues, this is missleading - // as we are in the context of a callback, not from an active read + // filter `event: true` parameters from currentValues, having them here is + // misleading as we are in the context of a callback, not from an active read const classDescription = this.getDescription(); for (let name in classDescription) { @@ -749,8 +744,7 @@ ${JSON.stringify(initValues, null, 2)}`); } const oldValues = {}; - const context = null; - listener(currentValues, oldValues, context); + listener(currentValues, oldValues); } return () => { diff --git a/src/common/logger.js b/src/common/logger.js index 763d8277..7f1a0403 100644 --- a/src/common/logger.js +++ b/src/common/logger.js @@ -168,7 +168,7 @@ dependencies on both your server and clients. throw new Error(`Invalid 'logger.deprecated call: a deprecation version is required`); } - const msg = `[Deprecation Warning] '${oldAPI}' is deprecated since version ${deprecationVersion} and will be removed in next major revision, please use '${newAPI}' instead`; + const msg = `[Deprecation Warning] ${oldAPI} is deprecated (last supported version: ${deprecationVersion}) and will be removed in next major revision, please use ${newAPI} instead`; console.warn(chalk.yellow(msg)); } }; diff --git a/src/server/SharedStatePrivate.js b/src/server/SharedStatePrivate.js index 63a0d0d7..c158169f 100644 --- a/src/server/SharedStatePrivate.js +++ b/src/server/SharedStatePrivate.js @@ -94,7 +94,7 @@ class SharedStatePrivate { } // attach client listeners - client.transport.addListener(`${UPDATE_REQUEST}-${this.id}-${remoteId}`, async (reqId, updates, context) => { + client.transport.addListener(`${UPDATE_REQUEST}-${this.id}-${remoteId}`, async (reqId, updates) => { // apply registered hooks const hooks = this.#manager[kServerStateManagerGetHooks](this.className); const values = this.#parameters.getValues(); @@ -102,7 +102,7 @@ class SharedStatePrivate { // cf. https://github.com/collective-soundworks/soundworks/issues/45 for (let hook of hooks.values()) { - const result = await hook(updates, values, context); + const result = await hook(updates, values); if (result === null) { // explicit abort if hook returns null hookAborted = true; @@ -160,7 +160,8 @@ class SharedStatePrivate { // no need to filter updates on requested, is blocked on client-side client.transport.emit( `${UPDATE_RESPONSE}-${this.id}-${remoteId}`, - reqId, acknowledgedUpdates, context, + reqId, + acknowledgedUpdates ); } @@ -176,7 +177,7 @@ class SharedStatePrivate { if (Object.keys(filteredUpdates).length > 0) { peer.transport.emit( `${UPDATE_NOTIFICATION}-${this.id}-${peerRemoteId}`, - filteredUpdates, context, + filteredUpdates, ); } } @@ -187,7 +188,8 @@ class SharedStatePrivate { // no need to filter updates on requested, is blocked on client-side client.transport.emit( `${UPDATE_RESPONSE}-${this.id}-${remoteId}`, - reqId, acknowledgedUpdates, context, + reqId, + acknowledgedUpdates ); } @@ -202,7 +204,7 @@ class SharedStatePrivate { if (Object.keys(filteredUpdates).length > 0) { peer.transport.emit( `${UPDATE_NOTIFICATION}-${this.id}-${peerRemoteId}`, - filteredUpdates, context, + filteredUpdates, ); } } @@ -210,7 +212,7 @@ class SharedStatePrivate { } else { // propagate back to the requester that the update has been aborted // ignore all other attached clients. - client.transport.emit(`${UPDATE_ABORT}-${this.id}-${remoteId}`, reqId, updates, context); + client.transport.emit(`${UPDATE_ABORT}-${this.id}-${remoteId}`, reqId, updates); } } else { // retrieve values from inner state (also handle immediate approriately) @@ -220,7 +222,7 @@ class SharedStatePrivate { oldValues[name] = this.#parameters.get(name); } // aborted by hook (updates have been overriden to {}) - client.transport.emit(`${UPDATE_ABORT}-${this.id}-${remoteId}`, reqId, oldValues, context); + client.transport.emit(`${UPDATE_ABORT}-${this.id}-${remoteId}`, reqId, oldValues); } }); diff --git a/tests/misc/deprecated.spec.js b/tests/misc/deprecated.spec.js index 206bdd72..92478ece 100644 --- a/tests/misc/deprecated.spec.js +++ b/tests/misc/deprecated.spec.js @@ -70,5 +70,16 @@ describe('# deprecated API', () => { server.stateManager.deleteClass('a'); await server.stop(); }); + + it('SharedState#set(updates, context)', async () => { + const server = new Server(config); + server.stateManager.defineClass('a', a); + await server.start(); + + const state = await server.stateManager.create('a'); + state.set({}, {}); + + await server.stop(); + }); }); }); diff --git a/tests/states/SharedState.spec.js b/tests/states/SharedState.spec.js index d7fd99ac..a9e1a926 100644 --- a/tests/states/SharedState.spec.js +++ b/tests/states/SharedState.spec.js @@ -32,7 +32,7 @@ describe('# SharedState', () => { server.stop(); }); - describe('## async set(updates[, context]) => updates', () => { + describe('## async set(updates) => updates', () => { it('should throw if first argument is not an object', async () => { const a = await server.stateManager.create('a'); @@ -52,25 +52,6 @@ describe('# SharedState', () => { await a.delete(); }); - it('should throw if second argument is not an object', async () => { - const a = await server.stateManager.create('a'); - - // weird stuff with async chai, did find better solution - let errored = false; - try { - await a.set({}, 'fail'); - } catch(err) { - console.log(err.message); - errored = true; - } - - if (errored === false) { - assert.fail('should throw error'); - } - - await a.delete(); - }); - it('should throw on undefined param name', async () => { const a = await server.stateManager.create('a'); @@ -256,15 +237,13 @@ describe('# SharedState', () => { const statePromise = new Promise((resolve) => { let step = 0; - state.onUpdate((newValues, oldValues, context) => { + state.onUpdate((newValues, oldValues) => { if (step === 0) { assert.deepEqual(newValues, { bool: true, int: 42 }); assert.deepEqual(oldValues, { bool: false, int: 0 }); - assert.deepEqual(context, null); } else if (step === 1) { assert.deepEqual(newValues, { bool: false, int: 76 }); assert.deepEqual(oldValues, { bool: true, int: 42 }); - assert.deepEqual(context, { someContext: true }); resolve(); } else { reject('something wrong happened'); @@ -277,15 +256,13 @@ describe('# SharedState', () => { const attachedPromise = new Promise((resolve) => { let step = 0; - attached.onUpdate((newValues, oldValues, context) => { + attached.onUpdate((newValues, oldValues) => { if (step === 0) { assert.deepEqual(newValues, { bool: true, int: 42 }); assert.deepEqual(oldValues, { bool: false, int: 0 }); - assert.deepEqual(context, null); } else if (step === 1) { assert.deepEqual(newValues, { bool: false, int: 76 }); assert.deepEqual(oldValues, { bool: true, int: 42 }); - assert.deepEqual(context, { someContext: true }); resolve(); } else { reject('something wrong happened'); @@ -296,7 +273,7 @@ describe('# SharedState', () => { }); await state.set({ bool: true, int: 42 }); - await state.set({ bool: false, int: 76 }, { someContext: true }); + await state.set({ bool: false, int: 76 }); await Promise.all([statePromise, attachedPromise]); @@ -337,11 +314,10 @@ describe('# SharedState', () => { const a = await server.stateManager.create('a'); let onUpdateCalled = false; - const unsubsribe = a.onUpdate((newValues, oldValues, context) => { + const unsubsribe = a.onUpdate((newValues, oldValues) => { onUpdateCalled = true; assert.deepEqual(newValues, { bool: false, int: 0 }); assert.deepEqual(oldValues, {}); - assert.deepEqual(context, null); }, true); await delay(10); @@ -358,11 +334,10 @@ describe('# SharedState', () => { const state = await server.stateManager.create('with-event'); let onUpdateCalled = false; - state.onUpdate((newValues, oldValues, context) => { + state.onUpdate((newValues, oldValues) => { onUpdateCalled = true; assert.deepEqual(newValues, { int: 20 }); assert.deepEqual(oldValues, {}); - assert.deepEqual(context, null); }, true); await delay(10); diff --git a/tests/states/StateManager.spec.js b/tests/states/StateManager.spec.js index 9b0554ed..cce5c8f8 100644 --- a/tests/states/StateManager.spec.js +++ b/tests/states/StateManager.spec.js @@ -776,11 +776,10 @@ describe(`# StateManager`, () => { it('hook API should be `hook(updates, currentValues, context)`', async () => { return new Promise(async (resolve, reject) => { server.stateManager.defineClass('hooked', hookSchema); - server.stateManager.registerUpdateHook('hooked', (updates, currentValues, context) => { + server.stateManager.registerUpdateHook('hooked', (updates, currentValues) => { try { assert.deepEqual(updates, { name: 'test' }); assert.deepEqual(currentValues, { name: null, value: null }); - assert.deepEqual(context, { myContext: true }); resolve(); } catch(err) { reject(err); @@ -790,7 +789,7 @@ describe(`# StateManager`, () => { const h = await server.stateManager.create('hooked'); - await h.set({ name: 'test' }, { myContext: true }); + await h.set({ name: 'test' }); server.stateManager.deleteClass('hooked'); });