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

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

Draft
wants to merge 41 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
50cdcf0
Mostly-working async find implementation, a few failing tests here.
pvh Dec 30, 2024
e510bb5
wip -- working on tests. there's an asymmetry to document discovery p…
pvh Dec 31, 2024
9fdda55
wip -- heaps of console logs but fixed a subtle networking race condi…
pvh Dec 31, 2024
72e6490
one failing test left
pvh Dec 31, 2024
6ddc2b2
fix those two tests
pvh Jan 3, 2025
858bd27
throw on not ready
pvh Dec 31, 2024
87d8fb6
remove unhelpful test and some cnosole.logs
pvh Jan 4, 2025
4be34ba
all tests and tsc passing
pvh Jan 4, 2025
5fa277f
alright, find is now abortable
pvh Jan 4, 2025
70ed5dc
added AbortController support -- couple of failing tests but it's too…
pvh Jan 4, 2025
1f0c7a0
fix abort test
pvh Jan 4, 2025
496fdc5
add a check that aborting also prevents going to unavailable
pvh Jan 4, 2025
4da60ad
add a check that aborting also prevents going to unavailable
pvh Jan 4, 2025
5bc01a8
wip of new useDoc/Handle
pvh Jan 4, 2025
c61e7e4
working on the react-hooks rewrite, still messy in here
pvh Jan 5, 2025
f548d88
okay, tests passing. still more to do
pvh Jan 5, 2025
1f73b8b
working on tests
pvh Jan 6, 2025
bc4fdcf
alright, more passing tests
pvh Jan 6, 2025
da5ed07
remove a duplicate test
pvh Jan 6, 2025
4d5f507
fix the test that was corrupting the shared repo
pvh Jan 6, 2025
c584221
usedocument passing all tests too
pvh Jan 6, 2025
4ccd975
continued cleanup
pvh Jan 6, 2025
c32039a
cleaned up the comment
pvh Jan 6, 2025
169b11d
failed wip
pvh Jan 6, 2025
5e7d3d1
useDocuments passing some basic tests
pvh Jan 6, 2025
4d4c763
one failing test: suspense false on useDocHandles
pvh Jan 6, 2025
ee74e58
update react-todo
pvh Jan 6, 2025
4b90c35
update react-counter example, too
pvh Jan 6, 2025
e880590
refactoring .doc away here
pvh Jan 6, 2025
5b91e7b
get rid of old .doc
pvh Jan 6, 2025
31b6d96
updating other packages
pvh Jan 6, 2025
65ffd20
clean up some warnings
pvh Jan 6, 2025
0f787e3
fix a couple comments I missed
pvh Jan 6, 2025
95f4882
fix a couple tests broken by moving to std. error boundary
pvh Jan 6, 2025
f61d420
fix useDocHandle
pvh Jan 7, 2025
98f40a4
okay, more tests passing
pvh Jan 7, 2025
73f0838
okay, more decent tests for useDocuments
pvh Jan 7, 2025
f4258e6
working on an async generator with progress version of find
pvh Jan 7, 2025
d568155
restore abortcontroller support
pvh Jan 7, 2025
5015c44
the patch is in a messy state but tests are passing
pvh Jan 8, 2025
485f4c7
missed a file
pvh Jan 8, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/react-counter/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ declare global {
const rootDocUrl = `${document.location.hash.substring(1)}`
let handle
if (isValidAutomergeUrl(rootDocUrl)) {
handle = repo.find(rootDocUrl)
handle = await repo.find(rootDocUrl)
} else {
handle = repo.create<{ count: number }>({ count: 0 })
}
Expand Down
3 changes: 2 additions & 1 deletion examples/react-todo/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@
"classnames": "^2.3.2",
"postcss": "^8.4.21",
"react": "^18.2.0",
"react-dom": "^18.2.0"
"react-dom": "^18.2.0",
"react-error-boundary": "^5.0.0"
},
"devDependencies": {
"tailwindcss": "^3.2.4"
Expand Down
28 changes: 15 additions & 13 deletions examples/react-todo/src/App.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { AutomergeUrl } from "@automerge/automerge-repo"
import { useDocument, useRepo } from "@automerge/automerge-repo-react-hooks"
import cx from "classnames"
import { useRef, useState } from "react"
import { Suspense, useRef, useState } from "react"

import { Todo } from "./Todo.js"
import { ExtendedArray, Filter, State, TodoData } from "./types.js"
Expand All @@ -28,7 +28,7 @@ export function App({ url }: { url: AutomergeUrl }) {
if (!state) return []
return state.todos.filter(async url => {
if (filter === Filter.all) return true
const todo = await repo.find<TodoData>(url).doc()
const todo = (await repo.find<TodoData>(url)).doc()
if (filter === Filter.completed) return todo.completed
if (filter === Filter.incomplete) return !todo.completed
return false
Expand All @@ -38,7 +38,7 @@ export function App({ url }: { url: AutomergeUrl }) {
const destroyCompleted = async () => {
if (!state) return
for (const url of await getFilteredTodos(Filter.completed)) {
const todo = await repo.find<TodoData>(url).doc()
const todo = (await repo.find<TodoData>(url)).doc()
if (todo.completed) destroy(url)
}
}
Expand Down Expand Up @@ -89,16 +89,18 @@ export function App({ url }: { url: AutomergeUrl }) {

{/* todos */}
<section>
<ul className="border-y divide-y divide-solid">
{state.todos.map(url => (
<Todo
key={url}
url={url}
onDestroy={url => destroy(url)}
filter={filter}
/>
))}
</ul>
<Suspense fallback={<li>Loading todo items...</li>}>
<ul className="border-y divide-y divide-solid">
{state.todos.map(url => (
<Todo
key={url}
url={url}
onDestroy={url => destroy(url)}
filter={filter}
/>
))}
</ul>
</Suspense>
</section>

{/* footer tools */}
Expand Down
11 changes: 8 additions & 3 deletions examples/react-todo/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ import { BroadcastChannelNetworkAdapter } from "@automerge/automerge-repo-networ
import { BrowserWebSocketClientAdapter } from "@automerge/automerge-repo-network-websocket"
import { RepoContext } from "@automerge/automerge-repo-react-hooks"
import { IndexedDBStorageAdapter } from "@automerge/automerge-repo-storage-indexeddb"
import React from "react"
import React, { Suspense } from "react"
import { ErrorBoundary } from "react-error-boundary"
import ReactDOM from "react-dom/client"
import { App } from "./App.js"
import { State } from "./types.js"
Expand All @@ -27,7 +28,7 @@ declare global {
const rootDocUrl = `${document.location.hash.substring(1)}`
let handle
if (isValidAutomergeUrl(rootDocUrl)) {
handle = repo.find(rootDocUrl)
handle = await repo.find(rootDocUrl)
} else {
handle = repo.create<State>({ todos: [] })
}
Expand All @@ -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>}>
<Suspense fallback={<div>Loading...</div>}>
<App url={docUrl} />
</Suspense>
</ErrorBoundary>
</React.StrictMode>
</RepoContext.Provider>
)
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,7 @@ describe("Websocket adapters", () => {
})

// make a change to the handle on the sync server
const handle = repo.find<{ foo: string }>(url)
await handle.whenReady()
const handle = await repo.find<{ foo: string }>(url)
handle.change(d => (d.foo = "baz"))

// Okay, so now there is a document on both the client and the server
Expand Down Expand Up @@ -621,7 +620,7 @@ describe("Websocket adapters", () => {

let localHeads = A.getHeads(clientDoc)
let remoteHeads = handle.heads()
if (!headsAreSame(localHeads, remoteHeads)) {
if (!headsAreSame(localHeads, remoteHeads || [])) {
throw new Error("heads not equal")
}
})
Expand Down
2 changes: 2 additions & 0 deletions packages/automerge-repo-react-hooks/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
"react-usestateref": "^1.0.8"
},
"devDependencies": {
"@testing-library/jest-dom": "^6.6.3",
"@testing-library/react": "^14.0.0",
"jsdom": "^22.1.0",
"react-error-boundary": "^5.0.0",
"rollup-plugin-visualizer": "^5.9.3",
"vite-plugin-dts": "^3.9.1"
},
Expand Down
2 changes: 1 addition & 1 deletion packages/automerge-repo-react-hooks/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
*/
export { useDocument } from "./useDocument.js"
export { useDocuments } from "./useDocuments.js"
export { useHandle } from "./useHandle.js"
export { useDocHandle } from "./useDocHandle.js"
export { RepoContext, useRepo } from "./useRepo.js"
export {
useLocalAwareness,
Expand Down
67 changes: 67 additions & 0 deletions packages/automerge-repo-react-hooks/src/useDocHandle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { AnyDocumentId, DocHandle } from "@automerge/automerge-repo/slim"
import { wrapPromise } from "./wrapPromise.js"
import { useRepo } from "./useRepo.js"
import { useEffect, useRef, useState } from "react"

// Shared with useDocHandles
export const wrapperCache = new Map<
AnyDocumentId,
ReturnType<typeof wrapPromise>
>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


interface UseDocHandleSuspendingParams {
suspense: true
}
interface UseDocHandleSynchronousParams {
suspense: false
}

type UseDocHandleParams =
| UseDocHandleSuspendingParams
| UseDocHandleSynchronousParams

export function useDocHandle<T>(
id: AnyDocumentId,
params: UseDocHandleSuspendingParams
): DocHandle<T>
export function useDocHandle<T>(
id: AnyDocumentId,
params?: UseDocHandleSynchronousParams
): DocHandle<T> | undefined
export function useDocHandle<T>(
id: AnyDocumentId,
{ suspense }: UseDocHandleParams = { suspense: false }
): DocHandle<T> | undefined {
const repo = useRepo()
const controllerRef = useRef<AbortController>()
const [handle, setHandle] = useState<DocHandle<T> | undefined>()

let wrapper = wrapperCache.get(id)
if (!wrapper) {
controllerRef.current?.abort()
controllerRef.current = new AbortController()

const promise = repo.find<T>(id, { signal: controllerRef.current.signal })
wrapper = wrapPromise(promise)
wrapperCache.set(id, wrapper)
}

useEffect(() => {
if (suspense === false) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would be simpler as

if (suspense) {
  return
}

and then handle the non-suspense case unconditionally.

void wrapper.promise
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why void here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.then(handle => {
setHandle(handle as DocHandle<T>)
})
.catch(e => {
console.log("handle promise caught", e)
setHandle(undefined)
})
}
}, [suspense, wrapper])

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
75 changes: 75 additions & 0 deletions packages/automerge-repo-react-hooks/src/useDocHandles.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { AutomergeUrl, DocHandle } from "@automerge/automerge-repo/slim"
import { useState, useEffect } from "react"
import { useRepo } from "./useRepo.js"
import { PromiseWrapper, wrapPromise } from "./wrapPromise.js"
import { wrapperCache } from "./useDocHandle.js"

interface UseDocHandlesParams {
suspense?: boolean
}

type DocHandleMap<T> = Map<AutomergeUrl, DocHandle<T> | undefined>

export function useDocHandles<T>(
ids: AutomergeUrl[],
{ suspense = false }: UseDocHandlesParams = {}
): DocHandleMap<T> {
const repo = useRepo()
const [handleMap, setHandleMap] = useState<DocHandleMap<T>>(() => new Map())

const pendingPromises: PromiseWrapper<DocHandle<T>>[] = []
const nextHandleMap = new Map<AutomergeUrl, DocHandle<T> | undefined>()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (!wrapper) {
try {
const promise = repo.find<T>(id)
wrapper = wrapPromise(promise)
wrapperCache.set(id, wrapper)
} catch (e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this catching?

continue
}
}

// Try to read each wrapper.
// Update handleMap with any available handles,
// and collect any pending promises
try {
const handle = wrapper.read() as DocHandle<T>
nextHandleMap.set(id, handle)
} catch (e) {
if (e instanceof Promise) {
pendingPromises.push(wrapper as PromiseWrapper<DocHandle<T>>)
} else {
nextHandleMap.set(id, undefined)
}
}
}

// If any promises are pending, suspend with Promise.all
if (suspense && pendingPromises.length > 0) {
throw Promise.all(pendingPromises.map(p => p.promise))
}

useEffect(() => {
if (pendingPromises.length > 0) {
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)
}
}, [suspense, ids])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're omitting pendingPromises and nextHandleMap from the useEffect dependencies. And suspense and ids are there even though they're not actually referenced in the useEffect function body. I think your logic checks out, but if you add eslint-plugin-react-hooks at some point, it will complain. Of course, you can't just add those deps since those variables are re-initialized on every render, but I think this at least deserves a comment for the poor soul who may have to debug this.


return handleMap
}
Loading