Use automerge traits as opposed to concrete document

[gterzian]

Currently the repo is built around an AutoCommit, we could instead abstract it using the existing traits in automerge, so that the user can choose what kind of document to use(automerge or autocommit).

[teohhanhui]

I'd like to work on this issue, if you don't mind.

We cannot use AutoCommit for our project, as explained here: automerge/automerge#443 (comment)

[teohhanhui]

It seems to me we cannot abstract over Automerge and AutoCommit, considering AutoCommit doesn't directly implement SyncDoc.

For example, would the caller be expected to call .sync() on AutoCommit themselves, and then pass in the document to the Repo?

[alexjg]

What's the usecase for the user calling sync() directly? The model for this library so far has been that the Repo would manage all sync related calls for the user. Maybe there are times you don't have a stream handy to pass to Repo::new_remote_repo?

[alexjg]

SyncDoc aside I think moving all the with_doc and with_doc_mut calls to expose ReadDoc and Transactable would be great!

[teohhanhui]

What's the usecase for the user calling sync() directly?

Sorry if I wasn't being clear. I'm just trying to see how that could work for AutoCommit, if we change the shared document to use generic bounds.

It seems like we'd need an architectural change? But perhaps I'm missing something here...

[alexjg]

Yeah I think I understand you. What I'm saying is that I'm not sure we need to provide access to the methods on SyncDoc to the user. The API I am imagining is basically this:

impl DocHandle {
    ...
    
    /// Run a closure over a immutable reference to the document,
    pub fn with_doc<F, T, R>(&self, f: F) -> T
    where
        R: ReadDoc,
        F: FnOnce(R) -> T {
        ...
    }
        
    /// Run a closure over a mutable reference to the document,
    pub fn with_doc_mut<F, T,D>(&mut self, f: F) -> T
    where
        D: Transactable,
        F: FnOnce(&mut D>) -> T,
    {
       ....
    }

I think there should be no need to call SyncDoc methods because sync is all being handled by the repo. Is this at all like what you were imagining?

[teohhanhui]

I was thinking of the bounds for the (hypothetical) generic parameter Doc on DocHandle<Doc> / SharedDocument<Doc>. It cannot be SyncDoc because AutoCommit doesn't implement that?

[alexjg]

Ah interesting, I had been thinking of this work as primarily about with_doc and with_doc_mut and hadn't considered making the handle itself generic over doc type. The latter seems like it would add a lot of complexity as we would have to track that type parameter through the whole repo (e.g having separate collections of doc types for Autonerge and AutoCommit everywhere). What does it allow you to do that you couldn't do with the above idea?

[teohhanhui]

As I've explained before in automerge/automerge#443 (comment) we cannot use AutoCommit as it doesn't allow explicit demarcation of a transaction's boundaries, and as a result it'd inadvertently break apart our transactions causing part of the changes to become interleaved with automerge sync changes. (I still believe the design of AutoCommit should be fixed to make it actually useful for transactions, but of course we should discuss that in the automerge issue tracker...)

Based on the OP description of this issue I'd assumed it's about letting the user choose not to use AutoCommit at all.

[alexjg]

How about something like this:

    pub fn transact<F, O, E>(&mut self, f: F) -> Result<O, E>
    where
        E: std::error::Error,
        F: FnOnce(&mut dyn automerge::transaction::Transactable) -> Result<O, E>,
    {
        todo!()
    }

Which runs F in a transaction, committing if it returns Ok and rolling back if it's an Err?

Longer term for your usecase I think branches will support this. You would basically branch the document at the point you want to start making changes, add your changes to it, and then when you want to commit you pull it back in to some kind of "main" branch. (branches are still in the design phase but they're the next thing we're planning to work on in automerge proper).

[teohhanhui]

Do you mean adding that method on AutoCommit? Sure, I think that should work.

[alexjg]

Sorry no I mean adding it on DocHandle.

I've realized I'm not being very clear in this thread about what I'm aiming for. My understanding is that your goal is to be able to make certain sets of changes atomic. One way to achieve this is to make DocHandle generic over the kind of automerge document it manages, such that it becomes DocHandle<T>, this would in turn imply making Repo generic as well. What you would get is the ability to make a DocHandle<Automerge> so that you can use Automerge::transact and friends. Let me know if this is not what you were thinking.

I am wondering if we can find a different approach. I would like to use Automerge in the DocHandle and not AutoCommit and then expose methods to read and modify the document such as the transact method I posted above which operate in terms of automerge::ReadDoc and automerge::Transactable. The main advantage of this is that it allows us to avoid schlepping a generic parameter around everywhere which adds a lot of complexity and I think it still allows users to do everything they could do with AutoCommit. What do you think?

[teohhanhui]

Hmm... I don't get how we could avoid exposing the generic parameter though. DocHandle will hold Arc<RwLock<Doc>>, right?

[gterzian]

I think using traits is a good idea, in the meantime, we'll switch to using Automerge as done in #20

[alexjg]

My point is I don't think there's much point in a DocHandle<AutoCommit>. The usefulness of AutoCommit is in not having to call transact etc. all the time. But for a DocHandle you have to call with_doc or with_doc_mut anyway so it seems to me like we should just stick to using Automerge inside the dochandle. However, I think that with_doc and with_doc_mut should expose their arguments as ReadDoc and Transactable where possible because that allows people to write code which is generic over Automerge and AutoCommit.