automerge / automerge-classic

A JSON-like data structure (a CRDT) that can be modified concurrently by different users, and merged again automatically.
http://automerge.org/
MIT License
14.75k stars 466 forks source link

Handle object paths that change due to other operations in the same change #354

Open skokenes opened 3 years ago

skokenes commented 3 years ago

Using automerge:1.0.1-preview.0, I am trying to move an object from one location to another. I am using a script that worked in AM 0.14 for moving an object, and seems to work with normal JS objects as well. However, in AM 1.0, it throws RangeError No children at key 2 of path... when I try to use the script.

Codesandbox of the issue here: https://codesandbox.io/s/am-10-move-node-bug-rduhq?file=/src/index.js

The code:

import { from, Text, change } from "automerge";

let doc = from({
  type: "document",
  children: [
    {
      type: "paragraph",
      children: [
        {
          text: new Text("hi")
        }
      ]
    },
    { type: "list-item", children: [{ text: new Text("* ") }] },
    { type: "bulleted-list", children: [] }
  ]
});

doc = change(doc, (doc) => {
  // Trying to move doc.children[1] to doc.children[2].children[0] (which becomes doc.children[1] in the process)
  const sourceParent = doc;
  const targetParent = doc.children[2];
  // Remove doc.children[1] from its source parent
  const nodeToMove = cloneNode(sourceParent.children.splice(1, 1));
  // Add it into its target parent -> This throws an error!
  targetParent.children.splice(0, 0, nodeToMove);
});

document.getElementById("app").innerHTML = JSON.stringify(doc, null, 2);

function cloneNode(n) {
  return JSON.parse(JSON.stringify(n));
}
ept commented 3 years ago

Hi @skokenes, thanks for this good bug report. I can reproduce the issue, and unfortunately it is a bit tricky to fix. What's happening here is that when you do const targetParent = doc.children[2], the targetParent object remembers the path from the document root to the object in question (namely, index 2 of the children array). When you later call targetParent.children.splice(), that path no longer refers to an object that exists, since it has moved to index 1.

The reason this code worked in 0.x is that we used to only identify objects by ID, and not by their path. I didn't realise when making this change that paths are not stable because they can be affected by other operations in the same change.

To fix this bug, I think we will have to change the frontend logic to only refer to objects by ID until the time when a mutation occurs, and only convert the ID-based addressing into path-based addressing at that point. This should be doable but will require a bit of work. I think it's important though, so I'll tag this issue as needing to be fixed before a 1.0 stable release.