-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Remote Functions #13986
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?
Remote Functions #13986
Conversation
* initial commit * various fixes * tweak demo * fix * remove acorn-typescript * simplify * fix * generate, don't transform * use an x- header * regenerate manifest when remote files are added/removed * move RPC logic out of page, it belongs elsewhere * use _app/remote/x * tighten up error handling * unused * CSRF * smaller payloads * belt and braces * don't use 204, that only applies to DELETE/PUT requests * turn remote_call into a factory - more compact and gives us a lot more options * analyze exports of remotes dynamically, add query/action/formAction as different types of remotes * update playground * POC: hydrate query results * remote form actions that hydrate and work without JS * conditional * load fn WIP * rerun query on invalidation * make it possible to call invalidate in rpc functions * fix * adjust form API * fix dev stale bug * let rpc partake in prerendering * prerender POC * cache POC * simplify server remote logic by leveraging ESM self imports * cleanup server remote info * rename functions + some docs * move more stuff into functions to deduplicate/cleanup/make connections clearer * prerendering * hide remote url, avoid unnecessary work * cache refinement * various fixes * tests * don't call prerender function at runtime, tweak tests * tweak * tweak caching behavior * remove cache function from public API for now * add experimental flag * remove load helper function for now * move file * add refreshAll, related to #13139 * adjust overrides signature * query/redirect/form-fail handling * adjust caching behavior to cache until query is removed * disallow non-remote-function exports from .remote files * bump dts-buddy to be able to generate types without bugs again * handle query redirect without going through error boundaries * harmonize refresh with override signature * fixes * resolve cyclic import dependency * prerender treeshaking * refine API * refresh from the server * adjust tests, fix * adjust prerender * reorder functions * make query eager on the client if in reactive context * implement current/error/pending * remove optimistic in favor of override callback * add validate * form.for(...) * tweak api around redirects; allow single flight mutation redirect * implement stacking override API * fixes * deduplicate remote calls on the server during full page visits * rework to use resource class, fix some bugs * ensure refresh/then resolve in order * microtask tweaking * fix * cleanse event for remote functions * implement `updates` and `withOverride` for command * implement `updates` and `withOverride` for form * cleanup * Prevent state_unsafe_mutation error * restrict to 0 or 1 argument, enforce validation * validation tests * tweak * comment out for the time being * remove validate function * lint * guard * bump Svelte version * silence type errors * Update packages/kit/src/runtime/app/server/remote.js Co-authored-by: Paolo Ricciuti <[email protected]> * fix * update playground --------- Co-authored-by: Dominic Gannaway <[email protected]> Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Paolo Ricciuti <[email protected]>
|
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.
it's going to take a few passes to fully review this glorious beast, but here's a few notes to start with. also:
- will need a changeset 😆
- if you have a
.remote.ts
that importsquery
or whatever from$app/server
, then at build time you'll get a 'Cannot import $app/server into client-side code' error rather than the more helpful remote-functions-specific error. Would be nice to be more specific here while not creating a breaking change for anyone that happens to have an existing.remote.ts
file (in which case they would probably appreciate a warning that they'll definitely need to rename that file)
Also, I'll probably find the answer to this as I read on but didn't want to lose the thought: are we (de)serializing arguments to remote functions that are called directly on the server? On the one hand it's wasteful, on the other it would avoid a client/server discrepancy (e.g. a mutation being respected on the server but not on the client)
Absolutely pumped for this to land
void enqueue( | ||
null, | ||
config.paths.base + '/' + config.appDir + '/remote/' + remote_function.__.id | ||
); |
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 this right? the absence of entries
doesn't indicate that there's no argument
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.
My first though was that if you have prerender with an argument but no entries then what are you prerendering? But if it happens as part of a page prerender then that's fine, so you're right, we gotta find a different indicator.
// /prerender/dependencies like indirect calls due to page prerenders? | ||
// Does it really matter? | ||
if (remote_function.__.entries) { | ||
for (const entry of await remote_function.__.entries()) { |
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.
am wondering if entries
is the right terminology to use here — we're using it because that's what it is for pages today, but in this context I wonder if arguments
or something might be clearer
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.
arguments
as not that much clearer to me compared to entries
, and entries
is already for page prerendering, and since it's the same concept it felt better to also use the same name
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 almost think that's more reason to call it something else, as it's helpful to be able to distinguish between them. 'Entries' specifically means entry points for the crawler, which is almost the opposite concept — in one case you're describing the start of the process ('start at blog/one, blog/two and blog/three and see what you can find'), in the other you're describing the end ('the user is going to need this data so I'm just going to tell you about it up-front').
What about data
?
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.
data
is too generic IMO. What about another very generic name: inputs
* @param {string} hashed_id | ||
* @param {string} remote_file_path | ||
*/ | ||
export function enhance_remotes(hashed_id, remote_file_path) { |
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.
could this be turned into a helper function rather than being inlined into every facade module? i.e. something like this:
import * as $$_self_$$ from './${sibling_file_name}';
import $$_enhance_remote_functions_$$ from '__sveltekit/remotes';
$$_enhance_remote_functions_$$($$_self_$$, ${s(hashed_id)}, ${s(remote_file_path)});
export * from './${sibling_file_name}';
dedent` | ||
// Auto-generated part, do not edit | ||
import * as $$_self_$$ from './${path.basename(id)}'; | ||
${enhance_remotes(hashed_id, id)} |
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.
see prior comment about helperifying this
return result; | ||
})(); | ||
|
||
promise.refresh = async () => { |
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.
surely this shouldn't be allowed? the output of a prerender function is immutable
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.
You're right we should treat it as immutable - corresponding code on the client still missing, will implement that soon.
const promise = Promise.resolve(run_remote_function(event, true, arg, validate, fn)); | ||
// @ts-expect-error | ||
promise.updates = () => { | ||
throw new Error('Cannot call `command(...).updates(...)` on the server'); |
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.
would be good if this used the actual name of the command (which could maybe be added to __
?)
Object.defineProperty(wrapper, 'enhance', { | ||
value: () => { | ||
return { action: wrapper.action, method: wrapper.method, onsubmit: wrapper.onsubmit }; | ||
}, | ||
writable: false, | ||
enumerable: false, | ||
configurable: false | ||
}); |
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.
writable
, enumerable
and configurable
all default to false
, so they don't actually need to be specified. I think something like this would be more readable?
Object.defineProperties(wrapper, {
__: {
value: {
type: 'form',
// ...
}
},
enhance: {
value: () => ({ ...wrapper })
},
formAction: {
value: { ... }
},
// ...
});
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.
removal of boolean is a good callout. I did it as separate defineProperty
because TS is smart enough to enhance the type of wrapper
in that case, with defineProperties
it's not sadly
export const handleValidationError = ({ result }) => { | ||
return { message: 'Schema Error', validationErrors: z.treeifyError(result.error)}; | ||
} |
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.
per above, we will need to rethink this 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.
It works, we just need to pass result
instead of result.error
(it will give you a type error I think but it works; will open an issue over at Zod.
export type HandleValidationError< | ||
Result extends { issues: StandardSchemaV1.Issue[] } = { issues: StandardSchemaV1.Issue[] } | ||
> = (input: { result: Result; event: RequestEvent }) => MaybePromise<App.Error>; |
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.
see prior comment — no point nesting issues
inside result
IMHO
export type HandleValidationError< | |
Result extends { issues: StandardSchemaV1.Issue[] } = { issues: StandardSchemaV1.Issue[] } | |
> = (input: { result: Result; event: RequestEvent }) => MaybePromise<App.Error>; | |
export type HandleValidationError< | |
Issue extends StandardSchemaV1.Issue = StandardSchemaV1.Issue | |
> = (input: { issues: Issue[]; event: RequestEvent }) => MaybePromise<App.Error>; |
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.
Theoretically it can be enhanced later by standard schema to have other properties so I'd rather be on the safe side and pass the whole object in.
error( | ||
400, | ||
await remoteInfo.handleValidationError({ | ||
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.
result, | |
issues: result.issues, |
export const handleValidationError = ({ event, result }) => { | ||
return { message: result.issues[0].message }; |
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.
export const handleValidationError = ({ event, result }) => { | |
return { message: result.issues[0].message }; | |
export const handleValidationError = ({ issues }) => { | |
return { message: issues[0].message }; |
const exports = new Map(); | ||
for (const [name, value] of Object.entries(modules)) { | ||
const type = /** @type {import('types').RemoteInfo} */ (value.__)?.type; | ||
if (!type) continue; |
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.
If a .remote.ts
file exports something like export lol = null
then this fails cryptically:
Cannot read properties of null (reading '__')
I don't know if it's possible to restructure things such that we can reuse the logic in enhance_remotes
(IIUC that needs this to run first) — maybe we need to duplicate stuff — but it would be nice to have a consistent 'lol is not a remote function' error
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.
Also, by deferring the check until later, this is the error I see:
Error: Invalid export from remote file /Users/rich/Development/SVELTE/KIT/kit/playgrounds/basic/.svelte-kit/output/server/remote/5mmnej.js: blah is not a remote function. Can only export remote functions from a .remote file
We could use path.relative(cwd, file)
as we do elsewhere and it would become this...
Error: Invalid export from remote file .svelte-kit/output/server/remote/5mmnej.js: blah is not a remote function. Can only export remote functions from a .remote file
...but that still doesn't really help me — what the user needs to see is something like this:
Error:
blah
exported from src/lib/todos.remote.ts is invalid — all exports from this file must be remote functions
// /prerender/dependencies like indirect calls due to page prerenders? | ||
// Does it really matter? | ||
if (remote_function.__.entries) { | ||
for (const entry of await remote_function.__.entries()) { |
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 almost think that's more reason to call it something else, as it's helpful to be able to distinguish between them. 'Entries' specifically means entry points for the crawler, which is almost the opposite concept — in one case you're describing the start of the process ('start at blog/one, blog/two and blog/three and see what you can find'), in the other you're describing the end ('the user is going to need this data so I'm just going to tell you about it up-front').
What about data
?
// TODO this writes to /prerender/pages/... eventually, should it go into | ||
// /prerender/dependencies like indirect calls due to page prerenders? | ||
// Does it really matter? |
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 separation is at best helpful and at worst harmless, though I would opt for prerender/data
Co-authored-by: Rich Harris <[email protected]>
PR for #13897, discussion should happen there.
For a more complete commit history (with lots of explorations and dead ends; also includes some caching ideas) see #13957
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.