kit icon indicating copy to clipboard operation
kit copied to clipboard

Clarify conditions `goto` uses to call `load`

Open theetrain opened this issue 2 years ago • 1 comments

Describe the problem

While trying to make use of goto and invalidateAll, I couldn't get my page to call its load function predictably. It turns out, my page's load method had no dependencies that would trigger it to change, even though it's a navigational change with different params.


Details for those curious

My example project

The layout and page files are in this file structure:

  • src/routes/[branch]/+layout.svelte
  • src/routes/[branch]/+layout.server.js
  • src/routes/[branch]/thing/[id]/+page.svelte
  • src/routes/[branch]/thing/[id]/+page.server.js

The broken behaviour:

  • Given the current page is /main/thing/abc123
  • When the user clicks a button in a 'Switch branch' dropdown (kinda like GitHub branches)
  • Then it calls goto('/otherbranch/thing/abc123')
  • And the target [branch] param has changed, and [id] stayed the same.
  • But the target +page.server.js's load method was not called again

In +page.server.js, I have:

/** @type {import('./$types').PageServerLoad} */
export async function load ({ params, locals }) {
  if (!params.id) {
    // No id
    throw error(400, 'No ID provided')
  }

  // get db connection passed in from `src/hooks.server.js@handle`
  // which actually makes use of `params.branch` but isn't explicitly utilized in this `load`
  const { db } = locals

  // ... do db stuff

  return { rows }
}

It calls params.id, but it does not call params.branch, so I assume SvelteKit is aggressively optimizing this load to not invalidate if params.branch ever changes; and I was unaware. I made this adjustment, and now load runs after I call goto('/different-branch-from-before/thing/abc123'):

+ export async function load ({ params, locals, depends }) {
+   depends(params.branch) // this is probably wrong and I'm sorry

I came across this insightful comment: https://github.com/sveltejs/kit/issues/1684#issuecomment-862472061

It describes how goto calls a page (or layout's) respective load under the following conditions:

goto() will only run load() if page.params, page.path, page.query, session or context is used inside of load and one of those values has changed since the last run of the load function.

On the surface this makes sense, the load method needs to have some sort of changed params in order for goto to call it again. However, I could not find this behaviour in the docs, and I still have uncertainties:

  1. Does page.params refer to the $page store used on that load method's respective +page.svelte? Or does it (more likely) refer to event.params used in load?
  2. How do I tell my page and its parent layout to call load when goto navigates to that page?
  3. Is there a way to tell a load to always refetch values if the user navigates to it? I understand it's optimal to make use of depends, but I don't understand its functionality, either.
  4. How do I make use of depends so that if a path param changes, that load should invalidate? I assume it's depends(params.targetParam), but I don't know.

Describe the proposed solution

I'm happy to work with the team to add some details to the docs to better capture the behaviour of goto and when it would or would not call load on the target route, how to assign path params to depends within load, and any applicable side-effects of these features.

Alternatives considered

I guess I could do without the event.locals trickery and be more explicit with my dependencies in load, but I wanted to avoid some boilerplate.

Importance

would make my life easier

Additional Information

This comment was also very informative and parts of it should probably make it to the docs: https://github.com/sveltejs/kit/issues/2560#issuecomment-937389988

Specifically the detail on how hard copies of store values will get properly recognized as changed in SvelteKit's optimization logic.

theetrain avatar Sep 14 '22 03:09 theetrain

The invalidation conditions are explained in https://kit.svelte.dev/docs/load#invalidation. To answer your questions:

  1. They're the same thing
  2. You don't — load will re-run if the conditions are met. If that's insufficient, there are a couple of avenues — we could add an API to load like event.alwaysRun() that makes a load function be considered invalid on every navigation, or we could add an API like goto(url, { invalidateAll: true }). But it would be better to avoid adding surface area if we can
  3. See 2.
  4. depends is very simple — you pass it a URL or custom identifier like app:whatever, and then later call invalidate(url) or invalidate('app:whatever') to cause the load function to re-run. It has no effect on whether a load function is run during navigation. Calling depends(params.branch) is incorrect, because it's equivalent to calling depends('main') which — since it's treated as a URL — is equivalent to depends('/main/thing/main'). But it 'works' because you're accessing params.branch, which is enough to tell SvelteKit 'hey, this load function should re-run when params.branch changes'

Basically we didn't anticipate that people would access route-specific params inside handle, and as such the dependency tracking doesn't apply there. The 'correct' fix would be to access that information inside load functions where the tracking applies. But I get that you want to avoid boilerplate. I guess one thing we could do is apply the dependency tracking to handle and subsequently invalidate every server load function that accesses locals, though that will likely yield lots of false positive invalidations. The less-correct-but-easy fix is basically what you already have but without the depends(...):

/** @type {import('./$types').PageServerLoad} */
export async function load ({ params, locals }) {
  if (!params.id) {
    // No id
    throw error(400, 'No ID provided')
  }

  // get db connection passed in from `src/hooks.server.js@handle`
- // which actually makes use of `params.branch` but isn't explicitly utilized in this `load`
+ params.branch; // signal to SvelteKit that this `load` function should re-run when user switches branch
  const { db } = locals

  // ... do db stuff

  return { rows }
}

Rich-Harris avatar Sep 22 '22 14:09 Rich-Harris

Thanks for the explanation 😃, I have a better grasp of how depends and goto work now. Though I think the lessons here deserve some elaboration on the docs; and I'd like to help if possible.

A couple of suggestions I have for the docs are:

1. I'm guessing this should be invalidateAll() and not invalidate():

https://github.com/sveltejs/kit/blob/fdb166cb80949c2f54093a7787bcd5eb43524666/documentation/docs/05-load.md?plain=1#L345

2. Add a second code snippet to the depends explanation

A page code snippet would help fully illustrate the relationship between depends('my-stuff:foo') in a load, and invalidate('my-stuff:foo') on a page or layout. I agree this statement covers that succinctly:

This function declares that the load function has a dependency on one or more URLs or custom identifiers, which can subsequently be used with invalidate() to cause load to rerun.

But the added code snippet would really help close the loop in my mind as I read through its explanation and see it being applied.


Thanks for lending a hand with my particular SvelteKit dilemma:

The 'correct' fix would be to access that information inside load functions where the tracking applies. But I get that you want to avoid boilerplate.

After thinking it over, I don't think I can avoid boilerplate, nor do I want to anymore; the benefits of connecting to a database in handle doesn't significantly outweigh connecting within load, especially when I look at it side-by-side:

+{page|layout}*.js

+import connect from '$lib/server/db'

export async function load({ locals, params }) {
- depends(params.branch) // should be `params.branch`
- const { db } = locals
+ const db = await connect(params.branch)
}

Writing my code this way:

✅ adds the params.branch dependency implicitly, fixing my goto issue ✅ cleans up handle logic, preventing it from getting stuffy if I ever have to write logic to NOT connect to my database ✅ -2/+2 lines of code, but better

theetrain avatar Sep 25 '22 01:09 theetrain