Skip to content

Avoid running load on the server unnecessarily #6056

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.

Sign up for GitHub

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

Merged
dummdidumm merged 21 commits into from
Aug 23, 2022
Merged

Avoid running load on the server unnecessarily #6056

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
04095cb
failing tests for #5960
Rich-Harris Aug 18, 2022
5bc2f54
merge
Rich-Harris Aug 18, 2022
646a884
explanatory note
Rich-Harris Aug 18, 2022
14e8f64
track dependencies
Rich-Harris Aug 19, 2022
f7e0af6
some progress
Rich-Harris Aug 19, 2022
936d7b3
tidy up tests
Rich-Harris Aug 19, 2022
9c52730
add Uses type
Rich-Harris Aug 19, 2022
472dfbb
some stuff i forgot to commit last night, totally forgot what it does…
Rich-Harris Aug 20, 2022
0d40210
merge master
Rich-Harris Aug 22, 2022
0e691ba
some progress
Rich-Harris Aug 22, 2022
dc4f089
fixes
Rich-Harris Aug 22, 2022
77a89af
types
Rich-Harris Aug 22, 2022
3c52cfb
fix tests
Rich-Harris Aug 22, 2022
a9e5044
gah missed a spot
Rich-Harris Aug 22, 2022
11291a3
more fixes
Rich-Harris Aug 22, 2022
f9d8fbe
complete a TODO
Rich-Harris Aug 22, 2022
b61c63f
small tweaks, reduce code
dummdidumm Aug 23, 2022
44a8299
Remove unnecessary code, comment initial server_data gotcha
dummdidumm Aug 23, 2022
a945a31
comments
dummdidumm Aug 23, 2022
9ed88f7
more comments
dummdidumm Aug 23, 2022
79b2a6a
changeset
dummdidumm Aug 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
243 changes: 156 additions & 87 deletions packages/kit/src/runtime/client/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,57 +468,50 @@ export function create_client({ target, base, trailing_slash }) {
* url: URL;
* params: Record<string, string>;
* routeId: string | null;
* server_data: Record<string, any> | null;
* server_data_node: import('./types').DataNode | null;
* }} options
* @returns {Promise<import('./types').BranchNode>}
*/
async function load_node({ node, parent, url, params, routeId, server_data }) {
async function load_node({ node, parent, url, params, routeId, server_data_node }) {
/** @type {Record<string, any> | null} */
let data = null;

/** @type {import('types').Uses} */
const uses = {
params: new Set(),
url: false,
dependencies: new Set(),
parent: false
params: new Set(),
parent: false,
url: false
};

/** @param {string[]} deps */
function depends(...deps) {
for (const dep of deps) {
const { href } = new URL(dep, url);
uses.dependencies.add(href);
if (node.shared?.load) {
/** @param {string[]} deps */
function depends(...deps) {
for (const dep of deps) {
const { href } = new URL(dep, url);
uses.dependencies.add(href);
}
}
}

/** @type {Record<string, any> | null} */
let data = null;

if (node.server) {
// +page|layout.server.js data means we need to mark this URL as a dependency of itself,
// unless we want to get clever with usage detection on the server, which could
// be returned to the client either as payload or custom headers
uses.dependencies.add(url.href);
uses.url = true;
}

/** @type {Record<string, string>} */
const uses_params = {};
for (const key in params) {
Object.defineProperty(uses_params, key, {
get() {
uses.params.add(key);
return params[key];
},
enumerable: true
});
}
/** @type {Record<string, string>} */
const uses_params = {};
for (const key in params) {
Object.defineProperty(uses_params, key, {
get() {
uses.params.add(key);
return params[key];
},
enumerable: true
});
}

const load_url = new LoadURL(url);
const load_url = new LoadURL(url);

if (node.shared?.load) {
/** @type {import('types').LoadEvent} */
const load_input = {
routeId,
params: uses_params,
data: server_data,
data: server_data_node?.data ?? null,
get url() {
uses.url = true;
return load_url;
Expand Down Expand Up @@ -564,11 +557,9 @@ export function create_client({ target, base, trailing_slash }) {
},
setHeaders: () => {}, // noop
depends,
get parent() {
// uses.parent assignment here, not on method inokation, else we wouldn't notice when someone
// does await parent() inside an if branch which wasn't executed yet.
parent() {
uses.parent = true;
return parent;
return parent();
}
};

Expand Down Expand Up @@ -614,11 +605,63 @@ export function create_client({ target, base, trailing_slash }) {

return {
node,
data: data || server_data,
uses
server: server_data_node
? {
data: server_data_node.data,
uses: {
dependencies: new Set(server_data_node.uses.dependencies ?? []),
params: new Set(server_data_node.uses.params ?? []),
parent: !!server_data_node.uses.parent,
url: !!server_data_node.uses.url
}
}
: null,
shared: node.shared?.load ? { data, uses } : null,
data: data ?? server_data_node?.data ?? null
};
}

/**
* @param {import('types').Uses} uses
* @param {boolean} parent_changed
* @param {{ url: boolean, params: string[] }} changed
*/
function detect_change(changed, parent_changed, uses) {
if (uses.parent && parent_changed) return true;
if (changed.url && uses.url) return true;

for (const param of changed.params) {
if (uses.params.has(param)) return true;
}

for (const dep of uses.dependencies) {
if (invalidated.some((fn) => fn(dep))) return true;
}

return false;
}

/**
* @param {import('types').ServerDataNode | import('types').ServerDataSkippedNode} node
* @param {import('./types').DataNode | null} previous
* @returns {import('./types').DataNode | null}
*/
function create_data_node(node, previous) {
if (node.type === 'data') {
return {
data: node.data,
uses: {
dependencies: new Set(node.uses.dependencies ?? []),
params: new Set(node.uses.params ?? []),
parent: !!node.uses.parent,
url: !!node.uses.url
}
};
}

return previous;
}

/**
* @param {import('./types').NavigationIntent} intent
* @returns {Promise<import('./types').NavigationResult | undefined>}
Expand All @@ -640,52 +683,74 @@ export function create_client({ target, base, trailing_slash }) {
// to act on the failures at this point)
[...errors, ...layouts, leaf].forEach((loader) => loader?.().catch(() => {}));

const nodes = [...layouts, leaf];
const loaders = [...layouts, leaf];

// To avoid waterfalls when someone awaits a parent, compute as much as possible here already
/** @type {boolean[]} */
const nodes_changed_since_last_render = [];
for (let i = 0; i < nodes.length; i++) {
if (!nodes[i]) {
nodes_changed_since_last_render.push(false);
} else {
const previous = current.branch[i];
const changed_since_last_render =
!previous ||
(changed.url && previous.uses.url) ||
changed.params.some((param) => previous.uses.params.has(param)) ||
Array.from(previous.uses.dependencies).some((dep) => invalidated.some((fn) => fn(dep))) ||
(previous.uses.parent && nodes_changed_since_last_render.includes(true));
nodes_changed_since_last_render.push(changed_since_last_render);

let server_parent_changed = false;
let shared_parent_changed = false;

/** @type {Array<{ server: boolean, shared: boolean } | null>} */
const nodes_changed_since_last_render = loaders.map((loader, i) => {
Comment thread
Rich-Harris marked this conversation as resolved.
Outdated
if (!loader) return null;

const previous = current.branch[i];
if (!previous) {
return {
server: (server_parent_changed = true),
shared: (shared_parent_changed = true)
};
}
}

/** @type {import('./types').ServerDataPayload | null} */
let server_data_payload = null;
// TODO if the previous node errored, we might need to always include it?
const { server, shared } = previous;

const result = {
server: server ? detect_change(changed, server_parent_changed, server.uses) : false,
shared: shared ? detect_change(changed, shared_parent_changed, shared.uses) : false
};

if (result.server) server_parent_changed = true;
if (result.shared) shared_parent_changed = true;

return result;
});

console.log(nodes_changed_since_last_render);

/** @type {import('types').ServerData | null} */
let server_data = null;

if (route.uses_server_data) {
try {
const res = await native_fetch(
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`
`${url.pathname}${url.pathname.endsWith('/') ? '' : '/'}__data.json${url.search}`,
{
headers: {
'x-svelte-kit-invalidated': nodes_changed_since_last_render
.map((node) => (node?.server ? '1' : '0'))
.join(',')
}
}
);

server_data_payload = /** @type {import('./types').ServerDataPayload} */ (await res.json());
server_data = /** @type {import('types').ServerData} */ (await res.json());

if (!res.ok) {
throw server_data_payload;
throw server_data;
}
} catch (e) {
throw new Error('TODO render fallback error page');
}

if (server_data_payload.type === 'redirect') {
return server_data_payload;
if (server_data.type === 'redirect') {
return server_data;
}
}

const server_data_nodes = server_data_payload?.nodes;
const server_data_nodes = server_data?.nodes;

const branch_promises = nodes.map(async (loader, i) => {
const branch_promises = loaders.map(async (loader, i) => {
return Promise.resolve().then(async () => {
if (!loader) return;
const node = await loader();
Expand All @@ -696,14 +761,15 @@ export function create_client({ target, base, trailing_slash }) {
nodes_changed_since_last_render[i] || !previous || node !== previous.node;

if (changed_since_last_render) {
const payload = server_data_nodes?.[i];

if (payload?.status) {
throw error(payload.status, payload.message);
}

if (payload?.error) {
throw payload.error;
const server_data_node = server_data_nodes?.[i] ?? null;

if (server_data_node?.type === 'error') {
if (server_data_node.httperror) {
// reconstruct as an HttpError
throw error(server_data_node.httperror.status, server_data_node.httperror.message);
} else {
throw server_data_node.error;
}
}

return await load_node({
Expand All @@ -718,7 +784,9 @@ export function create_client({ target, base, trailing_slash }) {
}
return data;
},
server_data: payload?.data ?? null
server_data_node:
server_data_node &&
create_data_node(server_data_node, previous ? previous.server : null)
});
} else {
return previous;
Expand All @@ -732,8 +800,8 @@ export function create_client({ target, base, trailing_slash }) {
/** @type {Array<import('./types').BranchNode | undefined>} */
const branch = [];

for (let i = 0; i < nodes.length; i += 1) {
if (nodes[i]) {
for (let i = 0; i < loaders.length; i += 1) {
if (loaders[i]) {
try {
branch.push(await branch_promises[i]);
} catch (e) {
Expand All @@ -760,12 +828,8 @@ export function create_client({ target, base, trailing_slash }) {
error_loaded = {
node: await errors[i](),
data: {},
uses: {
params: new Set(),
url: false,
dependencies: new Set(),
parent: false
}
server_uses: null,
shared_uses: null
};

return await get_navigation_result_from_branch({
Expand Down Expand Up @@ -824,7 +888,8 @@ export function create_client({ target, base, trailing_slash }) {
params,
routeId,
parent: () => Promise.resolve({}),
server_data: null // TODO!!!!!
// TODO!!!!! need to load root layout server data
server_data_node: null
});

const root_error = {
Expand Down Expand Up @@ -1230,10 +1295,14 @@ export function create_client({ target, base, trailing_slash }) {
const script = document.querySelector(`script[sveltekit\\:data-type="${type}"]`);
return script?.textContent ? JSON.parse(script.textContent) : fallback;
};
const server_data = parse('server_data', []);
/** @type {import('types').ServerDataNode[]} */
const server_data_nodes = parse('server_data', []);
const validation_errors = parse('validation_errors', undefined);

const branch_promises = node_ids.map(async (n, i) => {
// TODO handle case where this contains an "error" or "httperror"
const server_data_node = server_data_nodes[i];

return load_node({
node: await nodes[n](),
url,
Expand All @@ -1246,7 +1315,7 @@ export function create_client({ target, base, trailing_slash }) {
}
return data;
},
server_data: server_data[i] ?? null
server_data_node
});
});

Expand Down
Loading