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

[1.0.1-preview.2 regression] Counter type gets converted to number unnecessarily #373

Closed rongoro closed 3 years ago

rongoro commented 3 years ago

In a data structure like: Object->Array->Counter inside an automerge document, the Counter object is getting converted to a number.

The following code demonstrates the problem:

const automerge = require("automerge");

const works = automerge.from(["a", 1, new automerge.Counter(5)]);
console.log(`${works[2].constructor.name} should be Counter`);

const works2 = automerge.from({ x: "a", y: 1, z: new automerge.Counter(5) });
console.log(`${works2.z.constructor.name} should be Counter`);

const fails = automerge.from({ foo: ["a", 1, new automerge.Counter(5)] });
console.log(`${fails.foo[2].constructor.name} should be Counter`);
ept commented 3 years ago

Thanks @rongoro for the bug report!

To the other Automerge maintainers: this is a bug in the new patch format (#328). It's happening because appendEdit doesn't check the datatype flag of an insert operation before coalescing it into a multi-insert, and so the insertion of the counter object is turned into a multi-insert including the counter value. We should change appendEdit to keep an insertion as a separate operation if its datatype flag indicates that it is not a primitive.

A simple fix would be to stop all multi-insert coalescing whenever a datatype property is present in the operation. However, that will interact poorly with #356, which involves adding a datatype property to all numbers in a patch. We still want to use multi-insert coalescing for lists of numbers when possible. I will wait for #356 / #371 to land first, and then we can figure this one out.

rongoro commented 3 years ago

Looks like #371 still hasn't landed, so I don't think any work has happened on this, but I'll just confirm that this is still a problem in 1.0.1-preview.3.

ept commented 3 years ago

I have now merged #371 and the bug should be fixed as part of that PR. It will be part of the preview.4 release. Please let me know if you still have any issues with this.