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

failing test: loading empty doc from network #369

Closed
wants to merge 1 commit into from

Conversation

geoffreylitt
Copy link
Collaborator

In Jacquard, we have a use case where we sometimes try to load an empty doc from network. It's a metadata doc which sometimes has some stuff in it, but other times it starts out just being empty.

It turns out that if you do this, automerge-repo will never resolve the .doc() promise, so our app hangs. (This PR has a failing test for that)

The underlying cause is this code in DocHandle, which uses heads to decide whether to do a state transition:

 #checkForChanges(before: A.Doc<T>, after: A.Doc<T>) {
    const beforeHeads = A.getHeads(before)
    const afterHeads = A.getHeads(after)
    const docChanged = !headsAreSame(afterHeads, beforeHeads)
    if (docChanged) {
      this.emit("heads-changed", { handle: this, doc: after })

The "empty" handle pre-load has heads [], and the doc loaded from network also has heads [], so the state transition doesn't happen.

@joshuahhh and I got this far and we weren't sure what made sense to do next, thoughts @alexjg ?

For now we were able to workaround in our app by just making a change in the empty docs to avoid this case.

@geoffreylitt geoffreylitt requested a review from alexjg August 5, 2024 20:00
@alexjg
Copy link
Contributor

alexjg commented Aug 6, 2024

Interesting, in the latest alpha of automerge-repo I think we may fix this. We made the decision to always create a document with an empty change in it to distinguish a new document from one we are requesting. This is effectively what you are doing as well.

@geoffreylitt
Copy link
Collaborator Author

Great, sounds like that'll fix it! I'll just close this issue and then we can confirm fixed once we upgrade to that.

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