automerge-classic icon indicating copy to clipboard operation
automerge-classic copied to clipboard

Add OpId to remove actions in Patch

Open begor opened this issue 2 years ago • 4 comments

Currently we put OpId (operation id) for SingleInsertEdit and UpdateEdit, but omit it for RemoveItem. The same thing is true about map deletions -- we simply do {attr: {}} and there's no way to know which operation is responsible for this change.

This is sometimes annoying, because we'd like to have some ordering of actions inside one patch and it's tricky (or impossible) without OpId.

begor avatar Dec 15 '21 12:12 begor

Automerge CI / browsertest (pull_request) Failing after 11m — browsertest

It also fails on main branch, so it has nothing to do with this PR. @ept, FYI.

begor avatar Dec 15 '21 15:12 begor

Converted to draft -- this'll need more work, we also want removes of map keys to include op id.

E.g. currently we have {props: {}}, but we want {props: {deletionOpId: null | RemoveItem}. I'll try to implement it.

begor avatar Dec 16 '21 06:12 begor

Hi @begor, thanks for this. The browsertest failure can be ignored (it's been flaky since I moved from Travis CI to GitHub Actions and I haven't yet figured out how to make it reliable again).

I am happy to consider this, but could you please tell me more about why you need this information in the patches? I don't currently see a need for it, and I am worried you might be doing something that goes against the grain of the intended use of Automerge. For map keys, {props: {deletionOpId: null}} would not work, since that would be interpreted as setting the key to null, which is not the same as deleting the key.

Also, a heads up – we are planning to change the patch format/API with a view towards improving performance. We can also include deletion opIds in the new API if we have a need for it.

ept avatar Dec 16 '21 14:12 ept

@ept I've updated the PR.

Why do we need this: we use Patch to reconstruct state of the model when we load history. For this we use our own automerge frontend (which is more like a "patch interpreter") that doesn't keep the model in memory, but instead maps changes in JSON tree (as described in Patch) to some actions (API calls, updates in other modules, etc.).

For this we'd like to sort updates inside a Patch by opId to apply them in order of increasing lamport clocks. It also helpful to know which operation is responsible for which action in patch for various other reasons. Currently there's no way to do this, because opIds are missing from deletes. So we add them here.

The implementation is a bit peculiar when it comes to map deletes -- here we use {deletionOpId: '___DELETED___'} and update code to handle this situation. If there's a better way to do this -- let me know!

begor avatar Dec 17 '21 09:12 begor