okay, more decent tests for useDocuments

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

@@ -24,9 +24,13 @@ export function useDocHandles<T>(
   for (const id of ids) {
     let wrapper = wrapperCache.get(id)!
     if (!wrapper) {
-      const promise = repo.find<T>(id)
-      wrapper = wrapPromise(promise)
-      wrapperCache.set(id, wrapper)
+      try {
+        const promise = repo.find<T>(id)
+        wrapper = wrapPromise(promise)
+        wrapperCache.set(id, wrapper)
+      } catch (e) {
+        continue
+      }
     }
 
     // Try to read each wrapper.
@@ -38,6 +42,8 @@ export function useDocHandles<T>(
     } catch (e) {
       if (e instanceof Promise) {
         pendingPromises.push(wrapper as PromiseWrapper<DocHandle<T>>)
+      } else {
+        nextHandleMap.set(id, undefined)
       }
     }
   }
@@ -49,10 +55,17 @@ export function useDocHandles<T>(
 
   useEffect(() => {
     if (pendingPromises.length > 0) {
-      void Promise.all(pendingPromises.map(p => p.promise)).then(handles => {
-        handles.forEach(h => nextHandleMap.set(h.url, h))
-        setHandleMap(nextHandleMap)
-      })
+      void Promise.allSettled(pendingPromises.map(p => p.promise)).then(
+        handles => {
+          handles.forEach(r => {
+            if (r.status === "fulfilled") {
+              const h = r.value as DocHandle<T>
+              nextHandleMap.set(h.url, h)
+            }
+          })
+          setHandleMap(nextHandleMap)
+        }
+      )
     } else {
       setHandleMap(nextHandleMap)
     }

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

@@ -10,11 +10,15 @@ type ChangeDocFn<T> = (
   options?: ChangeOptions<T>
 ) => void
 
+interface UseDocumentsOptions {
+  suspense?: boolean
+}
+
 export function useDocuments<T>(
-  ids: AutomergeUrl[]
+  ids: AutomergeUrl[],
+  { suspense = true }: UseDocumentsOptions = {}
 ): [DocMap<T>, ChangeDocFn<T>] {
-  // Pass suspense: true to useDocHandles since we want to ensure data is ready
-  const handleMap = useDocHandles<T>(ids, { suspense: true })
+  const handleMap = useDocHandles<T>(ids, { suspense })
   const [docMap, setDocMap] = useState<DocMap<T>>(() => new Map())
 
   useEffect(() => {

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

@@ -1,6 +1,6 @@
 import React, { Suspense } from "react"
 import { AutomergeUrl, Repo, PeerId } from "@automerge/automerge-repo"
-import { render, act } from "@testing-library/react"
+import { render, act, waitFor } from "@testing-library/react"
 import { describe, expect, it, vi } from "vitest"
 import { useDocuments } from "../src/useDocuments"
 import { RepoContext } from "../src/useRepo"
@@ -233,4 +233,258 @@ describe("useDocuments", () => {
     const [finalDocs] = onState.mock.lastCall
     expect(finalDocs.get(handleA.url)?.counter).toBe(4)
   })
+
+  describe("useDocuments with suspense: false", () => {
+    const repo = new Repo({
+      peerId: "bob" as PeerId,
+    })
+
+    function setup() {
+      const handleA = repo.create<ExampleDoc>()
+      handleA.change(doc => (doc.foo = "A"))
+
+      const handleB = repo.create<ExampleDoc>()
+      handleB.change(doc => (doc.foo = "B"))
+
+      const handleC = repo.create<ExampleDoc>()
+      handleC.change(doc => (doc.foo = "C"))
+
+      return {
+        repo,
+        handleA,
+        handleB,
+        handleC,
+        handles: [handleA, handleB, handleC],
+        urls: [handleA.url, handleB.url, handleC.url],
+        wrapper: getRepoWrapper(repo),
+      }
+    }
+
+    const NonSuspendingDocumentsComponent = ({
+      urls,
+      onState,
+    }: {
+      urls: AutomergeUrl[]
+      onState: (docs: Map<AutomergeUrl, ExampleDoc>, change: any) => void
+    }) => {
+      const [docs, change] = useDocuments<ExampleDoc>(urls, { suspense: false })
+      onState(docs, change)
+      return null
+    }
+
+    it("should start with empty map and load documents asynchronously", async () => {
+      const { handleA, wrapper } = setup()
+      const onState = vi.fn()
+
+      const Wrapped = () => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent
+            urls={[handleA.url]}
+            onState={onState}
+          />
+        </ErrorBoundary>
+      )
+
+      render(<Wrapped />, { wrapper })
+
+      // Initial state should be empty map
+      expect(onState).toHaveBeenCalled()
+      let [docs] = onState.mock.lastCall
+      expect(docs.size).toBe(0)
+
+      // Wait for document to load
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      // Document should now be loaded
+      docs = onState.mock.lastCall[0]
+      expect(docs.get(handleA.url)?.foo).toBe("A")
+    })
+
+    it("should handle loading multiple documents asynchronously", async () => {
+      const { handleA, handleB, wrapper } = setup()
+      const onState = vi.fn()
+
+      const Wrapped = () => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent
+            urls={[handleA.url, handleB.url]}
+            onState={onState}
+          />
+        </ErrorBoundary>
+      )
+
+      render(<Wrapped />, { wrapper })
+
+      // Initial state should be empty
+      let [docs] = onState.mock.lastCall
+      expect(docs.size).toBe(0)
+
+      // Wait for documents to load
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      // Check loaded state
+      docs = onState.mock.lastCall[0]
+      expect(docs.size).toBe(2)
+      expect(docs.get(handleA.url)?.foo).toBe("A")
+      expect(docs.get(handleB.url)?.foo).toBe("B")
+
+      // Make changes after loading
+      const [, change] = onState.mock.lastCall
+      await act(async () => {
+        change(handleA.url, doc => {
+          doc.counter = 1
+          doc.nested = { value: "A1" }
+        })
+        change(handleB.url, doc => {
+          doc.counter = 2
+          doc.nested = { value: "B1" }
+        })
+      })
+
+      // Verify changes
+      const [finalDocs] = onState.mock.lastCall
+      expect(finalDocs.get(handleA.url)).toEqual({
+        foo: "A",
+        counter: 1,
+        nested: { value: "A1" },
+      })
+      expect(finalDocs.get(handleB.url)).toEqual({
+        foo: "B",
+        counter: 2,
+        nested: { value: "B1" },
+      })
+    })
+
+    it("should handle document removal with pending loads", async () => {
+      const { handleA, handleB, wrapper } = setup()
+      const onState = vi.fn()
+
+      const Wrapped = ({ urls }: { urls: AutomergeUrl[] }) => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent urls={urls} onState={onState} />
+        </ErrorBoundary>
+      )
+
+      const { rerender } = render(
+        <Wrapped urls={[handleA.url, handleB.url]} />,
+        { wrapper }
+      )
+
+      // Initial state should be empty
+      let [docs] = onState.mock.lastCall
+      expect(docs.size).toBe(0)
+
+      // Remove one document before load completes
+      rerender(<Wrapped urls={[handleA.url]} />)
+
+      // Wait for remaining document to load
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      // Should only have loaded the remaining document
+      waitFor(() => {
+        docs = onState.mock.lastCall[0]
+        expect(docs.size).toBe(1)
+        expect(docs.has(handleA.url)).toBe(true)
+        expect(docs.has(handleB.url)).toBe(false)
+      })
+    })
+
+    it("should cleanup listeners when unmounting with pending loads", async () => {
+      const { handleA, wrapper } = setup()
+      const onState = vi.fn()
+
+      const Wrapped = () => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent
+            urls={[handleA.url]}
+            onState={onState}
+          />
+        </ErrorBoundary>
+      )
+
+      const { unmount } = render(<Wrapped />, { wrapper })
+
+      // Initial state empty
+      expect(onState.mock.lastCall[0].size).toBe(0)
+
+      // Unmount before load completes
+      unmount()
+
+      // Wait for what would have been load completion
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      // Should not have received any updates after unmount
+      const callCount = onState.mock.calls.length
+      handleA.change(doc => (doc.foo = "Changed after unmount"))
+      expect(onState.mock.calls.length).toBe(callCount)
+    })
+
+    it("should handle document changes during loading", async () => {
+      const { handleA, wrapper } = setup()
+      const onState = vi.fn()
+
+      const Wrapped = () => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent
+            urls={[handleA.url]}
+            onState={onState}
+          />
+        </ErrorBoundary>
+      )
+
+      render(<Wrapped />, { wrapper })
+
+      // Make a change while document is loading
+      handleA.change(doc => (doc.counter = 1))
+
+      // Wait for load
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      // Should have latest state
+      const [docs] = onState.mock.lastCall
+      expect(docs.get(handleA.url)).toEqual({
+        foo: "A",
+        counter: 1,
+      })
+    })
+
+    it("should handle invalid urls with empty map", async () => {
+      const { wrapper } = setup()
+      const onState = vi.fn()
+      const invalidUrl = "invalid-url" as AutomergeUrl
+
+      const Wrapped = () => (
+        <ErrorBoundary fallback={<div>Error!</div>}>
+          <NonSuspendingDocumentsComponent
+            urls={[invalidUrl]}
+            onState={onState}
+          />
+        </ErrorBoundary>
+      )
+
+      render(<Wrapped />, { wrapper })
+
+      // Initial state empty
+      let [docs] = onState.mock.lastCall
+      expect(docs.size).toBe(0)
+
+      // Should remain empty after attempted load
+      await act(async () => {
+        await Promise.resolve()
+      })
+
+      docs = onState.mock.lastCall[0]
+      expect(docs.size).toBe(0)
+    })
+  })
 })