automerge / automerge-swift

Swift language bindings presenting Automerge
https://automerge.org/automerge-swift/documentation/automerge/
MIT License
217 stars 11 forks source link

Added commitWith() method to specify message/timestamp and exposing changes via changeByHash() #129

Closed bgomberg closed 3 months ago

bgomberg commented 3 months ago

We currently (with the older version of the swift bindings) use the timestamp field of the changes in order to determine when a document was last modified for display to the user in our application. I was directed to #104 where this feature has also been discussed. While changes to the underlying AutoCommit type in the core automerge library would likely be better long-term, this PR aims to provide a more targeted solution for this swift library. In the normal usage of this library with an AutomergeEncoder, the underlying transaction is made at the point that save() is called. Therefore, we can make this transaction more explicit and pass through commit options (message / timestamp).

I also needed a way to retrieve these commit options later, so added a new changeByHash(hash:) method which grabs a full Change object from the underlying automerge document for a given hash. This way, we can easily get the timestamp for the latest changes by calling this method on each head of the document.

I added a new test which showcases both of these new APIs.

As an aside, when I ran the build-xcframework.sh script locally, I ended up with a ton of other local changes to the generated code (mostly style changes). I removed those from the PR but would be great to know if there's a way to make this less noisy moving forwards.

bgomberg commented 3 months ago

Thanks for the feedback @heckj @alexjg.

I separated out commitWith() as that does seem cleaner than overloading save() like I had before. One thing that's a bit funky, but feels like the behavior I would want given the underlying Rust implementation, is that calling save() results in a nil message and 0 timestamp, whereas calling commitWith() results in a nil message and Date().timeIntervalSince1970 timestamp. I do like having the Swift API populate the date for me, and would personally vote for having the Rust code do the same by default (and thus move the save() behavior to match commitWith(), but could understand not wanting to add that dependency. Regardless, I updated the tests to explicitly exercise and demonstrate the differences.

alexjg commented 3 months ago

Re. the nil message and 0 timestamp. If you call commitWith followed by save then the save shouldn't actually create a new commit. The reason is that save (and all the other methods which trigger a commit) don't do anything unless there are actually outstanding changes, so if you've already saved everything then nothing should happen.