Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add OpId to remove actions in Patch #453

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

begor
Copy link
Contributor

@begor begor commented Dec 15, 2021

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
Copy link
Contributor Author

begor commented Dec 15, 2021

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 begor marked this pull request as draft December 15, 2021 16:08
@begor
Copy link
Contributor Author

begor commented Dec 16, 2021

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.

@ept
Copy link
Member

ept commented Dec 16, 2021

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.

@begor begor marked this pull request as ready for review December 17, 2021 09:22
@begor
Copy link
Contributor Author

begor commented Dec 17, 2021

@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 begor changed the title Add OpId to RemoveItem in Patch Add OpId to remove actions in Patch Dec 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants