diff --git a/services/document-updater/app/js/DiffCodec.js b/services/document-updater/app/js/DiffCodec.js index 5c017f0d4e..245903ca13 100644 --- a/services/document-updater/app/js/DiffCodec.js +++ b/services/document-updater/app/js/DiffCodec.js @@ -9,7 +9,7 @@ module.exports = { REMOVED: -1, UNCHANGED: 0, - diffAsShareJsOp(before, after, callback) { + diffAsShareJsOp(before, after) { const diffs = dmp.diff_main(before.join('\n'), after.join('\n')) dmp.diff_cleanupSemantic(diffs) @@ -35,6 +35,6 @@ module.exports = { throw new Error('Unknown type') } } - callback(null, ops) + return ops }, } diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index 8b6ca6e565..e97198ca05 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -200,80 +200,76 @@ const DocumentManager = { { docId, projectId, oldLines, newLines }, 'setting a document via http' ) - DiffCodec.diffAsShareJsOp(oldLines, newLines, (error, op) => { + const op = DiffCodec.diffAsShareJsOp(oldLines, newLines) + if (undoing) { + for (const o of op || []) { + o.u = true + } // Turn on undo flag for each op for track changes + } + const update = { + doc: docId, + op, + v: version, + meta: { + type: 'external', + source, + user_id: userId, + }, + } + // Keep track of external updates, whether they are for live documents + // (flush) or unloaded documents (evict), and whether the update is a no-op. + Metrics.inc('external-update', 1, { + status: op.length > 0 ? 'diff' : 'noop', + method: alreadyLoaded ? 'flush' : 'evict', + path: source, + }) + const applyUpdateIfNeeded = cb => { + if (op.length === 0) { + // Do not notify the frontend about a noop update. + // We still want to execute the callback code below + // to evict the doc if we loaded it into redis for + // this update, otherwise the doc would never be + // removed from redis. + return cb(null) + } + UpdateManager.applyUpdate(projectId, docId, update, cb) + } + applyUpdateIfNeeded(error => { if (error) { return callback(error) } - if (undoing) { - for (const o of op || []) { - o.u = true - } // Turn on undo flag for each op for track changes - } - const update = { - doc: docId, - op, - v: version, - meta: { - type: 'external', - source, - user_id: userId, - }, - } - // Keep track of external updates, whether they are for live documents - // (flush) or unloaded documents (evict), and whether the update is a no-op. - Metrics.inc('external-update', 1, { - status: op.length > 0 ? 'diff' : 'noop', - method: alreadyLoaded ? 'flush' : 'evict', - path: source, - }) - const applyUpdateIfNeeded = cb => { - if (op.length === 0) { - // Do not notify the frontend about a noop update. - // We still want to execute the callback code below - // to evict the doc if we loaded it into redis for - // this update, otherwise the doc would never be - // removed from redis. - return cb(null) - } - UpdateManager.applyUpdate(projectId, docId, update, cb) - } - applyUpdateIfNeeded(error => { - if (error) { - return callback(error) - } - // If the document was loaded already, then someone has it open - // in a project, and the usual flushing mechanism will happen. - // Otherwise we should remove it immediately since nothing else - // is using it. - if (alreadyLoaded) { - DocumentManager.flushDocIfLoaded( - projectId, - docId, - (error, result) => { - if (error) { - return callback(error) - } - callback(null, result) + // If the document was loaded already, then someone has it open + // in a project, and the usual flushing mechanism will happen. + // Otherwise we should remove it immediately since nothing else + // is using it. + if (alreadyLoaded) { + DocumentManager.flushDocIfLoaded( + projectId, + docId, + (error, result) => { + if (error) { + return callback(error) } - ) - } else { - DocumentManager.flushAndDeleteDoc( - projectId, - docId, - {}, - (error, result) => { - // There is no harm in flushing project history if the previous - // call failed and sometimes it is required - HistoryManager.flushProjectChangesAsync(projectId) + callback(null, result) + } + ) + } else { + DocumentManager.flushAndDeleteDoc( + projectId, + docId, + {}, + (error, result) => { + // There is no harm in flushing project history if the previous + // call failed and sometimes it is required + HistoryManager.flushProjectChangesAsync(projectId) - if (error) { - return callback(error) - } - callback(null, result) + if (error) { + return callback(error) } - ) - } - }) + callback(null, result) + } + ) + } }) } ) diff --git a/services/document-updater/test/unit/js/DiffCodec/DiffCodecTests.js b/services/document-updater/test/unit/js/DiffCodec/DiffCodecTests.js index 227e8b05ed..5913c64350 100644 --- a/services/document-updater/test/unit/js/DiffCodec/DiffCodecTests.js +++ b/services/document-updater/test/unit/js/DiffCodec/DiffCodecTests.js @@ -1,14 +1,3 @@ -/* eslint-disable - no-return-assign, - no-unused-vars, -*/ -// TODO: This file was created by bulk-decaffeinate. -// Fix any style issues and re-enable lint. -/* - * decaffeinate suggestions: - * DS102: Remove unnecessary code created because of implicit returns - * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md - */ const sinon = require('sinon') const { expect } = require('chai') const modulePath = '../../../../app/js/DiffCodec.js' @@ -17,80 +6,52 @@ const SandboxedModule = require('sandboxed-module') describe('DiffCodec', function () { beforeEach(function () { this.callback = sinon.stub() - return (this.DiffCodec = SandboxedModule.require(modulePath)) + this.DiffCodec = SandboxedModule.require(modulePath) }) - return describe('diffAsShareJsOps', function () { - it('should insert new text correctly', function (done) { + describe('diffAsShareJsOps', function () { + it('should insert new text correctly', function () { this.before = ['hello world'] this.after = ['hello beautiful world'] - return this.DiffCodec.diffAsShareJsOp( - this.before, - this.after, - (error, ops) => { - if (error) return done(error) - expect(ops).to.deep.equal([ - { - i: 'beautiful ', - p: 6, - }, - ]) - return done() - } - ) + const ops = this.DiffCodec.diffAsShareJsOp(this.before, this.after) + expect(ops).to.deep.equal([ + { + i: 'beautiful ', + p: 6, + }, + ]) }) - it('should shift later inserts by previous inserts', function (done) { + it('should shift later inserts by previous inserts', function () { this.before = ['the boy played with the ball'] this.after = ['the tall boy played with the red ball'] - return this.DiffCodec.diffAsShareJsOp( - this.before, - this.after, - (error, ops) => { - if (error) return done(error) - expect(ops).to.deep.equal([ - { i: 'tall ', p: 4 }, - { i: 'red ', p: 29 }, - ]) - return done() - } - ) + const ops = this.DiffCodec.diffAsShareJsOp(this.before, this.after) + expect(ops).to.deep.equal([ + { i: 'tall ', p: 4 }, + { i: 'red ', p: 29 }, + ]) }) - it('should delete text correctly', function (done) { + it('should delete text correctly', function () { this.before = ['hello beautiful world'] this.after = ['hello world'] - return this.DiffCodec.diffAsShareJsOp( - this.before, - this.after, - (error, ops) => { - if (error) return done(error) - expect(ops).to.deep.equal([ - { - d: 'beautiful ', - p: 6, - }, - ]) - return done() - } - ) + const ops = this.DiffCodec.diffAsShareJsOp(this.before, this.after) + expect(ops).to.deep.equal([ + { + d: 'beautiful ', + p: 6, + }, + ]) }) - return it('should shift later deletes by the first deletes', function (done) { + it('should shift later deletes by the first deletes', function () { this.before = ['the tall boy played with the red ball'] this.after = ['the boy played with the ball'] - return this.DiffCodec.diffAsShareJsOp( - this.before, - this.after, - (error, ops) => { - if (error) return done(error) - expect(ops).to.deep.equal([ - { d: 'tall ', p: 4 }, - { d: 'red ', p: 24 }, - ]) - return done() - } - ) + const ops = this.DiffCodec.diffAsShareJsOp(this.before, this.after) + expect(ops).to.deep.equal([ + { d: 'tall ', p: 4 }, + { d: 'red ', p: 24 }, + ]) }) }) }) diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 93d204d38e..258ab9e505 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -463,9 +463,7 @@ describe('DocumentManager', function () { this.unflushedTime, true ) - this.DiffCodec.diffAsShareJsOp = sinon - .stub() - .callsArgWith(2, null, this.ops) + this.DiffCodec.diffAsShareJsOp = sinon.stub().returns(this.ops) this.UpdateManager.applyUpdate = sinon.stub().callsArgWith(3, null) this.DocumentManager.flushDocIfLoaded = sinon.stub().callsArg(2) this.DocumentManager.flushAndDeleteDoc = sinon.stub().callsArg(3) @@ -473,7 +471,7 @@ describe('DocumentManager', function () { describe('when not loaded but with the same content', function () { beforeEach(function () { - this.DiffCodec.diffAsShareJsOp = sinon.stub().yields(null, []) + this.DiffCodec.diffAsShareJsOp = sinon.stub().returns([]) this.DocumentManager.getDoc = sinon .stub() .yields( @@ -520,7 +518,7 @@ describe('DocumentManager', function () { describe('when already loaded with the same content', function () { beforeEach(function () { - this.DiffCodec.diffAsShareJsOp = sinon.stub().yields(null, []) + this.DiffCodec.diffAsShareJsOp = sinon.stub().returns([]) this.DocumentManager.setDoc( this.project_id, this.doc_id, @@ -700,9 +698,7 @@ describe('DocumentManager', function () { { i: 'foo', p: 4 }, { d: 'bar', p: 42 }, ] - this.DiffCodec.diffAsShareJsOp = sinon - .stub() - .callsArgWith(2, null, this.ops) + this.DiffCodec.diffAsShareJsOp = sinon.stub().returns(this.ops) this.DocumentManager.setDoc( this.project_id, this.doc_id,