From 2b8206d1c574ec82b3d1687515d1e4db8652573b Mon Sep 17 00:00:00 2001 From: Eric Maciel Date: Tue, 5 Jan 2021 18:30:40 -0500 Subject: [PATCH] fix: old state passed to connection error --- packages/backend/src/AutomergeBackend.ts | 5 +- packages/backend/src/SocketIOConnection.ts | 2 +- .../bridge/src/connection/connection.spec.ts | 51 +++++++ packages/bridge/src/cursor/index.ts | 37 ++--- packages/client/package.json | 26 +++- packages/client/src/automerge-editor.ts | 20 ++- packages/client/src/client.spec.ts | 136 ++++++++++++++++++ packages/client/src/withAutomerge.ts | 32 +++-- packages/client/src/withSocketIO.ts | 1 + 9 files changed, 265 insertions(+), 45 deletions(-) create mode 100644 packages/bridge/src/connection/connection.spec.ts create mode 100644 packages/client/src/client.spec.ts diff --git a/packages/backend/src/AutomergeBackend.ts b/packages/backend/src/AutomergeBackend.ts index adba9ac..df07069 100644 --- a/packages/backend/src/AutomergeBackend.ts +++ b/packages/backend/src/AutomergeBackend.ts @@ -93,10 +93,7 @@ class AutomergeBackend { const sync = toSync({ cursors: {}, children: data }) const doc = Automerge.from(sync) - - if (!this.documentSetMap[docId]) { - this.documentSetMap[docId] = new Automerge.DocSet() - } + this.documentSetMap[docId] = new Automerge.DocSet() this.documentSetMap[docId].setDoc(docId, doc) } catch (e) { console.error(e, docId) diff --git a/packages/backend/src/SocketIOConnection.ts b/packages/backend/src/SocketIOConnection.ts index e32c9a2..afdd17b 100644 --- a/packages/backend/src/SocketIOConnection.ts +++ b/packages/backend/src/SocketIOConnection.ts @@ -10,7 +10,7 @@ import { SyncDoc, CollabAction, toJS } from '@hiveteams/collab-bridge' import { getClients } from './utils' import AutomergeBackend from './AutomergeBackend' -import { debugCollabBackend } from 'utils/debug' +import { debugCollabBackend } from './utils/debug' export interface SocketIOCollaborationOptions { entry: Server diff --git a/packages/bridge/src/connection/connection.spec.ts b/packages/bridge/src/connection/connection.spec.ts new file mode 100644 index 0000000..387610b --- /dev/null +++ b/packages/bridge/src/connection/connection.spec.ts @@ -0,0 +1,51 @@ +import * as Automerge from 'automerge' + +interface TestDoc { + _id: string + status: string +} + +// TODO: delete this? +describe('old state error replication', () => { + const clientDocSet = new Automerge.DocSet() + const serverDocSet = new Automerge.DocSet() + + const docId = 'test' + let clientDoc = Automerge.from({ + _id: docId, + status: 'Unstarted' + }) + let serverDoc = Automerge.from({ + _id: docId, + status: 'Unstarted' + }) + + it('replicate old state error', () => { + clientDocSet.setDoc(docId, clientDoc) + serverDocSet.setDoc(docId, serverDoc) + + let clientMessages: string[] = [] + const clientConnection = new Automerge.Connection(clientDocSet, msg => { + clientMessages.push(JSON.stringify(msg)) + }) + clientConnection.open() + let serverMessages: string[] = [] + const serverConnection = new Automerge.Connection(serverDocSet, msg => { + serverMessages.push(JSON.stringify(msg)) + }) + serverConnection.open() + + let oldClientDoc = clientDoc + clientDoc = Automerge.change(clientDoc, newClientDoc => { + newClientDoc.status = 'In progress' + }) + clientDocSet.setDoc(docId, clientDoc) + + expect(clientMessages.length).toEqual(2) + expect(serverMessages.length).toEqual(1) + + expect(() => { + clientDocSet.setDoc(docId, oldClientDoc) + }).toThrow() + }) +}) diff --git a/packages/bridge/src/cursor/index.ts b/packages/bridge/src/cursor/index.ts index 3c56b73..81e6054 100644 --- a/packages/bridge/src/cursor/index.ts +++ b/packages/bridge/src/cursor/index.ts @@ -1,6 +1,7 @@ import { Operation, Range } from 'slate' import { CursorData } from '../model' +import { toJS } from '../utils' export const setCursor = ( id: string, @@ -9,26 +10,30 @@ export const setCursor = ( operations: Operation[], cursorData: CursorData ) => { - const cursorOps = operations.filter(op => op.type === 'set_selection') + try { + const cursorOps = operations.filter(op => op.type === 'set_selection') - if (!doc.cursors) doc.cursors = {} + if (!doc.cursors) doc.cursors = {} - const newCursor = cursorOps[cursorOps.length - 1]?.newProperties || {} + const newCursor = cursorOps[cursorOps.length - 1]?.newProperties || {} - if (selection) { - const newCursorData = Object.assign( - (doc.cursors[id] && JSON.parse(doc.cursors[id])) || {}, - newCursor, - selection, - { - ...cursorData, - isForward: Range.isForward(selection) - } - ) + if (selection) { + const newCursorData = Object.assign( + (doc.cursors[id] && JSON.parse(doc.cursors[id])) || {}, + newCursor, + selection, + { + ...cursorData, + isForward: Range.isForward(selection) + } + ) - doc.cursors[id] = JSON.stringify(newCursorData) - } else { - delete doc.cursors[id] + doc.cursors[id] = JSON.stringify(newCursorData) + } else { + delete doc.cursors[id] + } + } catch (e) { + console.error(e, toJS(doc)) } return doc diff --git a/packages/client/package.json b/packages/client/package.json index da9b6e4..fa2dcbb 100644 --- a/packages/client/package.json +++ b/packages/client/package.json @@ -21,7 +21,8 @@ "build:module": "npm run build:types && npm run build:js", "build:types": "tsc --emitDeclarationOnly", "build:js": "babel src --out-dir lib --extensions \".ts,.tsx\" --source-maps inline", - "watch": "yarn build:js -w" + "watch": "yarn build:js -w", + "test": "jest" }, "dependencies": { "@babel/plugin-proposal-optional-chaining": "^7.9.0", @@ -41,10 +42,29 @@ "@babel/preset-env": "^7.6.0", "@babel/preset-typescript": "^7.6.0", "@types/react": "^16.9.34", - "@types/socket.io-client": "^1.4.32" + "@types/socket.io-client": "^1.4.32", + "@hiveteams/collab-backend": "^0.7.16", + "@types/jest": "^24.9.0", + "jest": "^26.6.3", + "ts-jest": "^26.4.4" }, "directories": { "lib": "lib" }, - "gitHead": "89dd1657ba1b39db298e00a380f45089b8b52a91" + "gitHead": "89dd1657ba1b39db298e00a380f45089b8b52a91", + "jest": { + "preset": "ts-jest", + "globals": { + "ts-jest": { + "babelConfig": ".babelrc" + } + }, + "roots": [ + "/src" + ], + "transform": { + "^.+\\.ts?$": "ts-jest" + }, + "testRegex": "(/__tests__/.*|(\\.|/)(test|spec))\\.tsx?$" + } } diff --git a/packages/client/src/automerge-editor.ts b/packages/client/src/automerge-editor.ts index 4926229..31d4260 100644 --- a/packages/client/src/automerge-editor.ts +++ b/packages/client/src/automerge-editor.ts @@ -33,6 +33,8 @@ export interface AutomergeEditor extends Editor { gabageCursor: () => void onCursor: (data: any) => void + + automergeCleanup: () => void } /** @@ -51,7 +53,7 @@ export const AutomergeEditor = { * Apply Slate operations to Automerge */ - applySlateOps: async ( + applySlateOps: ( e: AutomergeEditor, docId: string, operations: Operation[], @@ -63,19 +65,19 @@ export const AutomergeEditor = { throw new TypeError(`Unknown docId: ${docId}!`) } - let changed + let changed: any - for await (let op of operations) { + operations.forEach(op => { changed = Automerge.change(changed || doc, d => applyOperation(d.children, op) ) - } + }) changed = Automerge.change(changed || doc, d => { setCursor(e.clientId, e.selection, d, operations, cursorData || {}) }) - e.docSet.setDoc(docId, changed as any) + e.docSet.setDoc(docId, changed) }, /** @@ -150,14 +152,18 @@ export const AutomergeEditor = { garbageCursor: (e: AutomergeEditor, docId: string) => { const doc = e.docSet.getDoc(docId) + // if the document has already been cleaned up + // return early and do nothing + if (!doc) return + const changed = Automerge.change(doc, (d: any) => { delete d.cursors }) - e.onCursor && e.onCursor(null) - e.docSet.setDoc(docId, changed) + e.onCursor && e.onCursor(null) + e.onChange() } } diff --git a/packages/client/src/client.spec.ts b/packages/client/src/client.spec.ts new file mode 100644 index 0000000..87546d8 --- /dev/null +++ b/packages/client/src/client.spec.ts @@ -0,0 +1,136 @@ +import { createEditor, Element, Node, Transforms } from 'slate' +import * as Automerge from 'automerge' +import withAutomerge, { AutomergeOptions } from './withAutomerge' +import { SyncDoc, toJS } from '@hiveteams/collab-bridge' +import AutomergeBackend from '@hiveteams/collab-backend/lib/AutomergeBackend' +import { insertText } from '../../bridge/src/apply/text' + +describe('automerge editor client tests', () => { + const docId = 'test' + const automergeOptions: AutomergeOptions = { + docId, + onError: msg => console.log('Encountered test error', msg) + } + const editor = withAutomerge(createEditor(), automergeOptions) + const automergeBackend = new AutomergeBackend() + const clientId = 'test-client' + + /** + * Initialize a basic automerge backend + */ + + // Create a new server automerge connection with a basic send function + let serverMessages: any[] = [] + automergeBackend.appendDocument(docId, [ + { type: 'paragraph', children: [{ text: 'Hi' }] } + ]) + automergeBackend.createConnection(clientId, docId, (msg: any) => { + serverMessages.push(msg) + }) + automergeBackend.openConnection(clientId) + + // define an editor send function for the clientside automerge editor + let clientMessages: any[] = [] + editor.send = (msg: any) => { + clientMessages.push(msg) + } + + // open the editor connection + editor.openConnection() + + /** + * Helper function to flush client messages and send them to the server + */ + const sendClientMessagesToServer = () => { + // console.log('clientMessages', JSON.stringify(clientMessages)) + clientMessages.forEach(msg => { + automergeBackend.receiveOperation(clientId, msg) + }) + clientMessages = [] + } + + /** + * Helper function to flush server messages and send them to the client + */ + const receiveMessagesFromServer = () => { + console.log('serverMessages', JSON.stringify(serverMessages)) + serverMessages.forEach(msg => { + editor.receiveOperation(msg) + }) + serverMessages = [] + } + + afterEach(() => { + sendClientMessagesToServer() + receiveMessagesFromServer() + }) + + it('should properly receiveDocument', () => { + const initialDocData = Automerge.save(automergeBackend.getDocument(docId)) + editor.receiveDocument(initialDocData) + + expect(editor.children.length).toEqual(1) + const paragraphNode = editor.children[0] as Element + expect(paragraphNode.type).toEqual('paragraph') + expect(paragraphNode.children.length).toEqual(1) + expect(Node.string(paragraphNode)).toEqual('Hi') + }) + + it('should sync insert node operation with server', done => { + Transforms.insertNodes(editor, { + type: 'paragraph', + children: [{ text: 'a' }] + }) + + // ensure that we eventually send a message for the insert_node oepration + const handle = setInterval(() => { + sendClientMessagesToServer() + receiveMessagesFromServer() + + const serverDoc = toJS(automergeBackend.getDocument(docId)) + if (serverDoc.children.length === 2) { + const paragraphNode = serverDoc.children[1] + expect(Node.string(paragraphNode)).toEqual('a') + clearInterval(handle) + done() + } + }, 10) + }) + + it('should sync insert text operation with client', done => { + const serverDoc = automergeBackend.getDocument(docId) + + const updatedServerDoc = Automerge.change(serverDoc, newServerDoc => { + insertText(newServerDoc as any, { + type: 'insert_text', + path: [1, 0], + offset: 1, + text: 'b' + }) + }) + automergeBackend.documentSetMap[docId].setDoc(docId, updatedServerDoc) + + // ensure that we eventually send a message for the insert_node oepration + const handle = setInterval(() => { + sendClientMessagesToServer() + receiveMessagesFromServer() + const [, secondParagraph] = editor.children + console.log(secondParagraph) + if (Node.string(secondParagraph) === 'ab') { + clearInterval(handle) + done() + } + }, 10) + }) + + // it('replicate old state error', done => { + // serverConnection.close() + // serverConnection = new Automerge.Connection(serverDocSet, msg => { + // serverMessages.push(msg) + // }) + // serverConnection.open() + + // sendClientMessagesToServer() + // receiveMessagesFromServer() + // }) +}) diff --git a/packages/client/src/withAutomerge.ts b/packages/client/src/withAutomerge.ts index e822cf1..4dea830 100644 --- a/packages/client/src/withAutomerge.ts +++ b/packages/client/src/withAutomerge.ts @@ -34,21 +34,15 @@ const withAutomerge = ( e.docSet = new Automerge.DocSet() - const createConnection = () => { - e.connection = AutomergeEditor.createConnection(e, (data: CollabAction) => - //@ts-ignore - e.send(data) - ) - - e.connection.open() - } - /** * Open Automerge Connection */ e.openConnection = () => { - createConnection() + e.connection = AutomergeEditor.createConnection(e, (data: CollabAction) => + //@ts-ignore + e.send(data) + ) e.connection.open() } @@ -76,6 +70,10 @@ const withAutomerge = ( } } + e.automergeCleanup = () => { + e.docSet = new Automerge.DocSet() + } + /** * Editor onChange */ @@ -84,9 +82,11 @@ const withAutomerge = ( const operations: any = e.operations if (!e.isRemote) { - AutomergeEditor.applySlateOps(e, docId, operations, cursorData).catch( - onError - ) + try { + AutomergeEditor.applySlateOps(e, docId, operations, cursorData) + } catch (err) { + onError(err) + } onChange() } @@ -97,7 +97,11 @@ const withAutomerge = ( */ e.receiveDocument = data => { - AutomergeEditor.receiveDocument(e, docId, data) + try { + AutomergeEditor.receiveDocument(e, docId, data) + } catch (err) { + onError(err) + } } /** diff --git a/packages/client/src/withSocketIO.ts b/packages/client/src/withSocketIO.ts index a93c4fc..621f6a2 100644 --- a/packages/client/src/withSocketIO.ts +++ b/packages/client/src/withSocketIO.ts @@ -115,6 +115,7 @@ const withSocketIO = ( e.destroy = () => { socket.close() e.closeConnection() + e.automergeCleanup() } return e