From 19af0f659f6e1a0c2cce0687e4fae3d6847e86e1 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Mon, 7 Sep 2026 12:52:00 +0000 Subject: [PATCH] =?UTF-8?q?=E2=99=BB=EF=B8=8F=20Extract=20`util.callbackOr?= =?UTF-8?q?Emit()`=20for=20synchronous=20error=20paths?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the moment, `Doc` branches on whether it was handed a callback in seven separate places, and `Query` and `Backend` do it once each: ```js if (callback) return callback(err); return this.emit('error', err); ``` The idiom is already recognised elsewhere, but only as a private method copy-pasted onto three classes: `Presence` and `LocalPresence` carry byte-identical `_callbackOrEmit()` methods, and `MilestoneDB` has `_callBackOrEmitError()`, which differs in capitalisation and drops the falsy-error guard. None of them was reusable here, because all three call back asynchronously via `util.nextTick()`, and every site in `doc.js` calls back synchronously. Adopting one verbatim would change when the error callbacks on `doc.create()`, `doc.del()` and `doc.submitOp()` fire, which is a breaking change for consumers rather than a refactor. This change adds the synchronous helper to `util`, next to `callEach()`, and uses it at the nine sites whose behaviour it leaves untouched. It takes the emitter explicitly rather than relying on `this`, so it also works inside the nested callbacks in `Doc#destroy()` and `Backend#close()`. Three sites in `doc.js` deliberately keep their branch. `_emitResponseError()` interleaves `_emitNothingPending()` differently between its branches, and calls back with no error on a silent rejection. `_hardRollback()` and `_clearInflightOp()` use the array form — `util.callEach()` plus a `called` boolean — which is the shape of `Presence._callEachOrEmit()`, not this helper. Note the shared helper is synchronous while the three private ones aren't, despite the near-identical name. The tests pin that down, but it's worth watching when the presence helpers are eventually folded in: mechanically rewriting `this._callbackOrEmit(error, callback)` to `util.callbackOrEmit(this, error, callback)` would quietly make every presence callback synchronous. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- lib/backend.js | 5 +--- lib/client/doc.js | 25 ++++++-------------- lib/client/query.js | 3 +-- lib/util.js | 5 ++++ test/lib-util.js | 56 +++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 70 insertions(+), 24 deletions(-) create mode 100644 test/lib-util.js diff --git a/lib/backend.js b/lib/backend.js index 1302e29de..d70e4e9c8 100644 --- a/lib/backend.js +++ b/lib/backend.js @@ -106,10 +106,7 @@ Backend.prototype.close = function(callback) { var wait = 4; var backend = this; function finish(err) { - if (err) { - if (callback) return callback(err); - return backend.emit('error', err); - } + if (err) return util.callbackOrEmit(backend, err, callback); if (--wait) return; if (callback) callback(); } diff --git a/lib/client/doc.js b/lib/client/doc.js index 1f36a8257..97c8f90a3 100644 --- a/lib/client/doc.js +++ b/lib/client/doc.js @@ -131,10 +131,7 @@ Doc.prototype.destroy = function(callback) { doc.whenNothingPending(function() { if (doc.wantSubscribe) { doc.unsubscribe(function(err) { - if (err) { - if (callback) return callback(err); - return doc.emit('error', err); - } + if (err) return util.callbackOrEmit(doc, err, callback); doc.connection._destroyDoc(doc); if (callback) callback(); }); @@ -191,8 +188,7 @@ Doc.prototype.ingestSnapshot = function(snapshot, callback) { ERROR_CODE.ERR_INGESTED_SNAPSHOT_HAS_NO_VERSION, 'Missing version in ingested snapshot. ' + this.collection + '.' + this.id ); - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } // If the doc is already created or there are ops pending, we cannot use the @@ -214,8 +210,7 @@ Doc.prototype.ingestSnapshot = function(snapshot, callback) { ERROR_CODE.ERR_DOC_MISSING_VERSION, 'Cannot ingest snapshot in doc with null version. ' + this.collection + '.' + this.id ); - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } // If we got a snapshot for a version further along than the document is // currently, issue a fetch to get the latest ops and catch us up @@ -764,8 +759,7 @@ Doc.prototype._submit = function(op, source, callback) { ); } - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } // Try to normalize the op. This removes trailing skip:0's and things like that. if (this.type.normalize) op.op = this.type.normalize(op.op); @@ -773,10 +767,7 @@ Doc.prototype._submit = function(op, source, callback) { // This has to happen before _pushOp(), because _tryCompose() applies the op // to a pending create, well before _otApply() gets a chance to check it var pathError = ot.checkOpPathsForType(this.type, op); - if (pathError) { - if (callback) return callback(pathError); - return this.emit('error', pathError); - } + if (pathError) return util.callbackOrEmit(this, pathError, callback); } try { @@ -913,8 +904,7 @@ Doc.prototype.create = function(data, type, options, callback) { } if (this.type) { var err = new ShareDBError(ERROR_CODE.ERR_DOC_ALREADY_CREATED, 'Document already exists'); - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } var op = {create: {type: type, data: data}}; var source = options && options.source; @@ -935,8 +925,7 @@ Doc.prototype.del = function(options, callback) { } if (!this.type) { var err = new ShareDBError(ERROR_CODE.ERR_DOC_DOES_NOT_EXIST, 'Document does not exist'); - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } var op = {del: true}; var source = options && options.source; diff --git a/lib/client/query.js b/lib/client/query.js index a8665dc8a..9c747f06f 100644 --- a/lib/client/query.js +++ b/lib/client/query.js @@ -149,8 +149,7 @@ Query.prototype._finishResponse = function(err, callback) { this.ready = true; if (err) { this.connection._destroyQuery(this); - if (callback) return callback(err); - return this.emit('error', err); + return util.callbackOrEmit(this, err, callback); } if (callback) callback(null, this.results, this.extra); }; diff --git a/lib/util.js b/lib/util.js index 345fa50d8..3a347d3b2 100644 --- a/lib/util.js +++ b/lib/util.js @@ -84,6 +84,11 @@ exports.callEach = function(callbacks, error) { return called; }; +exports.callbackOrEmit = function(emitter, error, callback) { + if (callback) return callback(error); + if (error) emitter.emit('error', error); +}; + exports.truthy = function(arg) { return !!arg; }; diff --git a/test/lib-util.js b/test/lib-util.js new file mode 100644 index 000000000..920b39396 --- /dev/null +++ b/test/lib-util.js @@ -0,0 +1,56 @@ +var EventEmitter = require('../lib/emitter').EventEmitter; +var util = require('../lib/util'); +var expect = require('chai').expect; + +describe('util.callbackOrEmit', function() { + var emitter; + var error; + + beforeEach(function() { + emitter = new EventEmitter(); + error = new Error('some error'); + }); + + it('calls back synchronously with the error', function() { + var calledWith = 'not called'; + util.callbackOrEmit(emitter, error, function(err) { + calledWith = err; + }); + expect(calledWith).to.equal(error); + }); + + it('calls back synchronously when there is no error', function() { + var calledWith = 'not called'; + util.callbackOrEmit(emitter, null, function(err) { + calledWith = err; + }); + expect(calledWith).to.equal(null); + }); + + it('does not emit when a callback is provided', function() { + var emitted = false; + emitter.on('error', function() { + emitted = true; + }); + util.callbackOrEmit(emitter, error, function() {}); + expect(emitted).to.equal(false); + }); + + it('emits the error when there is no callback', function() { + var emittedWith = 'not emitted'; + emitter.on('error', function(err) { + emittedWith = err; + }); + util.callbackOrEmit(emitter, error); + expect(emittedWith).to.equal(error); + }); + + it('does nothing when there is no error and no callback', function() { + var emitted = false; + emitter.on('error', function() { + emitted = true; + }); + util.callbackOrEmit(emitter, null); + expect(emitted).to.equal(false); + }); +});