The end of undefined docSync() / await doc() #402
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import { AnyDocumentId, DocHandle } from "@automerge/automerge-repo/slim" | ||
| import { PromiseWrapper, wrapPromise } from "./wrapPromise.js" | ||
| import { useRepo } from "./useRepo.js" | ||
| import { useEffect, useRef, useState } from "react" | ||
|
|
||
| // Shared with useDocHandles | ||
| export const wrapperCache = new Map< | ||
| AnyDocumentId, | ||
| PromiseWrapper<DocHandle<unknown>> | ||
| >() | ||
|
There was a problem hiding this comment. Do we really want this global, and not, e.g., living in something that also wraps a There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. my weak compromise was to add the comment below. if you find this too odious, i'm open to improving on it. There was a problem hiding this comment. 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. |
||
| // NB: this is a global cache that isn't keyed on the Repo | ||
| // so if your app uses the same documents in two Repos | ||
| // this could cause problems. please let me know if you do. | ||
|
|
||
| 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 | undefined, | ||
| params?: UseDocHandleSynchronousParams | ||
| ): DocHandle<T> | undefined | ||
| export function useDocHandle<T>( | ||
| id: AnyDocumentId | undefined, | ||
| { suspense }: UseDocHandleParams = { suspense: false } | ||
| ): DocHandle<T> | undefined { | ||
| const repo = useRepo() | ||
| const controllerRef = useRef<AbortController>() | ||
| const [handle, setHandle] = useState<DocHandle<T> | undefined>() | ||
|
|
||
| let wrapper = id ? wrapperCache.get(id) : undefined | ||
| if (!wrapper && id) { | ||
| controllerRef.current?.abort() | ||
| controllerRef.current = new AbortController() | ||
|
|
||
| const promise = repo.find<T>(id, { signal: controllerRef.current.signal }) | ||
| wrapper = wrapPromise(promise) | ||
| wrapperCache.set(id, wrapper) | ||
| } | ||
|
|
||
| /* From here we split into two paths: suspense and not. | ||
| * In the suspense path, we return the wrapper directly. | ||
| * In the non-suspense path, we wait for the promise to resolve | ||
| * and then set the handle via setState. Suspense relies on | ||
| * re-running this function until it succeeds, whereas the synchronous | ||
| * form uses a setState to track the value. */ | ||
| useEffect(() => { | ||
| if (suspense || !wrapper) { | ||
| return | ||
| } | ||
| wrapper.promise | ||
| .then(handle => { | ||
| setHandle(handle as DocHandle<T>) | ||
| }) | ||
| .catch(() => { | ||
| setHandle(undefined) | ||
| }) | ||
| }, [suspense, wrapper]) | ||
|
|
||
| if (suspense && wrapper) { | ||
| return wrapper.read() as DocHandle<T> | ||
| } else { | ||
| return handle | ||
| } | ||
| } | ||
| 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)! | ||
|
There was a problem hiding this comment. Why |
||
| if (!wrapper) { | ||
| try { | ||
| const promise = repo.find<T>(id) | ||
| wrapper = wrapPromise(promise) | ||
| wrapperCache.set(id, wrapper) | ||
| } catch (e) { | ||
|
There was a problem hiding this comment. unavailable. this should probably have a test, now i think about it. i'm not sure what the behaviour would be without thinking hard which suggests they're probably wrong. There was a problem hiding this comment. Where is that thrown? I'm probably missing something, but as far as I can tell, neither |
||
| 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]) | ||
|
There was a problem hiding this comment. You're omitting There was a problem hiding this comment. having that plugin would have saved me a couple stupid hours the other day, so i guess i will install it and fix the gripes! There was a problem hiding this comment. alright, well, that's half an hour of my life. i think the bigger problem is that the effect probably should be managed differently. adding this to my TODO list at least. There was a problem hiding this comment. You're still omitting |
||
|
|
||
| return handleMap | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.