Closed orionz closed 3 years ago
Hi @orionz, this is great! Some responses:
There was no straight forward way to implement op coalesce - so I've just skipped the tests that cover this feature. Is this feature critical?
No, I think we could remove this feature. If it does turn out to be needed, we can bring it back again later.
Currently the patch is generated twice. Once by the frontend, then by the backend, then the frontend patch is thrown away and the backend patch is applied. Now we can just make the frontend patch canonical and have the back end not bother generating diffs if its local. This is optional but should be another nice optimization
If frontend and backend are on different threads, I think we need to keep the current construction: if frontend and backend generate patches concurrently, we have to throw away the frontend patch and apply the backend patch (because we assume that the frontend state is determined only by the sequence of backend-generated patches). If frontend and backend are on the same thread, we can do the optimisation you suggest.
I didn't need to change anything to give the frontend access to elemids since any list insert will only ever have one opId in the conflicts section of the patch and that opId is always equal to the elemId
I just noticed that this is not entirely correct. When a document is loaded using Automerge.load()
, we construct one big patch that instantiates the current state of the document. In that patch, every list element has the opIds of the most recent set
operations on that list element, which may be newer than the insertion operation that created the list element. Therefore, the frontend may end up with the wrong elemIds for some list elements.
I think we will have to explicitly add the list element IDs to the insert
actions in the edits
section of the patch. Later we can maybe compress this information by folding several consecutive insertions into one edit, but for now it'll be easiest to just keep one insertion per list element (or per character in the case of text).
I accidentally merged this PR without the commits I added to fix a bunch of bugs and tidy things up. There doesn't seem to be any way of un-merging it in GitHub, so I opened #293 to include the fixes.
Ok - this patch is ready for feedback but not ready to merge. This has the frontend generate a complete change instead of a request. It does this without needing to do any expensive opid lookups - all that is kept on the back end. Lists and Text objects do now need to track conflicts and elemids which requires some additional splice ops. These could be reduced with some further optimization but are going to be far far cheaper than all the management we're doing on the backend.
What changed
Patch now passes back a
maxOp
field so the frontend can generate opId'sI didn't need to change anything to give the frontend access to elemids since any list insert will only ever have one opId in the conflicts section of the patch and that opId is always equal to the elemId
The uuid's generated for "child" fields, keys in tables, and the actorids used in temp patches have changed to the correct opID - tests needed to be updated to reflect this
There was no straight forward way to implement op coalesce - so I've just skipped the tests that cover this feature. This cannot now be done in the backend b/c opIds have been assigned and seems odd to do in the frontend as its kind of slow. Is this feature critical?
Currently I'm not using the change generated in the frontend - im generating the request and the change - passing both to the backend. The backend converts the request into a change and then does a deep diff between the two to confirm they are the same and then throws the frontend-change away. Obviously the next step is to stop generating the request and just directly use the frontend-change.
What needs to be done
Pulling out the request code from the front end and using the change directly. Rename
__ops
toops
... etcCover the case where multiple frontend changes are made without hearing back from the backend - deps cannot be calculated by the frontend here. The plan is to just pass an empty deps to the backend and the
applyLocalChange
with add the correct deps before encodingPulling out the version code - since its not needed anymore
Decide what to do about coalesce
Currently the patch is generated twice. Once by the frontend, then by the backend, then the frontend patch is thrown away and the backend patch is applied. Now we can just make the frontend patch canonical and have the back end not bother generating diffs if its local. This is optional but should be another nice optimization
Update the typescript definitions to match the new API
There is one error being thrown that I have no idea where it came from since this code shouldn't effect it - but there is a save/load test that's blowing up on decode that i disabled - must get to the bottom of this before merging