feat: experimental query.live remote function#15705
feat: experimental query.live remote function#15705elliott-with-the-longest-name-on-github wants to merge 67 commits intomainfrom
query.live remote function#15705Conversation
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
elliott-with-the-longest-name-on-github
changed the title
query.live
elliott-with-the-longest-name-on-github
changed the title
query.livequery.live remote function
| * @param {() => void} [on_connect] | ||
| * @returns {AsyncGenerator<T>} | ||
| */ | ||
| export async function* create_live_iterator( |
There was a problem hiding this comment.
why is this stuff and related in shared.svelte.js when it's only used in query.svelte.js? If it's because query.svelte.js is so big then maybe we should have a query.live.svelte.js instead (or just .js if we don't use runes in there)
There was a problem hiding this comment.
I've really just been thinking of shared.svelte.js as utils.svelte.js, which is why I put them here. It could make sense to move them to their own file in the future but right now the file is still relatively small and easy to follow
|
I still think some of this should be extracted out into their own PR (bugfixes, adjacent work like the refreshes none stuff). The live work itself is LGTM |
| '@sveltejs/kit': patch | ||
| --- | ||
|
|
||
| fix: empty call to `.updates()` on a command/form invocation means "don't update anything" |
There was a problem hiding this comment.
Not sure if this is addressed elsewhere in the PR (am going top-to-bottom) but we really need a dedicated API for this, and the logic needs to be on the server.
I don't think updates() by itself should prevent things from being updated. That doesn't really make sense to me — if updates(fn) doesn't cause fn(...) to refresh (which it doesn't, unless there's corresponding logic on the server) then it's weird for the absence of an argument to prevent updates
There was a problem hiding this comment.
We do need to figure out a way to drive this from the server, but this is a bug anyway. If updates(query) on a form causes query to be updated and form to not invalidateAll, updates() should also cause form to not invalidateAll. My overall point being that it would be really weird for all invocations of updates to disable invalidateAll unless the number of arguments is 0, in which case it still does invalidateAll. In the case where you're doing something like updates(...array), you could just... end up with difficult-to-understand behavior. I think calling updates at all is a clear indication that you do not intend for this submission to invalidateAll.
There was a problem hiding this comment.
Hmm, yeah that logic makes sense. I would definitely quibble with 'clear indication', and part of me wonders if it should be updates(false) or updates(null) to make it explicit, but that would veer into making this the API for preventing invalidateAll rather than that making an incidental side-effect of updates behaving more consistently
| * @param {any} arg | ||
| */ | ||
| const run = (event, state, arg) => | ||
| run_remote_generator(event, state, false, () => validate(arg), fn, __.name); |
There was a problem hiding this comment.
does it make sense for this function to live in shared.js? It's not shared, it's only used here — it would make query.js larger but maybe it would be clearer to just inline the logic? If we're worried about the size of query.js then maybe breaking query.live (and query.batch) out into a separate module is the move
There was a problem hiding this comment.
See Elliott it's two people asking this question now, we should move that somewhere else 😛
Rich-Harris
left a comment
Rich-Harris
left a comment
There was a problem hiding this comment.
recapping offline discussion: LGTM aside from #15705 (comment) — for now, if the initial connection fails then the promise should reject. if we want it to retry (and hopefully eventually resolve) instead then we need to design that, and make it consistent with query/query.batch
3 participants
Alternative to #15563. I reimplemented a bunch of stuff to fix a bunch of papercuts, figured it would be easier to review this way. I still need to add
hydratablesupport.TODO (can probably be followups):
invalidateNoneonformcalls