fix: dedupe remote data#15991
Conversation
🦋 Changeset detectedLatest commit: 3f99dfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…[key])`, so falsy server-resolved values (`0`, `false`, `null`, `''`) are ignored, causing an unnecessary batch fetch and possible hydration mismatch.
This commit fixes the issue reported at packages/kit/src/runtime/client/remote-functions/query-batch.svelte.js:21
## Bug
In `query-batch.svelte.js` the hydration shortcut was:
```js
if (query_responses[key]) {
const value = query_responses[key];
delete query_responses[key];
return value;
}
```
After the dedupe rework, `query_responses[key]` holds an **already-decoded query result** (revived from `__SVELTEKIT_PAYLOAD__.query`). The server populates this from `store[remote_key] = v` in `render.js` (lines 550 / 557), which stores the resolved value **even when it is falsy**.
So if a batched query legitimately resolves to `0`, `false`, `null`, or `''`, the truthy guard is `false`. The cached value is then:
- not returned (so the client fires a redundant real batch `fetch` for data it already has), and
- not `delete`d from `query_responses`.
This is concretely triggerable: any `query.batch` returning a falsy value on the server.
## Fix
Match the existence-aware check already used by the other consumers:
- `query/index.js` uses `if (Object.hasOwn(query_responses, key))`
- `query-live/instance.svelte.js` checks `value !== undefined`
Changed the batch path to `if (Object.hasOwn(query_responses, key))`, which correctly distinguishes "a hydrated value (possibly falsy) exists" from "no hydrated value".
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
| : undefined | ||
| }); | ||
|
|
||
| void svelte.settled?.().then(() => { |
There was a problem hiding this comment.
settled was only added in Svelte 5.36. Does it matter to us to execute this even if settled is missing?
There was a problem hiding this comment.
If settled is missing then it means there's no async support which means you can't be using remote functions, meaning this wouldn't do anything
There was a problem hiding this comment.
Ah right. Forgot we made that change. Did we also get rid of query.current, etc?
There was a problem hiding this comment.
They're still there, but you can't use experimental.remoteFunctions without experimental.async
There was a problem hiding this comment.
If we're adding the requirement for settled but removing hydratable, we probably should create unfriendly_settled so that the error is clear -- you can enable remote functions without enabling async, so adding a more-helpful error before we stabilize both might be smart.
Alternatively maybe we should add an error to the config resolver if you have remote functions but not async?
There was a problem hiding this comment.
you can enable remote functions without enabling async
errr that must be a recent change. there was definitely a recent point at which that wasn't possible
honestly I would still just ignore it. the worst case scenario here is that the object persists longer than designed in the incredibly niche remote-but-no-async case, and only if someone uses some data on the server but then manages to not use it until some time in the future after which they should have refreshed (which would clear the relevant key) but failed to
for an experimental feature this is an awful lot of fucks to give
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
Everything looks good except for the removal of entry.serialize, which needs to remain and just have its behavior adjusted.
I agree with our new model of "data is fresh until .refresh" that hydratable is no longer necessary, which is cool and a nice simplification!
| for (const key in cache) { | ||
| const entry = cache[key]; | ||
|
|
||
| if (!entry.serialize) continue; |
There was a problem hiding this comment.
We can't just remove this behavior; in fact as we discussed this morning we need to modify it to be stricter. This causes us to automatically serialize every query, even if it's only ever used in a server context
There was a problem hiding this comment.
Are you sure? If I do this...
<script>
import { browser } from '$app/env';
import { shout } from './stuff.remote';
const promise = shout('hello');
</script>
{#if browser}
<p>{await promise}</p>
{/if}...the query isn't serialized. Stuff only gets serialized if it's included in state.remote.explicit (because of refresh() or set(...), or is included in state.remote.implicit and is ready by the time all the blocking work is done.
| }) | ||
| await Promise.race([ | ||
| Promise.resolve(value).then((v) => resolved && (store[remote_key] = v), noop), | ||
| Promise.resolve().then(() => (resolved = false)) |
There was a problem hiding this comment.
Did you look at the previous queueMicrotask behavior at all / try to figure out why we did it? It seems like a weirdly specific thing to include on accident
There was a problem hiding this comment.
yeah this just seemed simpler and more symmetrical. if value is resolved then the handlers are guaranteed to run before resolved = false
I thought that sveltejs/svelte#18406 would be necessary to do this, but I was wrong — it actually seems easier and simpler to bypass
hydratablealtogether. Instead, we serialize all the remote data in one go, allowing devalue to do its thing and deduplicate everything. (I still think it's worth merging that PR.)That way, if you have (for example) a
getUser(): Promise<User | null>and arequireUser(): Promise<User>that callsgetUserinternally (and redirects if it returnsnull), theUserobject is only serialized once.I can't remember exactly why we chose to use
hydratablein the first place. Perhaps it was a lifecycle thing — we wanted to be careful about data being stale by the time it was read? In which case I think that has changed now that query lifecycle is determined by the garbage collector. Maybe @elliott-with-the-longest-name-on-github remembers.Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits