Skip to content
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

Rethink or clarify when load runs #2301

Closed
benmccann opened this issue Aug 26, 2021 · 15 comments · Fixed by #6299
Closed

Rethink or clarify when load runs #2301

benmccann opened this issue Aug 26, 2021 · 15 comments · Fixed by #6299
Labels
breaking change feature / enhancement New feature or request load / layout p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Milestone

Comments

@benmccann
Copy link
Member

benmccann commented Aug 26, 2021

Describe the bug

The docs say:

A component that defines a page or a layout can export a load function that runs before the component is created.

But I don't think that is when load runs? I think load is reactive to its input parameters and runs everytime they change

This is confusing to people. E.g. they don't expect it to run when $session changes #2252.

Also, sometimes load in your layout will run whenever the path changes and sometimes it will load only when the page first loads, which is super hard to discover. This also leads to weird workarounds like accessing $page.path to cause load to run eventhough you don't need to do anything with it (#1165 (comment))

This all feels a little too magical to me. It's not very clear that it will happen (vs if I did something like $: load = function(page) {}. Maybe this is just how Svelte works, but it's not obvious to me. The Svelte docs don't have any stores as function parameters, and I never did something like $page.subscribe(load).

We also have page accessible from two places: the load parameters and a store. Is there a reason we have two ways of doing the same thing? Does load only re-run when we access page via one of those locations or via both?

We've also gotten a request to not have load run on every URL change (#2673)

At the very least we need to update the docs for load, but I wonder if we can't make things more explicit.

Part of the difficulty here is that components aren't recreated for each page view and so sometimes you get a new page component created and sometimes you don't. Given that, we sort of have to rerun load when $page changes. This is somewhat related to the transition bugs that we have. The reason we don't create new components for each page view is so that when viewing multiple pages rendered by the same svelte file you can have transitions - though I'm not sure that we couldn't accomplish that other ways

This also feels slightly related to the before / after hooks for routing (#552) because some people try to put code that will run on every page navigation on load in __layout.svelte, but you might also be able to accomplish some of those use cases with routing hooks.

Reproduction

n/a

Logs

No response

System Info

n/a

Severity

serious, but I can work around it

Additional Information

No response

@rmunn
Copy link
Contributor

rmunn commented Sep 6, 2021

I have a design for $session.refresh() (and also $session.persist) sketched out in #1726 (comment). Part of that design includes the idea of only re-running load on session changes if $session.refresh() was called, whether explicitly or implicitly. (Implicit calls to $session.refresh() would be triggerd by things like the user coming back to a tab after clicking away from that tab to a different one). Also note that load would only rerun if $session.refresh() returned a session value different from what it used to contain.

Would a design where load() only re-runs after a $session.refresh() call, but not after assigning to the session store on the client side, be a useful design? The other cases where load() would re-run (changes to page params, etc.) all make sense to me, it's only the reload-on-session-change behavior that seems to surprise people.

@benmccann
Copy link
Member Author

benmccann commented Sep 19, 2021

We just got another complaint about the existing behavior in #2455

I figured out where this is happening and under what conditions while looking at that issue:

const changed_since_last_render =

@Mlocik97
Copy link
Contributor

Mlocik97 commented Oct 13, 2021

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

in another thread, I think, we ended with this proposed API for setting what to invalidate... this gives really a lot of abilities to devs.

@dummdidumm
Copy link
Member

This is an interesting idea. The contract could be "the objects in invalidate need to implement some certain API, which page/session etc already implement, and then that stuff is just watched and load is retriggered whenever one of the objects update".

@GCastilho
Copy link

I just like to suggest a way to prevent load from running whatsoever, I think these suggested methods can also be used to implement such functionality

My use for this is: I have a global store that is initialized with data returned from the load function. Currently the store is resetted once the user navigates back to the page but I'm planning to implement a logic to just ignore the 'init' if the store has data

In either case I think that it's not that far of an idea to "prevent when load re-runs while on the same page" to "prevent load from running while navigating back to the page"

@jthegedus
Copy link
Contributor

At the very least we need to update the docs for load, but I wonder if we can't make things more explicit.

I believe this is where Next.js Data Fetching shines. It is explicit to users and the functionality is distinctly separate across the APIs. I realise load is not quite the same as this API in Next.js, but the clarity of the API is clear during discussion amongst relatively junior/new devs in the space. I've been using Kit for quite a while now and still get confused about it's capabilities and find it an extremely difficult area to discuss with people, even those familiar with Kit.

Has the idea of splitting load into a number of different, explicitly named functions been entertained?

@helmturner
Copy link

helmturner commented Nov 3, 2021

Chiming in from the 'new devs' perspective
(js is my first lang, picked up TFM beginning of 2021)

Splitting Load seems to make a lot of sense to me. Typescript was a huge factor in cutting my learning curve across the board, and hobbling Load into a monolith lacks the clarity of a well defined set of interfaces. Just looking at the docs, it's the 2nd largest section - outdone only by Configs! Even the Routing and Modules section are shorter.

I fully support splitting session per #1726 (comment). Perhaps a page function could act as the namespace for the navigation lifecycle functions (I'm intentionally not calling them 'hooks' for reasons already mentioned (maybe 'triggers'?) Following from this theme, the stuff function could encapsulate layout inheritance solutions a-la #1538 #1165 #2169 #1530 #2436.

That is to say, since For the root __layout.svelte component, it is equal to {}
Perhaps a /__stuff file could construct an initial object, instantiate any middleware, and even perhaps cover the edge case mentioned in #2436 of persisting components past layout resets, as the semantics of the __stuff file could extend that of the __layout api to allow configurable immutability.

I really hate the term 'stuff' for this though - something like 'mids' or 'fixtures' sounds more descriptive to me.
EDIT: If 'stuff' isn't already a widely used and understood term, and we're creating one... I'm proposing fixins.
My rationale:

(let me know if I'm misunderstanding anything, it's rather la-- early, on this side of the pond)

mquandalle added a commit to mquandalle/mesaidesvelo that referenced this issue Nov 21, 2021
Non activé car cela pose un problème bizarre avec la première animation.
Sans doute lié à sveltejs/kit#2301
@nonphoto
Copy link

nonphoto commented Dec 8, 2021

I am very new to Kit so I might be in over my head, but what if load only ran once on the client and the arguments to it implemented the store contract? Of course you wouldn't be able to use the $ syntax for reactivity in the module context, but this way users can explicitly subscribe to changes for only the arguments they need. load could even return "observables" for status, props, etc.

This feels more idiomatic to me because it mirrors how reactivity is handled in the component script, just without the syntax sugar. With the current approach (where load re-runs on argument changes) it feels closer to React function components without the hooks.

@Mlocik97
Copy link
Contributor

Mlocik97 commented Dec 9, 2021

load must run on server, if You want prerendering/SSR. The issue is not about if it runs on client or server, but about when it reruns, aka if it reruns when $session change. It makes sense to let user define when to rerun, and there is already suggestions about it, in other thread.

@nonphoto
Copy link

nonphoto commented Dec 9, 2021

I was suggesting that it still runs on both the server and the client, but only once on the client. Reactivity would then be provided by explicitly subscribing. I don't think this would affect SSR at all.

@GCastilho
Copy link

@nonphoto Yes, that's one of the ideas. There is more discussion about this topic on #2252

@bleucitron
Copy link
Contributor

bleucitron commented Dec 15, 2021

Also, something weird to me: load reruns when a page.query change, even if the specific query params is not referenced.

For instance, suppose i have 2 query params: a and b.

This load will run again if i change the a query but not the b.

export function load({ page }) {
	console.log('load', page.query.get('b'));
	return {};
}

I find this pretty annoying since it is counter intuitive, especially given that regular page.params reactivity work differently.

@harrylaulau
Copy link

I think that how load seems puzzling is when it is used with hooks and you need to consider how to do ssr, pre-rendering (or SSG), protected routes (authenticating user on every request implemented by maybe hooks?), and caching.

I am not a very experienced developer that I just transitioned from plain HTML js CSS, then react, nextjs, and now sveltkit.
While finding it really powerful and actually easier to use with the binds and stores, the most complicated part becomes figuring out how data is transferred or fetched as mentioned in the first paragraph.

nextjs provide simple functions and middleware implementations so you can be very clear of where that code is run (on client-side, on request, or during build time). While sveltekit have access to a lot of environment variable, I found that harder to maintain as while nextjs you can split concern into different functions or parts of code, in sveltekit you are doing conditionals in a big load function. I think it would be easier if we split server-side and client-side code into two functions just like nextjs, and use conditional in the server-side to distinguish build time and SSR (or maybe split again). Such that the load function in the client is really running before component rendering and only run once before creation or upon parameter changes (I am also actually not 100% sure what this suppose to mean, or it is a client-side hook somehow?)

I also wonder by if(!browser) does the code inside the if block also got included in the client-side code? (So increasing bundle size and maybe unsafe as others can access your code? I am not really sure about this for my knowledge XD)

Another issue I met (though I think this might only be caused by my lack of knowledge and should solve it easily maybe when I move to that point in my project?) is about caching. From my understanding traditionally headers are the only way to specify caching and normally you specify the proxy level(CDN) and client level (public and private caching), with optional maxage properties. But what modern cloud computing complicates things is that caching can be done in a whole lot more ways, and not only the "traditional ways" of caching. In an abstract way of thinking, normally we may cache info upon SSR endpoints and in the browser. When mixed with authentication, we might want to cache also protected content, or even protected user content on the CDN that requires authentication once people retrieve those content (just read some docs so may not get the full picture, but seems these all complications comes with Vercel middleware or even more freedom with amazon Cloudfront@functions and lambda@edge and other similar functions in other cloud platforms? like Alibaba Cloud. edge routines etc). Is that possible for sveltkit to also implement caching functions and then the adapters could choose how they would implement it? (I know it could also be done by those platform/adapters providing SDKs or APIs but it would be so advanced for svelte/kit maybe if it is a pluggable module maybe?)

Lastly, which I think might be a bit off-topic but rather more personal is that I would like to ask how you guys refactor code? I used components and tailwind to abstract out designs and a lot of binds to allow functions to deep dive into the fundamental components, but I still find a page with 375 lines is a bit too long to read and maintain. I searched online (not much content seemed to focus on refactoring frontend component frameworks), and I thought of splitting a page into different sections then including them back. Yet I am not sure if that's the right way to do it as that would create a new folder may be for each page, then also requires a store for state variables that are shared across the page(or maybe not just by binding starting from the page -> section -> fundamental components)

That's what I want to share and sorry that's so long lol. Thank you guys if you finished reading it!

@benmccann benmccann added the feature / enhancement New feature or request label Mar 17, 2022
@elliott-with-the-longest-name-on-github
Copy link
Contributor

There seem to be a couple of threads all talking about the same issue, but this seems to be the most active / best fleshed out -- I left this comment on the other one as well, but wanted to make sure it was documented in both places:

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.

edit: The "return an invalidate object from the load function" is a better example than the export example and much closer to the API that I'd like to use.

@benmccann benmccann added p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc. breaking change labels Jul 29, 2022
@rmunn
Copy link
Contributor

rmunn commented Aug 5, 2022

With the upcoming changes to load discussed in #5748, particularly the way that it now returns just the data that the page will use, rather than a complicated object, some of the proposals so far won't work in the new world. Specifically, the "return an invalidate object from the load function" proposal, while an excellent idea, is no longer viable.

What is probably most viable is to use the new depends function (which replaces the dependencies array). It may be possible to change the semantics of load so that it does not re-run on a session change by default, but only if you ask for it to re-run on certain session properties, e.g. by calling depends('$session.username'). The depends function normally expects a URI, so there would need to be a specific well-defined prefix string for specifying the session. $session might work, but that would need to be bikeshedded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change feature / enhancement New feature or request load / layout p1-important SvelteKit cannot be used by a large number of people, basic functionality is missing, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.