-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add new remote function query.batch
#14272
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
Changes from 17 commits
1affeca
b70a394
bd0c519
41fad93
4819f97
a955c16
1b5f453
5da377d
e036a27
afdae97
6434cf3
e630de7
cbfa1cf
aa7cd56
910125c
2ca6eb1
dfa4cb4
44f60b8
d8d02bc
0986c29
4e81119
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@sveltejs/kit': minor | ||
--- | ||
|
||
feat: add new remote function `query.batch` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,3 +104,145 @@ export function query(validate_or_fn, maybe_fn) { | |
|
||
return wrapper; | ||
} | ||
|
||
/** | ||
* Creates a batch query function that collects multiple calls and executes them in a single request | ||
* | ||
* See [Remote functions](https://svelte.dev/docs/kit/remote-functions#query.batch) for full documentation. | ||
* | ||
* @template Input | ||
* @template Output | ||
* @overload | ||
* @param {'unchecked'} validate | ||
* @param {(args: Input[]) => MaybePromise<(arg: Input, idx: number) => Output>} fn | ||
* @returns {RemoteQueryFunction<Input, Output>} | ||
* @since 2.35 | ||
*/ | ||
/** | ||
* Creates a batch query function that collects multiple calls and executes them in a single request | ||
* | ||
* See [Remote functions](https://svelte.dev/docs/kit/remote-functions#query.batch) for full documentation. | ||
* | ||
* @template {StandardSchemaV1} Schema | ||
* @template Output | ||
* @overload | ||
* @param {Schema} schema | ||
* @param {(args: StandardSchemaV1.InferOutput<Schema>[]) => MaybePromise<(arg: StandardSchemaV1.InferOutput<Schema>, idx: number) => Output>} fn | ||
* @returns {RemoteQueryFunction<StandardSchemaV1.InferInput<Schema>, Output>} | ||
* @since 2.35 | ||
*/ | ||
/** | ||
* @template Input | ||
* @template Output | ||
* @param {any} validate_or_fn | ||
* @param {(args?: Input[]) => MaybePromise<(arg: Input, idx: number) => Output>} [maybe_fn] | ||
* @returns {RemoteQueryFunction<Input, Output>} | ||
* @since 2.35 | ||
*/ | ||
/*@__NO_SIDE_EFFECTS__*/ | ||
function batch(validate_or_fn, maybe_fn) { | ||
/** @type {(args?: Input[]) => (arg: Input, idx: number) => Output} */ | ||
const fn = maybe_fn ?? validate_or_fn; | ||
|
||
/** @type {(arg?: any) => MaybePromise<Input>} */ | ||
const validate = create_validator(validate_or_fn, maybe_fn); | ||
|
||
/** @type {RemoteInfo & { type: 'query_batch' }} */ | ||
const __ = { | ||
type: 'query_batch', | ||
id: '', | ||
name: '', | ||
run: (args) => { | ||
const { event, state } = get_request_store(); | ||
|
||
return run_remote_function( | ||
event, | ||
state, | ||
false, | ||
args, | ||
(array) => Promise.all(array.map(validate)), | ||
fn | ||
); | ||
} | ||
}; | ||
|
||
/** @type {{ args: any[], resolvers: Array<{resolve: (value: any) => void, reject: (error: any) => void}> }} */ | ||
let batching = { args: [], resolvers: [] }; | ||
|
||
/** @type {RemoteQueryFunction<Input, Output> & { __: RemoteInfo }} */ | ||
const wrapper = (arg) => { | ||
if (prerendering) { | ||
throw new Error( | ||
`Cannot call query.batch '${__.name}' while prerendering, as prerendered pages need static data. Use 'prerender' from $app/server instead` | ||
); | ||
} | ||
|
||
const { event, state } = get_request_store(); | ||
|
||
/** @type {Promise<any> & Partial<RemoteQuery<any>>} */ | ||
const promise = get_response(__.id, arg, state, () => { | ||
// Collect all the calls to the same query in the same macrotask, | ||
// then execute them as one backend request. | ||
return new Promise((resolve, reject) => { | ||
// We don't need to deduplicate args here, because get_response already caches/reuses identical calls | ||
batching.args.push(arg); | ||
batching.resolvers.push({ resolve, reject }); | ||
|
||
if (batching.args.length > 1) return; | ||
|
||
setTimeout(async () => { | ||
const batched = batching; | ||
dummdidumm marked this conversation as resolved.
Show resolved
Hide resolved
|
||
batching = { args: [], resolvers: [] }; | ||
|
||
try { | ||
const get_result = await run_remote_function( | ||
event, | ||
state, | ||
false, | ||
batched.args, | ||
(array) => Promise.all(array.map(validate)), | ||
fn | ||
); | ||
|
||
for (let i = 0; i < batched.resolvers.length; i++) { | ||
batched.resolvers[i].resolve(get_result(batched.args[i], i)); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is quite right — if you have 10 items you might resolve the first 3, then the 4th errors, and then you call If the call to for (let i = 0; i < batched.resolvers.length; i += 1) {
const resolver = batched.resolvers[i];
try {
resolver.resolve(get_result(batched.args[i], i));
} catch (error) {
resolver.reject(error);
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. started implementing this but stumbled into an error handling question: Right now, we do call the Update: The mismatch AFAICT ends up being:
Right now this wouldn't make a difference, because on the server the error bubbles up all the way to the root, and will be handled there by our root-handleError catcher. But once we we have async SSR in place, and a boundary catches an error on the server (if we want that to happen; maybe a separate discussion), then it gets the raw error on the server but a transformed-to- Update: We concluded that we won't change the behavior of boundaries in SSR, i.e. they still won't catch errors, so the behavior will be the same, so it's fine as is. |
||
} catch (error) { | ||
for (const resolver of batched.resolvers) { | ||
resolver.reject(error); | ||
} | ||
} | ||
}, 0); | ||
}); | ||
}); | ||
|
||
promise.catch(() => {}); | ||
|
||
promise.refresh = async () => { | ||
const { state } = get_request_store(); | ||
const refreshes = state.refreshes; | ||
|
||
if (!refreshes) { | ||
throw new Error( | ||
`Cannot call refresh on query.batch '${__.name}' because it is not executed in the context of a command/form remote function` | ||
); | ||
} | ||
|
||
const cache_key = create_remote_cache_key(__.id, stringify_remote_arg(arg, state.transport)); | ||
refreshes[cache_key] = await /** @type {Promise<any>} */ (promise); | ||
}; | ||
|
||
promise.withOverride = () => { | ||
throw new Error(`Cannot call '${__.name}.withOverride()' on the server`); | ||
}; | ||
|
||
return /** @type {RemoteQuery<Output>} */ (promise); | ||
}; | ||
|
||
Object.defineProperty(wrapper, '__', { value: __ }); | ||
|
||
return wrapper; | ||
} | ||
|
||
// Add batch as a property to the query function | ||
Object.defineProperty(query, 'batch', { value: batch, enumerable: true }); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
export { command } from './command.svelte.js'; | ||
export { form } from './form.svelte.js'; | ||
export { prerender } from './prerender.svelte.js'; | ||
export { query } from './query.svelte.js'; | ||
export { query, query_batch } from './query.svelte.js'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
/** @import { RemoteQueryFunction } from '@sveltejs/kit' */ | ||
/** @import { RemoteFunctionResponse } from 'types' */ | ||
import { app_dir, base } from '__sveltekit/paths'; | ||
import { remote_responses, started } from '../client.js'; | ||
import { app, goto, remote_responses, started } from '../client.js'; | ||
import { tick } from 'svelte'; | ||
import { create_remote_function, remote_request } from './shared.svelte.js'; | ||
import * as devalue from 'devalue'; | ||
import { HttpError, Redirect } from '@sveltejs/kit/internal'; | ||
|
||
/** | ||
* @param {string} id | ||
|
@@ -25,6 +28,93 @@ export function query(id) { | |
}); | ||
} | ||
|
||
/** | ||
* @param {string} id | ||
* @returns {(arg: any) => Query<any>} | ||
*/ | ||
export function query_batch(id) { | ||
/** @type {Map<string, Array<{resolve: (value: any) => void, reject: (error: any) => void}>>} */ | ||
let batching = new Map(); | ||
|
||
return create_remote_function(id, (cache_key, payload) => { | ||
return new Query(cache_key, () => { | ||
if (!started) { | ||
const result = remote_responses[cache_key]; | ||
if (result) { | ||
return result; | ||
} | ||
} | ||
|
||
// Collect all the calls to the same query in the same macrotask, | ||
// then execute them as one backend request. | ||
return new Promise((resolve, reject) => { | ||
// create_remote_function caches identical calls, but in case a refresh to the same query is called multiple times this function | ||
// is invoked multiple times with the same payload, so we need to deduplicate here | ||
const entry = batching.get(payload) ?? []; | ||
entry.push({ resolve, reject }); | ||
batching.set(payload, entry); | ||
|
||
if (batching.size > 1) return; | ||
|
||
// Wait for the next macrotask - don't use microtask as Svelte runtime uses these to collect changes and flush them, | ||
// and flushes could reveal more queries that should be batched. | ||
setTimeout(async () => { | ||
const batched = batching; | ||
batching = new Map(); | ||
|
||
try { | ||
const response = await fetch(`${base}/${app_dir}/remote/${id}`, { | ||
method: 'POST', | ||
body: JSON.stringify({ | ||
payloads: Array.from(batched.keys()) | ||
}), | ||
headers: { | ||
'Content-Type': 'application/json' | ||
} | ||
}); | ||
|
||
if (!response.ok) { | ||
throw new Error('Failed to execute batch query'); | ||
} | ||
|
||
const result = /** @type {RemoteFunctionResponse} */ (await response.json()); | ||
if (result.type === 'error') { | ||
throw new HttpError(result.status ?? 500, result.error); | ||
} | ||
|
||
if (result.type === 'redirect') { | ||
// TODO double-check this | ||
await goto(result.location); | ||
await new Promise((r) => setTimeout(r, 100)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's this about? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copied from the existing query code. Last time I specifically tested this I had to give the async function a bit of time before I rejected it to not cause problems with async Svelte. Not sure if that's still the case hence the TODO There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just tried deleting it, no tests failed. Not sure if that means it's safe to discard but my inclination is to do so, at most we should be using |
||
throw new Redirect(307, result.location); | ||
} | ||
|
||
const results = devalue.parse(result.result, app.decoders); | ||
|
||
// Resolve individual queries | ||
// Maps guarantee insertion order so we can do it like this | ||
let i = 0; | ||
|
||
for (const resolvers of batched.values()) { | ||
for (const { resolve } of resolvers) { | ||
resolve(results[i]); | ||
} | ||
i++; | ||
} | ||
} catch (error) { | ||
// Reject all queries in the batch | ||
for (const resolver of batched.values()) { | ||
for (const { reject } of resolver) { | ||
reject(error); | ||
} | ||
} | ||
} | ||
}, 0); | ||
}); | ||
}); | ||
}); | ||
} | ||
|
||
/** | ||
* @template T | ||
* @implements {Partial<Promise<T>>} | ||
|
Uh oh!
There was an error while loading. Please reload this page.