Skip to content

Should parent() call parent load functions by default ? #12654

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

Closed
bleucitron opened this issue Sep 5, 2024 · 12 comments · Fixed by #12655
Closed

Should parent() call parent load functions by default ? #12654

bleucitron opened this issue Sep 5, 2024 · 12 comments · Fixed by #12655
Labels
documentation Improvements or additions to documentation

Comments

@bleucitron
Copy link
Contributor

Describe the bug

I recently realised that parent() did not work the way I thought it did.

My understanding was that await parent() did somehing like this (pseudo code incoming)

const layouts = [parent1.load(), parent2.load(), ...]

function parent() {
  return Promise.all(layouts)
}

but it seems it's more something like

const layouts = [parent1, parent2, ...]

function parent() {
  return Promise.all(layouts.map(layout => layout.load()))
}

This implies that having await parent() in a load function actually re-runs the parents' load functions when accessing the page, even if you're coming from another page (and thus the layout already executed its load). This is actually strange to me, because it could be just providing the previously calculated data without actually rerunning the whole load chain.

Is this a bug or is there a reason for this behavior ?

(if this is normal, the docs might need an update to reference this as a situation that would rerun a load function)

Also, see discussion on Discord

Reproduction

https://stackblitz.com/edit/sveltejs-kit-template-default-wj1pbt?file=src%2Froutes%2Fabout%2F%2Bpage.svelte,src%2Froutes%2Fabout%2F%2Bpage.server.js,src%2Froutes%2F%2Blayout.server.js

Logs

No response

System Info

System:
    OS: Linux 5.0 undefined
    CPU: (3) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
    Memory: 0 Bytes / 0 Bytes
    Shell: 1.0 - /bin/jsh
  Binaries:
    Node: 18.20.3 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 10.2.3 - /usr/local/bin/npm
    pnpm: 8.15.6 - /usr/local/bin/pnpm
  npmPackages:
    @sveltejs/adapter-auto: ^3.0.0 => 3.2.4 
    @sveltejs/kit: ^2.5.26 => 2.5.26 
    @sveltejs/vite-plugin-svelte: ^3.0.0 => 3.1.2 
    svelte: ^4.2.7 => 4.2.19 
    vite: ^5.0.3 => 5.4.3

Severity

annoyance

Additional Information

No response

@dummdidumm
Copy link
Member

This is expected. It is necessary to rerun because how else do you get the data from the upper layout functions? Remember that you can access the result from await parent(). So as soon as you use await parent(), you're declaring a dependency on your upper load functions, and so once the function that uses await parent() needs to rerun, the upper load functions are also rerun.

@dummdidumm dummdidumm added the documentation Improvements or additions to documentation label Sep 5, 2024
@bleucitron
Copy link
Contributor Author

bleucitron commented Sep 5, 2024

That's what i find disturbing.

The router knows nothing related to the layout has changed when navigating to the new page (in the example provided), so why not using the already resolved promise, caching it in some way ? Someone in the Discord discussion mentioned that it would result in a "confusing dependency graph", and i'm not sure why would that be.

@david-plugge
Copy link
Contributor

I agree, i expected it to be the way @bleucitron described. I honestly dont understand why it would have to be the way it works now

@dummdidumm
Copy link
Member

How would the server cache the data, and where? Keep in mind that we also have serverless environments to support

@david-plugge
Copy link
Contributor

david-plugge commented Sep 5, 2024

Simply keeping track of the load functions return value. Like in a map with the routeIds of the load functions as keys and the load result as value.

Edit: btw, i dont know how the structure is handled internally, just throwing out some ideas

@david-plugge
Copy link
Contributor

david-plugge commented Sep 5, 2024

Something like this:

interface Context {
	parent(): Promise<Record<string, any>>;
}

function a(ctx: Context) {
	ctx.parent().then((p) => console.log('a', p));

	return {
		a: 1,
	};
}
async function b(ctx: Context) {
	console.log('b', await ctx.parent());
	return {
		b: 2,
	};
}
function c(ctx: Context) {
	ctx.parent().then((p) => console.log('c', p));
	return {
		c: 3,
	};
}
async function d(ctx: Context) {
	console.log('d', await ctx.parent());
	return {
		d: 4,
	};
}

const chain = [a, b, c, d];

const x = Promise.withResolvers();

const promises = chain.map((fn, i) => {
	return fn({
		async parent(): Promise<Record<string, any>> {
			await x.promise;
			const parents = promises.slice(0, i);

			return Promise.all(parents).then((values) => {
				const result = {};
				for (const v of values) {
					Object.assign(result, v);
				}
				return result;
			});
		},
	});
});

x.resolve();

console.log(await Promise.all(promises));

@bleucitron
Copy link
Contributor Author

bleucitron commented Sep 5, 2024

Does it have to hit the server ? Can't the client router handle this (i.e return the resolved value) when we know the load does not need to be invalidated ?

@bleucitron
Copy link
Contributor Author

bleucitron commented Sep 5, 2024

I just realised that my previous suggestion is absurd.

The parent() call i'm referring to (in a +layout.server load function) is actually only handled in the server, which probably makes it impossible to do, given the serverless constraint.

@bleucitron
Copy link
Contributor Author

bleucitron commented Sep 5, 2024

I guess the only way to do this would be to cache somehow the data client-side, and provide the cached data with each page load request to the server, and resolve this data when calling parent() instead of actually calling parents' load function server-side.

But this is probably stupid too. It would probably require the page load request to POST, plus the amount of data that would travel back and forth would be insane (for each page load request).

@dummdidumm
Copy link
Member

And how would that work in a serverless environment? And how long would it get cached before clearing it? It's safer to just rerun the parent load functions

@david-plugge
Copy link
Contributor

Ahh yes, you are talking about situations when only a subset of the load functions have to rerun right? That does make sense. But in the initial case, does it behave the same? So If there are 3 layouts and one page which all have a load function that calls parent(), would that result in the following pattern:

1
1,2
1,2,3
1,2,3,4

@dummdidumm
Copy link
Member

I'm not sure if you mean "1 reruns four times" by that. If so - no. It only runs once per navigation/invalidation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants