Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Preload re-run upon hydration if server preload returned falsy #843

Open
Conduitry opened this issue Aug 12, 2019 · 2 comments
Open

Preload re-run upon hydration if server preload returned falsy #843

Conduitry opened this issue Aug 12, 2019 · 2 comments
Labels

Comments

@Conduitry
Copy link
Member

Describe the bug
If the server preload returns something falsy, the client-side preload is run upon hydration.

Expected behavior
If the server preload returns something serializable (even if it's falsy), the client-side preload should not be run upon hydration.

I imagine this could be solved by doing a || {} thing earlier in the process, so that the client actually gets a serialized {} in the initial rendered page, and knows not to run preload.

Severity
Probably low. If a preload is returning no data, it's probably not making any wasteful repeated ajax requests on the client.

@Conduitry Conduitry added the bug label Aug 12, 2019
@Conduitry
Copy link
Member Author

To document what happened with this this morning: Here's what I arrived at to not re-run client-side preload when server-side preload returned falsy:

diff --git a/runtime/src/server/middleware/get_page_handler.ts b/runtime/src/server/middleware/get_page_handler.ts
index 53f4136..cb94fac 100644
--- a/runtime/src/server/middleware/get_page_handler.ts
+++ b/runtime/src/server/middleware/get_page_handler.ts
@@ -148,33 +148,33 @@ export function get_page_handler(
 
 		try {
 			const root_preloaded = manifest.root_preload
-				? manifest.root_preload.call(preload_context, {
+				&& (await manifest.root_preload.call(preload_context, {
 					host: req.headers.host,
 					path: req.path,
 					query: req.query,
 					params: {}
-				}, session)
-				: {};
+				}, session))
+				|| {};
 
 			match = error ? null : page.pattern.exec(req.path);
 
 
 			let toPreload = [root_preloaded];
 			if (!is_service_worker_index) {
-				toPreload = toPreload.concat(page.parts.map(part => {
+				toPreload = toPreload.concat(page.parts.map(async part => {
 					if (!part) return null;
 
 					// the deepest level is used below, to initialise the store
 					params = part.params ? part.params(match) : {};
 
 					return part.preload
-						? part.preload.call(preload_context, {
+						&& (await part.preload.call(preload_context, {
 							host: req.headers.host,
 							path: req.path,
 							query: req.query,
 							params
-						}, session)
-						: {};
+						}, session))
+						|| {};
 				}))
 			}
 
@@ -251,7 +251,7 @@ export function get_page_handler(
 
 					props[`level${l++}`] = {
 						component: part.component,
-						props: preloaded[i + 1] || {},
+						props: preloaded[i + 1],
 						segment: segments[i]
 					};
 				}

However this did raise a question of what to do when people are mainly writing a preload for its side effects. This seems to be more likely with layout preloads. Someone could conceivably write a layout preload which checks for something in the session, and either calls this.redirect or returns undefined. Currently this preload would (kind of by accident) be calls on each route change within its purview. With the above changes it would not. Is this a design goal? Do we want to change/fix the behavior only for route-specific preloads?

@cudr
Copy link
Contributor

cudr commented Sep 4, 2019

@Conduitry, My think, this is bug. So we need to fix it

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants