What's the point of rerunning the load function on session change?

[csangonzo]

Describe the problem

It's more like a genuine question about a new feature in sveltekit. I just noticed this in the docs:

The load function is reactive, and will re-run when its parameters change, but only if they are used in the function.

After login when I set $session.user = response.user; like in the realworld demo (https://github.com/sveltejs/realworld/blob/master/src/routes/login/index.svelte) I see that the login page's load function runs again.
I use access and refresh tokens in my app and I implemented a silent refresh method, something like the one mentioned here: https://hasura.io/blog/best-practices-of-using-jwt-with-graphql/ .
When I do a silent refresh I reset the session, but this causes a rerun of the load function now.
The problem with this is I have a lot of components that use data returned by the load function (like an editor that gets it's content first from the load function, or a form that has it's model from the load function) and if it runs again the data changes on the page, so if I had unsaved changes in my editor for example and my silent refresh runs, it loads the data again from the server so my changes will be lost.

Describe the proposed solution

If someone could explain the benefits we get from this feature I'd be really glad, because right now this is the first thing that doesn't make any sense about sveltekit for me.

Alternatives considered

No response

Importance

i cannot use SvelteKit without it

Additional Information

I follow the changes of sveltekit from day to day, last year I decided to ditch angular in favour of svelte and migrated almost every app to it that we had and I'm having problems with the authentication part because of this change. I'm open to alternative solutions as well.

[dummdidumm]

This is beneficial because you are then able to refresh your page based on changed parameters. I can see that this behavior is sometimes not beneficial, not sure for the best way to handle this.

Random things I just came up with:

• make the load function return a parameter like (noFurtherUpdates: true) to signal SvelteKit to not refresh again
• another option similar to srr, like rerunLoad, which can be configured globally and be overwritten on a page level (export const rerunLoad = true

Random thoughts on how to work around it right now:

• don't update the session store / keep the token somewhere else
• don't reference session in the pages, if possible (load won't rerun then)
• restructure how page props overwrite unsafed stuff

[csangonzo]

Thanks for the response!
The real problem comes when I validate every page with session's user property, so I check $session.user, if it exists the user can stay on the page, if not then redirect.
I also use my access token in the load function (on the server and on the client as well) and it's in the session's user property. Since load can run on the client and on the server side, it was my only solution to store the token in.
So right now I don't think I can store it anywhere else, and since I check it on every page that requires a logged in user I have to reference it in load.
Restructuring how I handle the page props would be a solution, but there'd still be the exported data that would change to the newly fetched data from the load function, and needless to say it would make an unnecessary request to the server for data that would never be used.

[csangonzo]

Okay, so as I see the load function, it's main functionality is to do/check things before navigating to a specific page. Basically I use it for checking whether the user authenticated or not and if they are load some data associated with the user and the current page.

export async function load({ page, fetch, session }) {
		if (!session?.user) {
			return { status: 302, redirect: '/'};
		}
		const { data } = await api.get(...);
		return { props: { data } };}

This means that the page needs to have an exported value named data.

...
export let data;
...

We want to use this data for something, let's say display it's properties in a form so the user can edit it.
Now I have input fields bound to this data.
If my global store or any other function changes the session, it would be reloaded, the fields the user already edited would be filled with the data from the server, so all unsaved progress would be lost.

This means that I can't change the session's value when the user is using a page.

[csangonzo]

To reproduce it see: https://github.com/csangonzo/sveltekit-refresh-test
Run npm i, then npm run dev and navigate to about.

[mdynnl]

one way to workaround, probably?

export let server_state

// not reactive
let local_state = server_state

// so no cyclic dep
// changes along with local_state
$: server_state = local_state

// and server_state reverts back on load
// while local_state doesn't

[benmccann]

This has always been a controversial feature. I wonder if we could turn it off, but let people re-enable it with invalidate #1709 ?

[dummdidumm]

I'd say a flag with the invalidate logic you mentioned makes sense. Question is whether the default behavior should be on or off, i.e. what's more expected.

[csangonzo]

Question is whether the default behavior should be on or off ...

I can only speak for myself, but I've been using sveltekit since day one and I always looked at load kind of like the part of the page's 'lifecycle', and if I think about it like that, I don't see the benefit of 'rerunning a lifecycle event' by default on a parameter change. But maybe that's just me.

[seanlail]

I vote for opt in. It's only in very rare cases I think I would need load to be reactive.

[seanlail]

Slightly related: Is the Sapper "preload" reactive?

I'm not actually sure now but would this require an update to the migration doc too?

[csangonzo]

Is the Sapper "preload" reactive?

I don't think so, at least the sapper doc doesn't mention anything like that.

[benmccann]

Sapper's preload was also rerun when session changed and I remember people complaining about it there too

[zeckon]

i was thinking about the original post use-case,

what is the expected behaviour if silent-refresh session fails? I think some options are, the page would be no longer valid and it would need to disable input / maybe show a message, or redirect away.

to my understanding, load provides the page state, and if page content is dependent on session, page.params or page.query it seems natural to me to re evaluate load because changing inputs define a new page state. otherwise, setting new query parameters or changing params wouldn't load new data and update the page.

maybe what can be done here is, to implement a logic to keep previous state or user changes so page can recover them on re-load or even not re-fetch/update if a previous state exists

another solution i could come up would be to do silent-refresh when a graphql api call fails auth (token expired). For example urql auth exchange (https://formidable.com/open-source/urql/docs/api/auth-exchange/) has that kind of feature, if a graphql query/mutation fails auth (or about to expire before running the operation), it can run a refresh logic and retry the original operation transparently. this may prevent interrupting user input with a background refresh or doing unexpected loads

[seanlail]

@zeckon agree with your comment but I think it should explicitly be opt-in. There are probably many different use cases for a reactive load (although edge cases in my opinion) but it's currently not obvious and the unsupported (default) case should have it disabled in my opinion.

Perhaps a way to disable it could be an option... but that's still not clear and I think would lead to reloads when a user doesn't want/expect it.

[dummdidumm]

The last two comments make me conflicted on this. I think it's good to rerun load when the page parameters change and I guess quite a few sites rely on this feature. On the other hand, having session reactive seems unintuitive at first. So maybe this option should be more fine grained - an array defining which things should be reactive, with the default being ["page"] .

[seanlail]

The last two comments make me conflicted on this. I think it's good to rerun load when the page parameters change and I guess quite a few sites rely on this feature. On the other hand, having session reactive seems unintuitive at first. So maybe this option should be more fine grained - an array defining which things should be reactive, with the default being ["page"] .

Agree. Defaulting with page makes sense.
Would you export a const or as part of the return?

export a const	part of the return
<script context="module"> export const invalidate = ["page", "session"]; export async function load({ page, session }) { return { status: 200, props: { id: page.params.id }, }; } </script>	<script context="module"> export async function load({ page, session }) { return { invalidate: ["page", "session"], status: 200, props: { id: page.params.id }, }; } </script>

[csangonzo]

I vote for the export const option. Looks cleaner and you don't have to modify your load function's return statement, only add a new line for this option.

[Mlocik97]

I would go for part of return (as it affects load function itself),... and maybe also instead return whole page and session object instead strings, to tell which objects to invalidate, (not just name of object, but it's reference instead).

[rmunn]

I have a design proposal for session persistence and refreshing sketched out in #1726 (comment). Part of that design includes the idea that load() would not re-run whenever you set the session value, e.g. $session.user = loggedInUser would not trigger a re-run of load(). But if you explicitly call $session.refresh(), and your page's load() function uses the value of its session parameter, then it would be re-run with the value that $session.refresh() returns if (and only if) that value was different from what the session store contained before calling $session.refresh(). (If you refresh the session on the client, and the server returns the exact same session data that the client already knew about, then you almost certainly wouldn't want the page reloading).

As far as I can tell from reading the discussion here, most people are surprised by $session.user = loggedInUser causing a page reload, but would be generally okay with the page reloading if its other params change (e.g., /preview?itemID=123 and /preview?itemId=456 are different pages and should cause the load() function to re-run). If so, then I think having session only trigger a reload after an explicit $session.refresh() call would fit your use cases.

For anyone who wants session assignment not to cause load() to re-run, would you be okay with it rerunning after an explicit call to $session.refresh()? Or would that still surprise you?

---

Edit to add: Also, would an automatic session refresh feature (where the user clicking back on the tab automatically triggers a $session.refresh() call) surprise you if that triggered a re-run of load()? The scenario I'm thinking of is this:

• User has a shopping cart that's empty, and tabs A and B open.
• In tab B, user adds something to the shopping cart. Your code calls $session.persist() whenever something is added to the shopping cart, so that their cart contents are stored on the server.
• User clicks back on tab A, which automatically triggers $session.refresh() because the visibilitychange event fired on tab A's window object.
• Because tab A's load() function used the session parameter, and $session.refresh() returned a different value, tab A's load() function re-runs and reloads the page.
  • Note: if tab A didn't use the session when it loaded its data, but some components in tab A's page did (say, a component that displays "0 Item(s) in your shopping cart" in the upper right corner of the page), then only those components would end up re-running their load() functions.

Would that behavior, where tab B calling $session.persist caused a reload on tab A because tab A auto-refreshed the session value from the server, surprise you? Or would that behavior be really useful for your particular use case?

[csangonzo]

Sounds good!

[seanlail]

@rmunn I think in your case the session.refresh feature is great but as far as I can tell it's not really part of this issue? As you said, your feature would assume that the session is not reactive, right?

[Mlocik97]

$ prefix seems like short and simple to write, but I think it can be confusing, at because it can contain both prefixed and unprefixed, it can lead to bugs as someone will forget to add prefix in code, when changing existing code... I would not recommend this. $ symbol is already used in two cases in Svelte, so it would not be good to add anothers, as it would be harder to beginners to understand what it means there, and what there. Not to mentoy, You can use stores in context module script. This would be conflicting, and practically impossible to implement. I agree mostly with:

<script context="module">
  export async function load({ page, session }) {
    return {
      invalidate: { page, session }, // see reasons down.
      status: 200,
      props: { id: page.params.id },
    };
  }
</script>

reasons: array is ordered list, and I would say, order doesn't matter in this case, so object makes it feels more natural. name instead string? auto complete in IDE and IDE understand that page in params of load is connected to page in invalidate property. Performance is not problem, as it's passing reference, not moving whole objects in memory. (it will even takes less memory, as there will be not another array of strings, even tho it's just few bytes)
Why in return, because it gives more power about using sessions and invaliding it (like having if-else before returning, and making it in one case invalidate, in other not). So yes, I think this is really best solution.
Also, it's related to load, so it's good to have it in load... having it as another export feels like it's not related to load, as there are exported other stuff not related to load (like SSR, route, hydration, prerender)

[seanlail]

FWIW, I think this is where the session reactivity is happening in Sapper: https://github.com/sveltejs/sapper/blob/master/runtime/src/app/app.ts#L241
(When the session is "dirty" then it forces the case where the preload is called).

Looks like it was done as part of this commit to support Svelte 3: sveltejs/sapper@ca034d0
Specifically for this ticket: sveltejs/sapper#554

The original thinking seems to be that users could end up in weird states and need the preload to be run because a user logged out. This makes sense of course but we just need to give developers the control to turn this on/off in SvelteKit.

[seanlail]

@Mlocik97 Agree. I've hidden my comment as it's just going to confuse things.

[ramiroaisen]

I vote up for making the reactivity of load optional. It's too much magic sometimes and making it optional gives more control to the user.

[rmunn]

@rmunn I think in your case the session.refresh feature is great but as far as I can tell it's not really part of this issue? As you said, your feature would assume that the session is not reactive, right?

@seanlail - I missed the notification email when you wrote this two weeks ago so I just saw your question. You're right that that's not really part of this issue per se, but it's certainly related. The reason I asked that question here was because I want to get feedback from people who are surprised when session changes cause load() to be re-run. I want to know whether it would also be surprising to have session.refresh() cause load() to be re-run, or whether you (plural you) would expect a session.refresh() call to re-run your load() function. That will help whoever implements this feature in the future (which might end up being me) to write it in such a way as to break as few expectations as possible.

[rmunn]

I vote up for making the reactivity of load optional. It's too much magic sometimes and making it optional gives more control to the user.

What about this half-baked idea? If people like it I'll try to bake it fully and turn it into a feature request. The page parameter to load() is actually an object with special getters so that Svelte-Kit can determine whether the load function uses page.query or page.path, and if you only use path then changes to query don't cause a re-run. What if the session object passed into load() was similarly decorated with getters so that it can tell which parts of the session this load function is using? I.e. if your load function uses session.user then a change to session.darkMode wouldn't cause the load function to re-run, only a change to session.user would trigger a re-run.

Would that be too much "magic" for people? Would you prefer explicitly calling invalidate('session.user') or something? Or would that be just the right amount of "magic" to be useful without being confusing?

[seanlail]

@rmunn If I understand what you're suggesting:

So instead marking one or many of the load store params as "reactive" to rerun load, you instead have more fine-grained ability to say "re-run when this specific nested value of X store changes".

I like it. Although I imagine it will probably be difficult to implement, it may just cover all use cases. 👍

[rmunn]

It would only apply to the special $session store, and would only be possible on the direct properties of the session object. (It would be impossible to know ahead of time that session.foo would be an object, so it would be impossible to set up getters to watch session.foo.bar because session.foo might have been a string or a number). So if your load function accesses session.user.email, a change to $session.user.avatar elsewhere would still end up re-running your load function, because the granularity of load was limited to direct properties. And since something in session.user changed, that means the load function (which as registered a dependency on session.user) would be forced to re-run.

... I think that might be a bit too much magic. Better to have a new return value from load to specify things explicitly. Perhaps something like this:

export async function load({ page, session }) {
  const { user } = session;
  // Do something with session.user, like check access permissions to the page
  return {
    props: { user },
    reloadIfChanged: { session: { user: { permissions: true } } },
  };
}

Then the default would be to not rerun on session changes. If the session.user.permissions value ever changed, the load() function would rerun. But a change to session.user.avatar would not cause a rerun.

Hmm, I can already see a problem with that proposal. What if the username changes, to a different user who happens to have the same set of permissions? Say, Jane Smith logged out and Bob Jones logged in, but both are basic free-tier users so their page permissions are the same. You'd have to remember to write username: true, permissions: true in the reloadIfChanged object, and that's starting to put a bigger burden on you for something Svelte-Kit might have been able to handle by itself.

Not 100% sure what the right answer is here.

[ramiroaisen]

Vote up to make it opt-in with export const invalidate: InvalidateOpts = { session: boolean, page: boolean } or something like that

[LucianVoju]

Hello,

Why not leaving the functionality as it is and use a derived or custom store around session?

REPL ideas: https://svelte.dev/repl/fdbf426e0bd84855b8f88bc6c3b7f488?version=3.45.0

[elliott-with-the-longest-name-on-github]

Just a quick note that the approach here that seems to be most accepted, export const invalidate: InvalidateOpts, has great precedent in the React world -- it's basically useEffect's dependency array -- and load is already a lot like a page-level version of useState and useEffect combined. I personally think it's a great and easy-to-understand solution, and it'd make for a lot cleaner implementations of a couple of things for me.

[Mlocik97]

We are not React here.... I'm against export const invalidate, it also removes a lot of abilities,...

[elliott-with-the-longest-name-on-github]

@Mlocik97

I absolutely agree we're not React here, but I also think it'd be fallacious to say "It's like React, therefore it's bad and we don't want to do it." React does a lot of things well.

Can you clarify what abilities we would lose?

[Mlocik97]

I already explained how invalidate would give better control in load... in case of load, you can do something like:

export async function load({ session, page, fetch }) {
    // some logic
   const shouldSessionInvalidate /* @type boolean */ = ...
    return {
        invalidate: shouldSessionInvalidate ? { session } : {}
    }
}

that's impossible to do with export const invalidate case...

[elliott-with-the-longest-name-on-github]

Absolutely agreed that it goes better in load than as an export. As a sidenote, that's actually more similar to React's useEffect dependency array than the export. My intent behind the original comment was to voice support for "the ability to expose a dependency object to control rerun behavior" than "the specific implementation of exporting that object from the script block", but I didn't make that clear -- my bad on that! 🥲

[pooledge]

If using session precisely as described here for 2 pages, let's say a login and account index page, bound by redirect to their counterpart, I am observeing both of them in DOM for a short amount of time after session has been updated. Disregard this, that here is the way.

I feel like rerun load() on load input change is a good idea overall, it's just that some edge cases like this occur, and suddenly an old good goto() would be way more comfortable. Opting in for an opt-in.

[dummdidumm]

The "$session causes props to be reset"-issue is tracked in #3732, more general discussion about load timing is discussed in #2301. Closing as duplicates of those.