-
Notifications
You must be signed in to change notification settings - Fork 14
Thenable query #32
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
base: main
Are you sure you want to change the base?
Thenable query #32
Conversation
|
@Tyler-Petrov is attempting to deploy a commit to the Convex Team on Vercel. A member of the Team first needs to authorize it. |
|
One thing I noticed is HMR doesn't re-render a component if the new handler is present in it. I'm not sure why this is as there isn't an error on the svelte boundary. I also wish there was a way to return the query result directly and not use a getter. This would be more inline with how Remote Functions work, but I don't see a way to accomplish that do to how Svelte's reactivity model works. |
42f4e04 to
c39f26b
Compare
|
Thanks for the contribution @Tyler-Petrov. Thanks for updating deps, I just landed that as #33 just to keep these PRs smaller since we squash the commits when we merge PRs. |
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.
Is there a better name for this? Between useQuery and convexQuery it's not clear to me which returns a promise.
I'd love docs on when to use which of these, I don't understand Svelte await yet and I dont' know
I'd rather not duplicate so much code here, it looks like the meat of this PR is this code below. Could we rename useQuery to useQueryInner, and make two wrappers for it, useQuery and useQueryAsync or useQueryAwaitable or something?
type PromiseResult = { get data(): NonNullable<FunctionReturnType<Query>> };
let resolveProxyPromise: (value: PromiseResult) => void;
let rejectProxyPromise: (value: Error) => void;
const proxyPromise = new Promise<PromiseResult>((resolve, reject) => {
resolveProxyPromise = resolve;
rejectProxyPromise = reject;
});
$effect(() => {
if (error) {
rejectProxyPromise(error);
} else if (data) {
resolveProxyPromise({
get data() {
return data
}
});
}
});
// This TypeScript cast promises data is not undefined if error and isLoading are checked first.
return {
then: (
onfulfilled: (value: PromiseResult) => void,
onrejected: (reason: any) => void
) => {
proxyPromise.then(onfulfilled, onrejected);
},
} as const;
src/lib/thenable-query.svelte.ts
Outdated
| } | ||
| return undefined; | ||
| }); | ||
| const isLoading = $derived(error === undefined && data === undefined); |
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 looks unused, along with isStale above. Is there something special going on here that makes this used?
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.
The isLoading and isStale properties were returned as a part of the query result in the useQuery implementation. I didn't remove them so that we could have a discussion on if they needed to be apart of the new handler. I should have originally added a little section about that in the PR description
isLoading:
As for me I don't see this as a helpful property anymore because the promise itself will say if it has/hasn't resolved
isStale:
TBH I didn't really get the use of this property. My guess is if the client looses connection with the server, but still has old data, but I don't know
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.
Cool, I agree isLoading should be removed. isStale is true when you change the arguments to a query but the old results are still displayed because the new results haven't loaded, see the "Display old results while loading" checkbox in https://convex-svelte.vercel.app/ for an example.
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.
I missed this comment. That makes sense. My most recent implementation doesn't handle args changing internally for better or worse
src/lib/thenable-query.svelte.ts
Outdated
| // If the args have never changed, fine to use initialData if provided. | ||
| haveArgsEverChanged: boolean; | ||
| } = $state({ | ||
| result: extractSnapshot(options).initialData, |
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.
some changes like this and MaybeGetter look like changes we should make to src/lib/client.svelte.ts too, could be a separate PR but would be nice to fix these as well
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.
Agreed. I'll defer to you on if you want a separate PR. I'm happy to actually put it together if that's what we decide
|
Thanks for the contribution. As an experimental thing, happy to merge this with all the duplicated code and duplicated example code so folks can try it out; the only thing stopping me is a description of why someone might use this instead and a better name, because To get this in beyond an experimental thing I'd like to not duplicate so much code. I also want to understand what this is good for, why this is a helpful new API. If it's better, we can tell everyone to use it instead! I just don't understand what's nice about |
Makes sense. I'll keep that in mind. |
src/lib/thenable-query.svelte.ts
Outdated
| * @param options - UseQueryOptions like `initialData` and `keepPreviousData`. | ||
| * @returns an object containing data, isLoading, error, and isStale. | ||
| */ | ||
| export function convexQuery<Query extends FunctionReference<'query'>>( |
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.
The name for the new function totally depends on if it will be the "condoned" handler to use. await is under an experimental flag until Svelte 6 is released, so naming it query might not be the best idea until then. I don't love the idea of having it named anything else after Svelte 6 though. For example having to call useAsyncQuery seems verbose if it will be the most idiomatic solution in a few months time.
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.
Let's say it's not the replacement API until Svelte 6 is released or until I understand why it's better. Until then call it something else or import it from a different entry point, like import { convexQuery } from convex-svelte/awaitable.
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.
I like that. I opted for import { convexQuery } from convex-svelte/async instead, but same idea
src/lib/thenable-query.svelte.ts
Outdated
| * @param query - a FunctionRefernece like `api.dir1.dir2.filename.func`. | ||
| * @param args - The arguments to the query function. | ||
| * @param options - UseQueryOptions like `initialData` and `keepPreviousData`. | ||
| * @returns an object containing data, isLoading, error, and isStale. |
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.
note this is no longer true, please update this docstring and remove as much code as possible from this implementation if we're duplicating code
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.
Right 😅
|
Got it, now that I understand this is a proposal to (eventually) replace the existing |
There are a few things that add up to a big DX win.
|
|
Oh cool! Sounds like what I'm familiar with in React as "suspense." In this case I don't think we'll want to get rid of useQuery, which works better for when you don't want to block the render, but we definitely want this! One possible API is Happen to know what any other data fetching libraries are doing for this? I don't see a mention of this in TanStack Query for Svelte. |
Yeah, it's an exciting day to be a Svelte dev! 😁
The closest thing I know of right now is Svelte's Remote Functions. It uses a It is possible to have queries that don't block render by wrapping the I don't personally love the idea of |
| export const firstMessage = query({ | ||
| args: { | ||
| fail: v.boolean() | ||
| }, |
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 is in for dev purposes for now, and won't be in the final PR
src/lib/convex-query.svelte.ts
Outdated
| const unsubscribe = client.onUpdate( | ||
| queryFunc, | ||
| args, | ||
| (result) => { |
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 onUpdate call makes convexQuery recursive when called from inside the markup. After setting state.current the whole function gets rebuilt, so onUpdate runs again. The function still works. I'd be curious if anyone has any solutions to this problem. I looked into getting the underlying query token from the convex client, but I gave up trying to find methods that weren't private to do what I wanted.
src/lib/convex-query.svelte.ts
Outdated
| /* If the query is already in the cache, return the cached value */ | ||
| client.query(queryFunc, args).then((result) => { | ||
| resolve(value ?? result); | ||
| }).catch((err) => { |
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 call doesn't trigger another call to the server, as there's a local cache for onUpdate (which query calls under the hood)
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.
For dev on this feature. Won't be included in final PR
src/lib/async.svelte.ts
Outdated
| >( | ||
| queryFunc: Query, | ||
| args: Args, | ||
| options: ConvexQueryOptions<Query> = {} |
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.
The only option is initialData now. Should initialData be a top level parameter
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.
No let's leave it as options in case something else comes up, we're always considering all kinds of options like how long to stay subscribed after the subscription is no longer needed, whether to wait for auth, etc
Having two separate functions seems a bit finicky and weird since they literally do the exact same thing other than async, and once asynchronous svelte comes out I think it would make sense for this to be the default (maybe even potentially completely removing the non async version). As a compromise maybe mark the current version of convex-svelte as a svelte 5 specific version (and have 2 separate functions), but once svelte 6 is fully released remove the non async version? |
|
There is also the potential that the overall strategy for convex-svelte could change with svelte 6 if it somehow becomes possible to somehow declare query's in a remote function (but I don't know enough to warrant if that is a plausible guess or not on my part). EDIT actually now that I'm thinking about this more carefully the only benefit I can see with this would be bypassing the need to have an initialData parameter and a load function to populate it. Simplifying SSR and only needing to have code in one place... still I'm unsure if this is possible. |
|
To take it further. You can declare a remote function like this: import { query } from '$app/server';
import * as db from '$lib/server/database';
export const getPosts = query(async () => {
const posts = await db.sql`
SELECT title, slug
FROM post
ORDER BY published_at
DESC
`;
return posts;
});And then use it in your page however you see fit (either in the script or in your markdown, and you don't need to hold it in a variable since it is automatically cached ). <script lang="ts">
import { getPosts } from './data.remote';
// // You can also do it this way
// let posts = getPosts();
</script>
<h1>Recent posts</h1>
<ul>
{#each await getPosts() as { title, slug }}
<li><a href="/blog/{slug}">{title}</a></li>
{/each}
<!---{#each await posts as { title, slug }}
<li><a href="/blog/{slug}">{title}</a></li>
{/each}-->
<button onclick={() => getPosts().refresh()}>
Check for new posts
</button>
</ul>EDIT: wouldn't want to use refresh() since it calls the remote function from the server (realizing this after Tyler pointed it out)
If convex-svelte could somehow take advantage of this I think it would be excellent. Although this is perhaps now getting pretty far off topic 😅 |
|
The end game is to remove the old I don't see how Remote Functions are helpful here. They're designed to facilitate communication from the backend to the frontend via a developer friendly api, but Convex functions are on their own server (aka not in SvelteKit). This means that best case SvelteKit is listening to updates from Convex and then streaming the updates to the client. It's round about, and I don't see any advantages. If you're worried about missing out on the DX of Remote Functions have no fear, because the new handler has a very similar API. If you'd like you can await the query right in the markup like so: Separate calls of |
|
You would use the remote function PURELY to load the
I'm now realizing how silly this ^ comment was on my part since you have to define your backend queries in the |
|
Hindsight is 20/20 😂 So actually what we have now will work SSR once Svelte has an async renderer. Remote Functions is just a fancy wrapper around Here's a video of a talk by Rich Harris called Promise.then() where he lays out how Remote Functions and Async Svelte work. Would highly recommend! It got me up to speed on everything. |
|
I have yet again rewrote the handler. This time (thanks to augment code) I copied the client API that Svelte uses internally for Remote Functions, and customized it to our needs. I also made a cache so that calls to |
|
No way, did you get it working with RemoteFunctions?... or am I misunderstanding something again XD |
|
Yes and no (mostly no 🙃). It uses a customized copy of the implementation for the client Remote Functions API. Yesterday I just pushed out an update that fixes an async reactivity loss error, so everything should be mostly usable. There are a few other minor bugs still. If you clone the repo there's an example page for the new async handler, so that's a good place to start messing around with it. |
This PR adds a handler for calling (and subscribing to) a convex query using Svelte's new
awaitkeyword. I made no attempt to share logic with the olduseQueryhandler and my new implementation. Everything I've added is in a newthenable-query.svelte.ts. I also added a new example page under /thenable to show the new handler.I also upgraded all the dev deps, and added runed and esm-env (which should have been there previously) as standard deps.
I'm new to this whole PR thing, so I welcome feedback on the PR itself and my presentation.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.