Merge pull request #17678 from overleaf/em-diff-as-share-js-ops

Make diffAsShareJsOps() synchronous

GitOrigin-RevId: 5a3f96cc2f4b81e27a4ee971667f52c48c07e938

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
   },
 }

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)
+              }
+            )
+          }
         })
       }
     )

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 },
+      ])
     })
   })
 })

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,