From 4576605958af14efd6c69b81443c37771d7385f9 Mon Sep 17 00:00:00 2001 From: Ulion Date: Thu, 4 Feb 2021 15:16:35 +0800 Subject: [PATCH 1/6] Do not do shortcut merge batch operations, do it one by one for toSlateOps. --- packages/bridge/src/convert/index.ts | 8 ++--- packages/bridge/src/convert/insert.ts | 45 ++++++++++++++++++++------- packages/bridge/src/convert/remove.ts | 27 +++++++++++----- packages/bridge/src/convert/set.ts | 14 ++++++--- 4 files changed, 65 insertions(+), 29 deletions(-) diff --git a/packages/bridge/src/convert/index.ts b/packages/bridge/src/convert/index.ts index 61c837d..dedb83d 100644 --- a/packages/bridge/src/convert/index.ts +++ b/packages/bridge/src/convert/index.ts @@ -19,10 +19,12 @@ const byAction = { const rootKey = '00000000-0000-0000-0000-000000000000' const toSlateOp = (ops: Automerge.Diff[], doc: SyncDoc) => { + const tempDoc = toJS(doc) + const iterate = (acc: [any, any[]], op: Automerge.Diff): any => { const action = byAction[op.action] - const result = action ? action(op, acc, doc) : acc + const result = action ? action(op, acc, doc, tempDoc) : acc return result } @@ -34,9 +36,7 @@ const toSlateOp = (ops: Automerge.Diff[], doc: SyncDoc) => { [] ]) - const tempDoc = toJS(doc) - - return defer.flatMap(op => op(tempTree, tempDoc)).filter(op => op) + return defer.flatMap(op => op).filter(op => op) } export { toSlateOp } diff --git a/packages/bridge/src/convert/insert.ts b/packages/bridge/src/convert/insert.ts index 95b3787..45ace19 100644 --- a/packages/bridge/src/convert/insert.ts +++ b/packages/bridge/src/convert/insert.ts @@ -1,25 +1,35 @@ import * as Automerge from 'automerge' +import { Element, Node } from 'slate' import { toSlatePath, toJS } from '../utils' import { SyncDoc } from '../model' -const insertTextOp = ({ index, path, value }: Automerge.Diff) => () => ({ - type: 'insert_text', - path: toSlatePath(path), - offset: index, - text: value, - marks: [] -}) +const insertTextOp = ({ index, path, value }: Automerge.Diff) => ( + map: any, + doc: Element +) => { + const slatePath = toSlatePath(path) + const node = Node.get(doc, slatePath)! + const text = node.text! as string + node.text = [text.slice(0, index), value, text.slice(index)].join('') + return { + type: 'insert_text', + path: slatePath, + offset: index, + text: value, + marks: [] + } +} const insertNodeOp = ( { value, obj, index, path }: Automerge.Diff, doc: any -) => (map: any) => { +) => (map: any, tmpDoc: Element) => { const ops: any = [] const iterate = ({ children, ...json }: any, path: any) => { - const node = children ? { ...json, children: [] } : json + const node = toJS(children ? { ...json, children: [] } : json) ops.push({ type: 'insert_node', @@ -27,6 +37,11 @@ const insertNodeOp = ( node }) + // update the temp doc so later remove_node won't error. + const parent = Node.parent(tmpDoc, path) + const index = path[path.length - 1] + parent.children.splice(index, 0, toJS(node)) + children && children.forEach((n: any, i: any) => { const node = map[n] || Automerge.getObjectById(doc, n) @@ -48,7 +63,12 @@ const insertByType = { list: insertNodeOp } -const opInsert = (op: Automerge.Diff, [map, ops]: any, doc: SyncDoc) => { +const opInsert = ( + op: Automerge.Diff, + [map, ops]: any, + doc: SyncDoc, + tmpDoc: Element +) => { try { const { link, obj, path, index, type, value } = op @@ -61,10 +81,11 @@ const opInsert = (op: Automerge.Diff, [map, ops]: any, doc: SyncDoc) => { .concat(value) .concat(map[obj].slice(index)) : value - } else { + } + if (path) { const insert = insertByType[type] - const operation = insert && insert(op, doc) + const operation = insert && insert(op, doc)(map, tmpDoc) ops.push(operation) } diff --git a/packages/bridge/src/convert/remove.ts b/packages/bridge/src/convert/remove.ts index dff5d79..e9f783b 100644 --- a/packages/bridge/src/convert/remove.ts +++ b/packages/bridge/src/convert/remove.ts @@ -23,7 +23,9 @@ const removeTextOp = (op: Automerge.Diff) => (map: any, doc: Element) => { const text = node?.text?.[index] || '*' if (node) { - node.text = node?.text ? (node.text.slice(0, index) + node.text.slice(index + 1)) : '' + node.text = node?.text + ? node.text.slice(0, index) + node.text.slice(index + 1) + : '' } return { @@ -38,17 +40,15 @@ const removeTextOp = (op: Automerge.Diff) => (map: any, doc: Element) => { } } -const removeNodeOp = (op: Automerge.Diff) => ( - map: any, - doc: Element -) => { +const removeNodeOp = (op: Automerge.Diff) => (map: any, doc: Element) => { try { const { index, obj, path } = op const slatePath = toSlatePath(path) const parent = getTarget(doc, slatePath) - const target = parent?.children?.[index as number] || parent?.[index as number] || { children: [] } + const target = parent?.children?.[index as number] || + parent?.[index as number] || { children: [] } if (!target) { throw new TypeError('Target is not found!') @@ -62,6 +62,12 @@ const removeNodeOp = (op: Automerge.Diff) => ( throw new TypeError('Index is not a number') } + if (parent?.children?.[index as number]) { + parent.children.splice(index, 1) + } else if (parent?.[index as number]) { + parent.splice(index, 1) + } + return { type: 'remove_node', path: slatePath.length ? slatePath.concat(index) : [index], @@ -72,7 +78,12 @@ const removeNodeOp = (op: Automerge.Diff) => ( } } -const opRemove = (op: Automerge.Diff, [map, ops]: any) => { +const opRemove = ( + op: Automerge.Diff, + [map, ops]: any, + doc: any, + tmpDoc: Element +) => { try { const { index, path, obj, type } = op @@ -95,7 +106,7 @@ const opRemove = (op: Automerge.Diff, [map, ops]: any) => { const fn = key === 'text' ? removeTextOp : removeNodeOp - return [map, [...ops, fn(op)]] + return [map, [...ops, fn(op)(map, tmpDoc)]] } catch (e) { console.error(e, op, toJS(map)) diff --git a/packages/bridge/src/convert/set.ts b/packages/bridge/src/convert/set.ts index bf79618..6b42953 100644 --- a/packages/bridge/src/convert/set.ts +++ b/packages/bridge/src/convert/set.ts @@ -1,29 +1,33 @@ import * as Automerge from 'automerge' +import { Element, Node } from 'slate' import { toSlatePath, toJS } from '../utils' const setDataOp = ( { key = '', obj, path, value }: Automerge.Diff, doc: any -) => (map: any) => { +) => (map: any, tmpDoc: Element) => { + const slatePath = toSlatePath(path) + const node = Node.get(tmpDoc, slatePath) + node[key] = toJS(map?.[value] || value) return { type: 'set_node', - path: toSlatePath(path), + path: slatePath, properties: { [key]: toJS(Automerge.getObjectById(doc, obj)?.[key]) }, newProperties: { - [key]: map?.[value] || value + [key]: toJS(node[key]) } } } -const opSet = (op: Automerge.Diff, [map, ops]: any, doc: any) => { +const opSet = (op: Automerge.Diff, [map, ops]: any, doc: any, tmpDoc: any) => { const { link, value, path, obj, key } = op try { if (path && path.length && path[0] !== 'cursors') { - ops.push(setDataOp(op, doc)) + ops.push(setDataOp(op, doc)(map, tmpDoc)) } else if (map[obj]) { map[obj][key as any] = link ? map[value] : value } From 3f3257541869fdab8e9ca6f9afa12a7ec66038fc Mon Sep 17 00:00:00 2001 From: Ulion Date: Sun, 7 Feb 2021 12:55:23 +0800 Subject: [PATCH 2/6] Implement remove a key from a map op (slate set to undefined) --- packages/bridge/src/convert/remove.ts | 7 +++++++ packages/bridge/src/convert/set.ts | 2 +- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/bridge/src/convert/remove.ts b/packages/bridge/src/convert/remove.ts index e9f783b..609d627 100644 --- a/packages/bridge/src/convert/remove.ts +++ b/packages/bridge/src/convert/remove.ts @@ -3,6 +3,7 @@ import { Element } from 'slate' import { toSlatePath, toJS } from '../utils' import { getTarget } from '../path' +import { setDataOp } from './set' const removeTextOp = (op: Automerge.Diff) => (map: any, doc: Element) => { try { @@ -87,6 +88,12 @@ const opRemove = ( try { const { index, path, obj, type } = op + if (type === 'map' && path) { + // remove a key from map, mapping to slate set a key's value to undefined. + ops.push(setDataOp(op, doc)(map, tmpDoc)) + return [map, ops] + } + if ( map.hasOwnProperty(obj) && typeof map[obj] !== 'string' && diff --git a/packages/bridge/src/convert/set.ts b/packages/bridge/src/convert/set.ts index 6b42953..ac666e2 100644 --- a/packages/bridge/src/convert/set.ts +++ b/packages/bridge/src/convert/set.ts @@ -3,7 +3,7 @@ import { Element, Node } from 'slate' import { toSlatePath, toJS } from '../utils' -const setDataOp = ( +export const setDataOp = ( { key = '', obj, path, value }: Automerge.Diff, doc: any ) => (map: any, tmpDoc: Element) => { From dfbb59616a6cc2f036c8361ca4c30fcb471a5eef Mon Sep 17 00:00:00 2001 From: Ulion Date: Sun, 7 Feb 2021 12:55:42 +0800 Subject: [PATCH 3/6] Fix undefined caused toJS failure. --- packages/bridge/src/utils/index.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/bridge/src/utils/index.ts b/packages/bridge/src/utils/index.ts index 137a2c4..da1257f 100644 --- a/packages/bridge/src/utils/index.ts +++ b/packages/bridge/src/utils/index.ts @@ -6,6 +6,9 @@ import { CollabAction } from '../model' export * from './testUtils' const toJS = (node: any) => { + if (node === undefined) { + return undefined + } try { return JSON.parse(JSON.stringify(node)) } catch (e) { From 56b190257725587216de51f13a523c82397ef316 Mon Sep 17 00:00:00 2001 From: Ulion Date: Sun, 7 Feb 2021 13:42:04 +0800 Subject: [PATCH 4/6] ignore non-children path of remove op --- packages/bridge/src/convert/remove.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/bridge/src/convert/remove.ts b/packages/bridge/src/convert/remove.ts index 609d627..c92518b 100644 --- a/packages/bridge/src/convert/remove.ts +++ b/packages/bridge/src/convert/remove.ts @@ -90,7 +90,9 @@ const opRemove = ( if (type === 'map' && path) { // remove a key from map, mapping to slate set a key's value to undefined. - ops.push(setDataOp(op, doc)(map, tmpDoc)) + if (path[0] === 'children') { + ops.push(setDataOp(op, doc)(map, tmpDoc)) + } return [map, ops] } @@ -109,8 +111,6 @@ const opRemove = ( const key = path[path.length - 1] - if (key === 'cursors' || op.key === 'cursors') return [map, ops] - const fn = key === 'text' ? removeTextOp : removeNodeOp return [map, [...ops, fn(op)(map, tmpDoc)]] From e7648b083df0fa2044e8357c129336682e98868f Mon Sep 17 00:00:00 2001 From: Ulion Date: Sun, 7 Feb 2021 14:04:29 +0800 Subject: [PATCH 5/6] process remove property case properly. --- packages/bridge/src/convert/set.ts | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/packages/bridge/src/convert/set.ts b/packages/bridge/src/convert/set.ts index ac666e2..82a4865 100644 --- a/packages/bridge/src/convert/set.ts +++ b/packages/bridge/src/convert/set.ts @@ -9,15 +9,24 @@ export const setDataOp = ( ) => (map: any, tmpDoc: Element) => { const slatePath = toSlatePath(path) const node = Node.get(tmpDoc, slatePath) - node[key] = toJS(map?.[value] || value) + const oldValue = node[key] + const newValue = toJS(map?.[value] || value) + map[obj] = node // node from tmpDoc is the newest value at the moment, keep map sync + + if (newValue == null) { + // slate does this check. + delete node[key] + } else { + node[key] = newValue + } return { type: 'set_node', path: slatePath, properties: { - [key]: toJS(Automerge.getObjectById(doc, obj)?.[key]) + [key]: toJS(oldValue) }, newProperties: { - [key]: toJS(node[key]) + [key]: toJS(newValue) } } } @@ -26,7 +35,7 @@ const opSet = (op: Automerge.Diff, [map, ops]: any, doc: any, tmpDoc: any) => { const { link, value, path, obj, key } = op try { - if (path && path.length && path[0] !== 'cursors') { + if (path && path.length && path[0] === 'children') { ops.push(setDataOp(op, doc)(map, tmpDoc)) } else if (map[obj]) { map[obj][key as any] = link ? map[value] : value From 16cf59536f5382a0e03c42349b15aaf8b8b84159 Mon Sep 17 00:00:00 2001 From: Ulion Date: Tue, 9 Feb 2021 10:42:12 +0800 Subject: [PATCH 6/6] Fix set wrong map value for list remove item op. --- packages/bridge/src/convert/remove.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/bridge/src/convert/remove.ts b/packages/bridge/src/convert/remove.ts index c92518b..566a6ae 100644 --- a/packages/bridge/src/convert/remove.ts +++ b/packages/bridge/src/convert/remove.ts @@ -55,18 +55,16 @@ const removeNodeOp = (op: Automerge.Diff) => (map: any, doc: Element) => { throw new TypeError('Target is not found!') } - if (!map.hasOwnProperty(obj)) { - map[obj] = target - } - if (!Number.isInteger(index)) { throw new TypeError('Index is not a number') } if (parent?.children?.[index as number]) { parent.children.splice(index, 1) + map[obj] = parent?.children } else if (parent?.[index as number]) { parent.splice(index, 1) + map[obj] = parent } return {