-
-
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
Changes from all commits
4706f36
47e995a
91b94c0
731cbce
e4134ee
75b33a0
b8c0026
902e228
5e48a48
351512c
5e3fc90
729c7a2
73b6f1b
88d4118
fc90222
f25299b
fc2869b
f9fa164
446483b
ddecc8f
9b3ecb6
dab2946
65f9dbc
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 |
---|---|---|
|
@@ -82,7 +82,8 @@ async function analyse({ | |
/** @type {import('types').ServerMetadata} */ | ||
const metadata = { | ||
nodes: [], | ||
routes: new Map() | ||
routes: new Map(), | ||
remotes: new Map() | ||
}; | ||
|
||
const nodes = await Promise.all(manifest._.nodes.map((loader) => loader())); | ||
|
@@ -164,6 +165,18 @@ async function analyse({ | |
}); | ||
} | ||
|
||
// analyse remotes | ||
for (const [hash, load] of Object.entries(manifest._.remotes)) { | ||
const modules = await load(); | ||
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 commentThe reason will be displayed to describe this comment to others. Learn more. If a
I don't know if it's possible to restructure things such that we can reuse the logic in 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. Also, by deferring the check until later, this is the error I see:
We could use
...but that still doesn't really help me — what the user needs to see is something like this:
|
||
exports.set(type, (exports.get(type) ?? []).concat(name)); | ||
} | ||
metadata.remotes.set(hash, exports); | ||
} | ||
|
||
return { metadata, static_exports }; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,7 @@ import { forked } from '../../utils/fork.js'; | |
import * as devalue from 'devalue'; | ||
import { createReadableStream } from '@sveltejs/kit/node'; | ||
import generate_fallback from './fallback.js'; | ||
import { stringify_remote_arg } from '../../runtime/shared.js'; | ||
|
||
export default forked(import.meta.url, prerender); | ||
|
||
|
@@ -186,6 +187,7 @@ async function prerender({ hash, out, manifest_path, metadata, verbose, env }) { | |
} | ||
const seen = new Set(); | ||
const written = new Set(); | ||
const remote_responses = new Map(); | ||
|
||
/** @type {Map<string, Set<string>>} */ | ||
const expected_hashlinks = new Map(); | ||
|
@@ -229,7 +231,8 @@ async function prerender({ hash, out, manifest_path, metadata, verbose, env }) { | |
throw new Error('Cannot read clientAddress during prerendering'); | ||
}, | ||
prerendering: { | ||
dependencies | ||
dependencies, | ||
remote_responses | ||
}, | ||
read: (file) => { | ||
// stuff we just wrote | ||
|
@@ -460,8 +463,25 @@ async function prerender({ hash, out, manifest_path, metadata, verbose, env }) { | |
} | ||
} | ||
|
||
/** @type {Array<Function & { __: import('types').RemoteInfo & { type: 'prerender'}}>} */ | ||
const remote_functions = []; | ||
|
||
for (const remote of Object.values(manifest._.remotes)) { | ||
const functions = Object.values(await remote()).filter( | ||
(value) => | ||
typeof value === 'function' && | ||
/** @type {import('types').RemoteInfo} */ (value.__)?.type === 'prerender' | ||
); | ||
if (functions.length > 0) { | ||
has_prerenderable_routes = true; | ||
remote_functions.push(...functions); | ||
} | ||
} | ||
|
||
if ( | ||
(config.prerender.entries.length === 0 && route_level_entries.length === 0) || | ||
(config.prerender.entries.length === 0 && | ||
route_level_entries.length === 0 && | ||
remote_functions.length === 0) || | ||
!has_prerenderable_routes | ||
) { | ||
return { prerendered, prerender_map }; | ||
|
@@ -499,6 +519,32 @@ async function prerender({ hash, out, manifest_path, metadata, verbose, env }) { | |
} | ||
} | ||
|
||
const transport = (await internal.get_hooks()).transport ?? {}; | ||
for (const remote_function of remote_functions) { | ||
// TODO this writes to /prerender/pages/... eventually, should it go into | ||
// /prerender/dependencies like indirect calls due to page prerenders? | ||
// Does it really matter? | ||
Comment on lines
+524
to
+526
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. The separation is at best helpful and at worst harmless, though I would opt for |
||
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 commentThe reason will be displayed to describe this comment to others. Learn more. am wondering if 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.
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 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 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.
|
||
void enqueue( | ||
null, | ||
config.paths.base + | ||
'/' + | ||
config.appDir + | ||
'/remote/' + | ||
remote_function.__.id + | ||
'/' + | ||
stringify_remote_arg(entry, transport) | ||
); | ||
} | ||
} else { | ||
void enqueue( | ||
null, | ||
config.paths.base + '/' + config.appDir + '/remote/' + remote_function.__.id | ||
); | ||
Comment on lines
+541
to
+544
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. is this right? the absence of 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. 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. |
||
} | ||
} | ||
|
||
await q.done(); | ||
|
||
// handle invalid fragment links | ||
|
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 ofresult.error
(it will give you a type error I think but it works; will open an issue over at Zod.