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

Avoid running load on the server unnecessarily #6056

Merged
merged 21 commits into from
Aug 23, 2022
Merged

Avoid running load on the server unnecessarily #6056

merged 21 commits into from
Aug 23, 2022

Conversation

Rich-Harris
Copy link
Member

@Rich-Harris Rich-Harris commented Aug 18, 2022

#5960. Just a failing test for now (have to run, will implement later if time allows).

Basic strategy:

  • Track, in load_server_data, what gets used (url, params.foo, parent)
  • Send that information to the client alongside data
  • On navigation, when requesting __data.json, specify (probably in a custom header) which nodes have been invalidated (i.e. if a node uses url, it will always be invalid if the URL has changed; if it uses params.foo, it is only invalid if params.foo has changed; if it uses parent then it will be invalid if any parent is invalid)
  • On the server, only run the invalid nodes, return a value like -1 for everything that remains valid. If an invalid node unexpectedly calls await parent, run everything upstream as well
  • On the server, fill in the -1 values from memory

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Aug 18, 2022

🦋 Changeset detected

Latest commit: 79b2a6a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Rich-Harris
Copy link
Member Author

Obviously there are other things that could invalidate a load function — event.locals might have changed, or it might construct parameters from request.url for some reason. For now I'm thinking of those as edge cases that can be handled with invalidate(), though we might need to refine that in future.

@Conduitry
Copy link
Member

If there are no +layout.server.js/+page.server.js loads that have been invalidated (i.e., only +layout.js/+page.js loads have been invalidated), I'm hoping it's going to be straightforward to not make a __data.json call at all? It feels like it should be. The client should already also have the information it needs to make that decision it seems like.

@Rich-Harris
Copy link
Member Author

I hadn't considered that but yes, it should definitely work that way. (We already skip requesting server data if we know a given route doesn't use it)

@dummdidumm
Copy link
Member

Should we err on the side of caution/correctness and always rerun in certain cases? You might scratch your head why something doesn't update when you run into the locals situation for example.

@Conduitry
Copy link
Member

I'm more likely I think to be annoyed if it is re-run when I don't want it to than I am to be annoyed that it isn't re-run when I want it to. The latter leads to obvious bugs, while the former leads to inefficiencies that might not even be obvious during local development because network requests are so fast. It's also a lot clearer how to tell the load function "okay, manually invalidate your value now" than it is to tell it "okay, don't manually invalidate your value at some point in the near future when you're going to otherwise want to".

