[draft] The end of undefined docSync() / await doc() #402
Conversation
| const clientDoc = client.find(doc.url) | ||
| await pause(100) | ||
| assert.strictEqual(clientDoc.docSync(), undefined) | ||
| expect(async () => { |
There was a problem hiding this comment.
When passing an async function to expect the return value becomes a promise itself which needs to be awaited.
| expect(async () => { | |
| await expect(async () => { |
| @@ -1335,16 +1290,16 @@ describe("Repo", () => { | |||
| const aliceDoc = aliceRepo.create() | |||
| aliceDoc.change((doc: any) => (doc.text = "Hello world")) | |||
|
|
|||
| const bobDoc = bobRepo.find(aliceDoc.url) | |||
| await eventPromise(bobDoc, "unavailable") | |||
| expect(async () => { | |||
There was a problem hiding this comment.
| expect(async () => { | |
| await expect(async () => { |
| expect( | ||
| await repo.find<{ foo: string }>(handle2.documentId).doc() | ||
| ).toEqual(undefined) | ||
| expect(async () => { |
There was a problem hiding this comment.
| expect(async () => { | |
| await expect(async () => { |
| @@ -706,7 +695,7 @@ describe("Repo", () => { | |||
| } | |||
|
|
|||
| await connectedPromise | |||
|
|
|||
| console.log("All connected") | |||
There was a problem hiding this comment.
| console.log("All connected") |
| @@ -574,9 +562,10 @@ describe("Repo", () => { | |||
| }) | |||
|
|
|||
| // Could not find the document that is not yet saved because of slow storage. | |||
| const reloadedHandle = repo2.find<{ foo: string }>(handle.url) | |||
| expect(async () => { | |||
There was a problem hiding this comment.
| expect(async () => { | |
| await expect(async () => { |
| const handle = repo.find<TestDoc>(url) | ||
| assert.equal(handle.isReady(), false) | ||
| await eventPromise(handle, "unavailable") | ||
| expect(async () => { |
There was a problem hiding this comment.
| expect(async () => { | |
| await expect(async () => { |
| } catch (e: any) { | ||
| assert.equal(e.message, "Invalid AutomergeUrl: 'invalid-url'") | ||
| } | ||
| expect(async () => { |
There was a problem hiding this comment.
| expect(async () => { | |
| await expect(async () => { |
| "received sync message in state", | ||
| this.#handle.documentId, | ||
| this.#handle.state | ||
| ) |
There was a problem hiding this comment.
This probably need removal before merging too?
|
I see you're not awaiting the |
|
Nice, excited to see this revisited. Can you talk about how this affects this diagram? |
…resumably caused by my recent patch
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. |
…tion. this code needs serious rethinking
There was a problem hiding this comment.
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.
| export const wrapperCache = new Map< | ||
| AnyDocumentId, | ||
| ReturnType<typeof wrapPromise> | ||
| >() |
There was a problem hiding this comment.
Do we really want this global, and not, e.g., living in something that also wraps a RepoContext.Provider?
| if (suspense) { | ||
| return wrapper.read() as DocHandle<T> | ||
| } else { | ||
| return handle || undefined |
There was a problem hiding this comment.
What does || undefined give you here? Isn't the handle already a DocHandle or undefined?
|
|
||
| useEffect(() => { | ||
| if (suspense === false) { | ||
| void wrapper.promise |
There was a problem hiding this comment.
Ah, this is to deal with floating promises? But you have a then and catch here, which should take care of that, no?
| } | ||
|
|
||
| useEffect(() => { | ||
| if (suspense === false) { |
There was a problem hiding this comment.
I think this would be simpler as
if (suspense) {
return
}and then handle the non-suspense case unconditionally.
|
|
||
| // 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 !? Aren't you testing if (!wrapper) right below?
| @@ -407,7 +398,7 @@ export class Repo extends EventEmitter<RepoEvents> { | |||
| ) | |||
| } | |||
|
|
|||
| const sourceDoc = clonedHandle.docSync() | |||
| const sourceDoc = clonedHandle.doc() | |||
| if (!sourceDoc) { | |||
| const { handleA, wrapper } = setup() | ||
| const onDoc = vi.fn() | ||
| // First we should see the loading state | ||
| expect(screen.getByTestId("loading")).toBeInTheDocument() |
There was a problem hiding this comment.
We expect to see a loading state even if we're dealing with a locally-created document?
| it("avoids showing stale data", async () => { | ||
| const { handleA, handleSlow, wrapper } = setup() | ||
| // Test document changes during loading | ||
| it("should handle document changes while loading", async () => { |
| }) | ||
|
|
||
| await act(async () => { | ||
| await Promise.resolve() |
There was a problem hiding this comment.
Is this waiting for useEffect? It might be nice to have a comment.
There was a problem hiding this comment.
Ah, this is waiting for the doc to load, right?
| ) | ||
| await waitFor(() => unmount()) | ||
| } | ||
| it("should handle multiple documents and parallel changes", async () => { |
|
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. |
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.)