sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.45k stars 1.89k forks source link

+layout.server.ts load re-runs on every navigation #5960

Closed CaptainCodeman closed 2 years ago

CaptainCodeman commented 2 years ago

Describe the bug

Layout load function runs on every navigation

Reproduction

Log to console when layout load function is executed and it happens on every navigation.

import type { LayoutServerLoad } from './$types'

export const load: LayoutServerLoad = async ({ }) => {
  console.log('load /layout')

  return {
    name: 'My Site'
  }
}

Logs

No response

System Info

npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.64 
    @sveltejs/kit: next => 1.0.0-next.415 
    svelte: ^3.44.0 => 3.49.0 
    vite: ^3.0.0 => 3.0.8

Severity

blocking an upgrade

Additional Information

No response

Rich-Harris commented 2 years ago

Yep, reusing values is a TODO. We need to track whether load functions use url and params, just like they do on the client (in theory request and locals being different could also affect the output, but since that's always true I think we probably have to rely on explicit invalidation otherwise there's no point).

The hard part will be figuring out what to do if a +page.server.js or intermediate +layout.server.js calls await parent(), since the server, being stateless, won't have the parent values. There's basically two options:

  1. re-run the load functions if a downstream load callsawait parent(), even if it would otherwise not have been run
  2. POST the parent data to the server in cases where await parent() is called, to simulate statefulness on the server
gterras commented 2 years ago

even if it would otherwise not have been run

This would still be a huge improvement compared to the situation now, which is layout.server.js just being a small boilerplate reducing helper.

Rich-Harris commented 2 years ago

An argument against posting data back to the server is that we may want to introduce the ability to send non-JSONable values from the server:

// +layout.server.js
export function load() {
  const map = new Map();
  map.set(map, map);

  return {
    map
  };
}

It's likely that we'd achieve that with https://github.com/rich-harris/devalue, which is only safe to use when sending data from the server to the client, not the other way around.

CaptainCodeman commented 2 years ago

This vaguely reminds me of Cache Aware Server Push which uses a Bloom filter in a cookie to keep track of what the client already does or doesn't have to communicate to the server what needs to be sent. See also this Go implementation.

I don't know enough about SK internals to know if that would be applicable or not. It maybe wouldn't have to be a cookie (forcing use a cookie may be a bit imposing) but could be something the client passes to communicate what it does or doesn't already have, so it can merge data on the client after a partial fetch.

gterras commented 2 years ago

we may want to introduce the ability to send non-JSONable values

I understand the concern and I'm probably lacking many knowledge to make a fair point but the two use cases the docs quote for using server.js rarely involve non-JSON values :

By the way "fetch data from an external API" could be another use case (which also can do pretty well already with JSON).

I still think it would be a huge win to be able to use layout.server.js like it seems it can be used at first glance. The introduction of non-JSONable values afterwards should not break anything anyway.

Rich-Harris commented 2 years ago

This Remix issue asking for loaders to use superjson has 37 👍 votes — it's definitely not an unheard of requirement to include things like Date objects etc.

Another case that JSON can't handle is this, if the referential equality matters (which it would in a <select bind:value> for example):

export function load() {
  const options = [
    { flavour: 'chocolate' },
    { flavour: 'strawberry' },
    { flavour: 'banana' }
  ];

  const selected = options[0];

  return { options, selected };
}

We don't plan on using superjson, because devalue is faster, produces smaller output, and doesn't force you to add a not-that-tiny client to deserialize on the client. But it does mean the data can only safely go one way. (Though we decided that sending data back to the server to simulate statefulness is a terrible idea anyway.)

(Re cache aware server push — the logic is on the wrong side. The server doesn't need to know what the client has, because the client is going to tell it. All the server has to do is tell the client 'this blob of data will be invalidated if params.foo changes' or whatever.)