In my app, I'm currently planning on using event.locals as a way to hide some information from Kit that I need in +layout.server.js. I want it to request different data depending on whether it's in two rather different sections of the app (and I'm consciously opting in to being very careful about links between these two sections), and I don't want +layout.server.js to be re-run on every navigation just because it checked url to see what part of the app it was in. My solution had been to put a flag on event.locals that says where in the app this request is, and then directly expose that in +layout.server.js. I wasn't crazy about it, but I didn't see another obvious way to deal with this.

Just like in Svelte's reactivity where we give ways to access variables without Svelte seeing it, there should also be an escape hatch in SvelteKit for "I'm accessing this value in my load, but don't automatically call load again when it changes, I know what I'm doing". Maybe that way is via load, or maybe it's via some other mechanism.

@dummdidumm
Copy link
Member

dummdidumm commented Aug 19, 2022

These are good points. I also just realized that the locals behavior was the same previously, because the page didn't know if locals was used on the server request. The give a way to opt out of reruns also sounds related to #5850, sometimes the opposite of depends could be valuable.

@sserdyuk
Copy link

sserdyuk commented Aug 19, 2022

I am glad to see this is already on the radar. I'd like to mention our use case to help shape future changes. We're building a multi-tenant Node app. The app loads tenant's information (skins, for example) based on the hostname in the request. We used to do this with a handle and getSession combination in hooks.ts. This option is no longer available. Now, we have to use handle paired with +layout.server.ts to convert locals to data to make it accessible. It works, but the problem is that both handle and load in the +layout.server.ts fire on each client-side navigation via a __data.json call. Since the hostname never changes, it is unneccessary.

/// hooks.ts 
import type { Handle } from '@sveltejs/kit';

export const handle: Handle = async ({ event, resolve }) => {
	console.log('Called handle');
	// In real life we're doing something like this:
	// event.locals.config = (await configByHostname(event.url.hostname)) as ApiParams;
	event.locals.tenant = event.url.hostname;
	return await resolve(event);
};

/// +layout.server.ts
import type { ServerLoad } from '@sveltejs/kit';

export const load: ServerLoad = ({ locals }): { tenant: string } => {
	console.log('Called layout server load');
	return { tenant: locals.tenant };
};

@gterras
Copy link

gterras commented Aug 19, 2022

It feels like the number of unique cases of "who reloads what when except if" is not fully graspable by the average mind, just like with reactivity except reactivity is mostly hidden to users but load functions are not. The solution should be explainable in a very straightforward way (almost as a precept) to avoid mob incomprehension.

The "intelligent reload" is definitely a killer feature of sveltekit but it is possible I'm biased since all my cases have always fitted with its magic. The three levels of management "simple/advanced/iknowwhatimdoing" approach feels great for tools that solve infinite cases problems, being :

  • simple: any prop you use trigger reload
  • advanced: you can play with invalidate/depends
  • iknowwhatimdoing: you can break the rules

On a semi-serious note it would be a nice case for some documentation-driven-development!

…, but need to switch branches so im gonna commit it now with an undisciplined commit message
@AlbertMarashi
Copy link

I'm creating a PWA and running into this issue. How can I prevent SvelteKit from running layout.server.ts when the browser is offline?

@dummdidumm
Copy link
Member

Shouldn't the PWA take care of that transparently? The service worker intercepts the request and serves something from cache instead.

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did some small tweaks, overall looks good 👍

@rohanrajpal
Copy link

rohanrajpal commented Aug 24, 2022

Hey, have run into a small issue after this PR

.
├── +layout.server.ts
├── +layout.svelte
├── +page.svelte
├── login
│   ├── +layout.svelte
│   ├── +page.svelte
│   └── select-profile
│       └── +page.svelte
└── register
    ├── +layout.svelte
    ├── info
    │   └── +page.svelte
    └── password
        ├── +page.svelte
        └── page.server.ts

In the layout.server.ts I have a logic that checks if auth is done and fetches relevant info of the user, which is used in select-profile & register/info

Before this PR, layout.server.ts was triggered on every navigation & I had the latest data shown on each page, but that is not the case now.

I was wondering if there is an explicit way to call load in +page.server.ts? Or is there a better way to do what I'm doing?

I've tried the following but they dont work

// select-profile/+page.svelte

invalidate();
goto('/auth/login/select-profile');

The above does refresh the load in +layout.server.ts, but select-profile still uses the old data until I refresh the page

@dummdidumm
Copy link
Member

When is invalidate(); goto(..); called? Is it in response to some action (i.e. inside a function that is called after the user clicks a button)?

@rohanrajpal
Copy link

rohanrajpal commented Aug 24, 2022

Hey, just realized that invalidate is working as expected, the culprit here is my silly code.

//+layout.server.ts

import { get } from '$lib/api';
import type { OnboardingInfo } from '$lib/types/onboarding.type';
import { redirectIfNotAlreadyThere } from '$lib/utils';
import { redirect } from '@sveltejs/kit';
import type { LayoutServerLoad } from './$types';

export const load: LayoutServerLoad = async ({ locals, url }) => {
    if (locals.bearerToken) {
        const response: OnboardingInfo = await get(`users/onboardingInfo`, locals.bearerToken);

        if (response.error) return { bearerToken: locals.bearerToken };
        else if (response.authStatus === 'COMPLETE') throw redirect(302, '/app');
        else if (response.authStatus === 'LOGIN')
            redirectIfNotAlreadyThere('/auth/login', url.pathname);
        else if (response.authStatus === 'CREATE_PASSWORD')
            redirectIfNotAlreadyThere('/auth/register/password', url.pathname);
        else if (response.authStatus === 'ENTER_INFO')
            redirectIfNotAlreadyThere('/auth/register/info', url.pathname);
        else if (response.authStatus === 'SELECT_PROFILE')
            redirectIfNotAlreadyThere('/auth/login/select-profile', url.pathname);

        return {
            bearerToken: locals.bearerToken,
            user: response,
        };
    } else {
        redirectIfNotAlreadyThere('/auth/login', url.pathname);
    }
};

export function redirectIfNotAlreadyThere(to: string, pathname: string) {
    if (to !== pathname) throw redirect(302, to);
}

Thing is that I use my +layout.server.ts as a router of sorts, depending on the authStatus of user. Now the issue is that invalidate() was called in the login page, so according to my logic, instead of returning the new params, it throws a redirect & uses the old params since nothing new was returned.

Is this way of routing in a parent layout file recommended in general?

When is invalidate(); goto(..); called? Is it in response to some action (i.e. inside a function that is called after the user clicks a button)?

Yes it is called when the user clicks the login button

Login function
async function submit() {
        isLoading = true;
        const response: { authStatus: AuthStatus } | { error: string; message: string } =
            await post(`auth/login`, {
                email,
                password,
                userProfileId: data.user?.userProfile.at(0)?.id,
            });

        await invalidate();

        // TODO handle network errors
        if ('error' in response) errors = [response.message];
        isLoading = false;

        if ('authStatus' in response) {
            if (response.authStatus === 'COMPLETE') {
                setBearerForApi(data.bearerToken);
                instagramConfig.fetch();
                storeConfig.sync(false);
                goto('/app/dashboard');
            } else if (response.authStatus === 'ENTER_INFO') {
                goto('/auth/register/info');
            } else if (response.authStatus === 'SELECT_PROFILE') {
                goto('/auth/login/select-profile');
            }
        }
    }

@kristjanmar
Copy link

It looks like +layout.server.ts is still being called every time if the page inside the layout has a catch-all route.

I have:
/stocks/[symbol]/+layout.server.ts
/stocks/[symbol]/+layout.svelte
/stocks/[symbol]/financials/[...routes]/+page.server.ts
/stocks/[symbol]/financials/[...routes]/+page.svelte

Every time I client-side navigate to /financials/ or a subpage like /financials/balance-sheet/ or /financials/balance-sheet/quarterly/, it calls the load function in the +layout.server.ts.

I tried to create a reproduction in StackBlitz but it's just throwing an error "node.component is not a function".

@dummdidumm
Copy link
Member

How does the +page.server.js look like? Does it await the parent and does it have something in it that makes at least that page run every time?

@ollema
Copy link

ollema commented Aug 28, 2022

maybe related: #6349

@kristjanmar
Copy link

How does the +page.server.js look like? Does it await the parent and does it have something in it that makes at least that page run every time?

The +page.server.ts is supposed to run every time when the route changes. For example, going from /stocks/aapl/financials/ to /stocks/aapl/financials/balance-sheet/. But I would prefer to re-use the response from the parent +layout.server.ts.

The +page.server.ts does have an await parent() inside because I need to access info from the parent layout, which has information about the stock.

I can see now that another page that is not a catch-all route but has an await parent() also calls the load function in the +layout.server.ts file every time. So I guess this does not happen in catch-all routes specifically, rather just any page that has an await parent().

Is the await parent() supposed to make the load function run again? In my case it is redundant because I am navigating from a page that already caused the layout load function to run and the data is the same. But if this is expected behavior then I can try to find a way to work around it.

Here is the code in the catch-all route's +page.server.ts:

export const load: PageServerLoad = async ({ params, parent }) => {
	/**
	 * Get the correct statement and range combination from the URL path
	 */
	let symbol = params.symbol
	let routes = params.routes.split('/')
	let first = routes[0] ? routes[0].toLowerCase() : null
	let second = routes[1] ? routes[1].toLowerCase() : null

	let statement: Statement =
		first && first !== 'quarterly' && first !== 'trailing' ? (first as Statement) : 'income-statement'
	let range: Range = second ? (second as Range) : first === 'quarterly' || first === 'trailing' ? first : 'annual'

	if (
		!['income-statement', 'balance-sheet', 'cash-flow-statement', 'ratios'].includes(statement) ||
		!['annual', 'quarterly', 'trailing'].includes(range)
	) {
		throw error(404, 'not found')
	}

	let host = import.meta.env.VITE_PUBLIC_API_URL
	let url = `${host}/financials?type=${statement}&symbol=${symbol}&range=${range}`

	const req = await fetch(url)
	const data: PageDataProps | NotFoundResponse | RedirectResponse = await req.json()
	const { info }: { info: Info } = await parent()

	if (data) {
		if (data.status === 301) throw redirect(301, data.destination)
		if (data.status === 404) throw error(404, 'not found')

		let { heading, title, description, url, map } = getFinancialsProps(statement, symbol, info.name)

		return {
			statement,
			range,
			heading,
			title,
			description,
			url,
			map,
			financialData: data.data.data,
			fullCount: data.data.fullCount
		}
	}

	throw error(404, 'not found')
}

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

Successfully merging this pull request may close these issues.

10 participants