Compose does not preserve invertable operations

[Jocs]

Bad case:

const json1 = require('ot-json1')

const doc = [{
    text: '-'
}]

const op1 = json1.editOp([0, 'text'], 'text-unicode', [{ d: '-' }])
const op2 = json1.removeOp([0], { text: '' })

const op = json1.type.compose(op1, op2)

const result = json1.type.apply(doc, op)

console.log(result) // []

const invertedOp = json1.type.invert(op)

const result2 = json1.type.apply(result, invertedOp)

console.log(result2) // [ { text: '' } ]

Expect result2 equal to doc ? But they are not equal.

[josephg]

Yep good find. This is a known issue, but we should document it and fix it. From here:

• Compose doesn't preserve invert information in {r:_} components. This is fixable - it should more or less mirror the logic its already using for inserts. But it hasn't been fixed.

First step, we should update the readme to talk about this stuff. Having a footgun like this is bad form.

Until compose has been fixed, use

const invertedOp = json1.type.invertWithDoc(op, doc)

.. and that will fix the issue. Alternately call makeInvertible with the document context after calling compose:

const op = json1.type.makeInvertible(json1.type.compose(op1, op2), doc)

[Jocs]

I have never wanted to use the invertWithDoc method, because I have to store a deep copy of the entire document. Because the document will be relatively large, it will take up a lot of memory space, but there is no better way. I will use invertWithDoc first. Thank you very much for your reply, and look forward to fixing this issue. 😃

[josephg]

You need a copy of the document when you call makeInvertible. But you shouldn't need to store the document itself after you've made the operation invertible.

... Are you calling compose on old operations? How are you using it?

[Jocs]

Are you calling compose on old operations? How are you using it?

I misunderstood makeInvertible . I don’t need to store the entire document. Below is the code for me to record the history stack.

 record (op, doc) {
    if (op.length === 0) {
      return
    }
    this.stack.redo = []
    let undoOperation = json1.type.invert(op)
    const timestamp = Date.now()
    if (
      this.lastRecorded + this.options.delay > timestamp &&
      this.stack.undo.length > 0
    ) {
      const { operation: lastOperation } = this.stack.undo.pop()

      undoOperation = json1.type.makeInvertible(json1.type.compose(undoOperation, lastOperation), doc)
    } else {
      this.lastRecorded = timestamp
    }

    if (!undoOperation || undoOperation.length === 0) {
      return
    }

    this.stack.undo.push({ operation: undoOperation })
  }

and it works well now after by using makeInvertible . doc at the second parameter is the document after apply op at the first parameter in record method. am i right?

[josephg]

Yeah that looks ok.

• The first test should be if op == null. (I should have made the empty list be the null operation - but its null; and its too late to change it now).
• Is your loop conditional backwards? Shouldn't it be < timestamp rather than > timestamp?

doc at the second parameter is the document after apply op at the first parameter in record method. am i right?

Huh? The document you pass needs to be the state before you apply the operation. Otherwise deleted content will have already been removed before we can copy it into the op.

[Jocs]

Is your loop conditional backwards? Shouldn't it be < timestamp rather than > timestamp?

it equal to

this.options.delay > timestamp - this.lastRecorded

I think it is greater than timestamp is right, because if the two operations are within the delay, the two operations will be merged into one operation through compose.