The end of undefined docSync() / await doc() #402

pvh · 2024-12-30T17:23:27Z

After many long conversations with automerge-repo users, I have finally seen the error of my ways. I see now that repo.find() should not be synchronous and should instead return a promise to a handle that resolves when the handle is actually loaded.

This change should integrate better with suspense and radically reduce the amount of annoying awaits, undefined checks, and other irritations when consuming documents in automerge-repo.

Here's an initial draft that passes most tests. I as I continue to work through the consequences and simplify the resulting library, I'll update it here.

Comments & questions welcome.

(Note that this PR probably currently breaks a few features such as unload() but we'll work through that stuff before landing.)

packages/automerge-repo/test/Repo.test.ts

packages/automerge-repo/src/synchronizer/DocSynchronizer.ts

kraenhansen · 2024-12-30T20:24:25Z

I see you're not awaiting the expect calls wrapping callbacks returning promises, some places. It seems the pattern is when you chain .rejects.toThrow. If there's a reason for this, feel free to disregard my comments, but it looks like an oversight to me. If the promise resolves "slowly" I suspect the tests won't fail.

msakrejda · 2024-12-30T23:51:55Z

Nice, excited to see this revisited. Can you talk about how this affects this diagram?

pvh · 2024-12-31T06:11:47Z

Nice, excited to see this revisited. Can you talk about how this affects this diagram?

Ideally, all of those states except READY and DELETED will go away from a public consumer point of view. If you call find() and it doesn't reach a useful state, you'll get an exception.

That might happen because you're offline and trying to open a document you have heard of but didn't fetch yet for whatever reason, or because the peers you're connected to don't have the data.

That said, we won't give you the handle back from the promise unless and until you can actually use it! Long-term, I think some kind of signal-based model here would be preferable: you might want to be able to see incremental loading details to show some kind of progress bar. That said, we don't have that yet, so it feels premature to worry too much about it.

msakrejda

Nice, I'm excited to see this direction. I left a bunch of comments and questions, but overall it looks good. You had asked me privately about testing, but I don't see anything missing: test coverage looks great to me.

My main concern: in the React hooks, is it possible to load local documents without suspending? That would be really nice. Is Suspense always triggered because find returns a promise? If that's the case, would a findSync work, that returns undefined if the handle's document is not loaded? I think that could be used to build hooks that don't suspend unless necessary.

msakrejda · 2025-01-07T06:39:56Z

packages/automerge-repo-react-hooks/src/useDocHandle.ts

+export const wrapperCache = new Map<
+  AnyDocumentId,
+  ReturnType<typeof wrapPromise>
+>()


Do we really want this global, and not, e.g., living in something that also wraps a RepoContext.Provider?

oooooh. that might be better. it would also be a bit more complicated... my immediate instinct is to defer this until someone asks for it. the failure mode would be that if you had the same document in two different repos in your react app they'd share a cache of their state.

that would be hard to debug and understand for sure. should this be part of the repo context? i seem to remember in one version of this patch trying to rely on the consistency of the promises from repo.find() ... but then you can't resolve it in two places so i guess actually we need the cache here. hmmm.

i think i'd prefer to leave this as-is because the failure mode seems pretty remote and a robust solution seems complicated but i could be persuaded this is the wrong idea.

my weak compromise was to add the comment below. if you find this too odious, i'm open to improving on it.

Yeah, okay. Though for what it's worth, I use context for stuff like this all the time, though, and I've never run into problems. The odds of anyone running into a problem here are pretty remote, but if they do, it's going to be a fun debugging time for sure. Plus, I think introducing an AutomergeContext with this breaking change lets us put other per-repo state (that does not belong in a repo) in this context in the future without breaking existing APIs at that point. But I don't think it's a huge deal; it'd be a pretty easy migration if necessary.

msakrejda · 2025-01-07T06:42:48Z

packages/automerge-repo-react-hooks/src/useDocHandle.ts

+  if (suspense) {
+    return wrapper.read() as DocHandle<T>
+  } else {
+    return handle || undefined


What does || undefined give you here? Isn't the handle already a DocHandle or undefined?

msakrejda · 2025-01-07T06:43:58Z

packages/automerge-repo-react-hooks/src/useDocHandle.ts

+
+  useEffect(() => {
+    if (suspense === false) {
+      void wrapper.promise


Why void here?

Ah, this is to deal with floating promises? But you have a then and catch here, which should take care of that, no?

eslint was likely complaining. i'll see if i can remove it

msakrejda · 2025-01-07T06:45:42Z

packages/automerge-repo-react-hooks/src/useDocHandle.ts

+  }
+
+  useEffect(() => {
+    if (suspense === false) {


I think this would be simpler as

if (suspense) { return }

and then handle the non-suspense case unconditionally.

i can fix that, yeah

msakrejda · 2025-01-07T06:47:58Z

packages/automerge-repo-react-hooks/src/useDocHandles.ts

+
+  // Check if we need any new wrappers
+  for (const id of ids) {
+    let wrapper = wrapperCache.get(id)!


Why !? Aren't you testing if (!wrapper) right below?

packages/automerge-repo/src/Repo.ts

msakrejda · 2025-01-07T08:02:36Z

packages/automerge-repo-react-hooks/test/useDocument.test.tsx

-    const { handleA, wrapper } = setup()
-    const onDoc = vi.fn()
+    // First we should see the loading state
+    expect(screen.getByTestId("loading")).toBeInTheDocument()


We expect to see a loading state even if we're dealing with a locally-created document?

msakrejda · 2025-01-07T08:06:21Z

packages/automerge-repo-react-hooks/test/useDocument.test.tsx

-  it("avoids showing stale data", async () => {
-    const { handleA, handleSlow, wrapper } = setup()
+  // Test document changes during loading
+  it("should handle document changes while loading", async () => {


Nice, good test idea.

msakrejda · 2025-01-07T08:10:38Z

packages/automerge-repo-react-hooks/test/useDocuments.test.tsx

+    })
+
+    await act(async () => {
+      await Promise.resolve()


Is this waiting for useEffect? It might be nice to have a comment.

Ah, this is waiting for the doc to load, right?

msakrejda · 2025-01-07T08:13:07Z

packages/automerge-repo-react-hooks/test/useDocuments.test.tsx

-      )
-      await waitFor(() => unmount())
-    }
+  it("should handle multiple documents and parallel changes", async () => {


msakrejda · 2025-01-08T22:27:45Z

Oh, also, does this approach to hooks work on React 19? I think it should, but given the state of the Suspense APIs, it might be good to check.

…resumably caused by my recent patch

…tion. this code needs serious rethinking

… late

pvh · 2025-01-31T19:49:32Z

examples/react-todo/src/main.tsx

@@ -38,7 +39,11 @@ window.repo = repo
 ReactDOM.createRoot(document.getElementById("root") as HTMLElement).render(
  <RepoContext.Provider value={repo}>
    <React.StrictMode>
-      <App url={docUrl} />
+      <ErrorBoundary fallback={<div>Something went wrong</div>}>


this officialish error boundary is chatty in the logs. i should silence it and/or replace it with one that is better for tests.

There's not much to it—for my project, I ended up just borrowing from it liberally and writing my own. But I think that can wait.

pvh · 2025-01-31T20:53:36Z

packages/automerge-repo-svelte-store/package.json

  "repository": "https://github.com/automerge/automerge-repo/tree/master/packages/automerge-repo-svelte-store",
  "license": "MIT",
  "type": "module",
  "main": "dist/index.js",
  "scripts": {
-    "build": "tsc",
+    "build": "",


right, i forgot i need some svelte help

i think it's fine to ship an alpha but i need to either get help or learn some svelte i suppose

:disappear:

pvh · 2025-01-31T20:54:24Z

packages/automerge-repo-svelte-store/src/index.ts

@@ -87,7 +87,7 @@ export function setContextRepo(repo: Repo) {
 export function document<T>(documentId: AutomergeUrl, repo?: Repo) {
  repo = repo ?? getContextRepo()
  const handle = repo.find<T>(documentId)


this line here is the problem for svelte!

… dependency

…g logs, fix doc references to useHandle

msakrejda

Nice, I left some questions and comments, but broadly this looks great.

msakrejda · 2025-01-31T22:07:59Z

eslint.config.mjs

@@ -1,4 +1,5 @@
 import typescriptEslint from "@typescript-eslint/eslint-plugin"
+import pluginReact from "eslint-plugin-react"


Don't you need something similar for the hooks plugin?

msakrejda · 2025-01-31T22:10:54Z

examples/react-todo/src/main.tsx

@@ -38,7 +39,11 @@ window.repo = repo
 ReactDOM.createRoot(document.getElementById("root") as HTMLElement).render(
  <RepoContext.Provider value={repo}>
    <React.StrictMode>
-      <App url={docUrl} />
+      <ErrorBoundary fallback={<div>Something went wrong</div>}>


There's not much to it—for my project, I ended up just borrowing from it liberally and writing my own. But I think that can wait.

msakrejda · 2025-01-31T22:17:46Z

packages/automerge-repo-react-hooks/src/useDocHandle.ts

+export const wrapperCache = new Map<
+  AnyDocumentId,
+  ReturnType<typeof wrapPromise>
+>()


Yeah, okay. Though for what it's worth, I use context for stuff like this all the time, though, and I've never run into problems. The odds of anyone running into a problem here are pretty remote, but if they do, it's going to be a fun debugging time for sure. Plus, I think introducing an AutomergeContext with this breaking change lets us put other per-repo state (that does not belong in a repo) in this context in the future without breaking existing APIs at that point. But I don't think it's a huge deal; it'd be a pretty easy migration if necessary.

msakrejda · 2025-01-31T22:25:53Z

packages/automerge-repo-react-hooks/src/useDocHandles.ts

+        const promise = repo.find<T>(id)
+        wrapper = wrapPromise(promise)
+        wrapperCache.set(id, wrapper)
+      } catch (e) {


Where is that thrown? I'm probably missing something, but as far as I can tell, neither find nor wrapPromise throw synchronously.

msakrejda · 2025-01-31T22:38:00Z

packages/automerge-repo-react-hooks/src/useDocHandles.ts

+    } else {
+      setHandleMap(nextHandleMap)
+    }
+  }, [suspense, ids])


You're still omitting pendingPromises and nextHandleMap from the dependency array. I think your hook lint isn't running here. I remember when I was in a spot like this that refactoring to avoid that is not trivial, and the logic seems sound, so maybe throw an eslint-ignore-next-line on it once you get linting working for now.

msakrejda · 2025-01-31T22:55:48Z

packages/automerge-repo/src/helpers/tests/network-adapter-tests.ts

-        await eventPromise(bobRepo, "document")
-        const bobHandle = bobRepo.find<TestDoc>(aliceHandle.url)
+        // TODO: ... let connections complete. this shouldn't be necessary.
+        await pause(50)


Are you sure this is necessary? Tests pass reliably for me without this (granted, CI may be a different story).

yeah, i needed it. i don't know why... investigating it is beyond my motivation right now.

msakrejda · 2025-01-31T22:58:45Z

packages/automerge-repo/src/synchronizer/DocSynchronizer.ts

@@ -97,8 +102,7 @@ export class DocSynchronizer extends Synchronizer {
  /// PRIVATE

  async #syncWithPeers() {
-    this.#log(`syncWithPeers`)
-    const doc = await this.#handle.doc()
+    const doc = await this.#handle.legacyAsyncDoc() // XXX THIS ONE IS WEIRD


If you have more context to leave here for future you or other future maintainers, that might prove worthwhile.

msakrejda · 2025-01-31T22:59:52Z

packages/automerge-repo/src/synchronizer/DocSynchronizer.ts

-
-    const docPromise = this.#handle
-      .doc([READY, REQUESTING, UNAVAILABLE])
+    const docPromise = this.#handle // TODO THIS IS ALSO WEIRD


Same as above.

msakrejda · 2025-01-31T23:01:56Z

packages/automerge-repo/test/DocHandle.test.ts

@@ -260,7 +259,7 @@ describe("DocHandle", () => {
    const handle = new DocHandle<TestDoc>(TEST_ID)
    assert.equal(handle.isReady(), false)

-    handle.doc()
+    handle.legacyAsyncDoc()


And here...

msakrejda · 2025-01-31T23:03:39Z

packages/automerge-repo/test/remoteHeads.test.ts

      // we should only be notified of the head changes of doc A
-      assert.strictEqual(remoteHeadsChangedMessages.length, 2)
+      assert.strictEqual(remoteHeadsChangedMessages.length, 1)


Why did this change?

i don't know why. i decided i don't need to

packages/automerge-repo-react-hooks/test/useDocument.test.tsx