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

Setup each page - init hook, export const blocking, etc. #6183

Open
stalkerg opened this issue Aug 23, 2022 · 34 comments
Open

Setup each page - init hook, export const blocking, etc. #6183

stalkerg opened this issue Aug 23, 2022 · 34 comments
Labels
feature request New feature or request router
Milestone

Comments

@stalkerg
Copy link
Contributor

stalkerg commented Aug 23, 2022

Describe the problem

For i18n or similar subsystems, we should make initialization before any language things are needed, and for such initialization, we usually use a session (user language) and information from the HTTP request.
Before the new routing system was introduced, we were using the root __layout.svelte to do it, it's working well for SSR and CSR time.
I should mention that what i18n should be inited on the server side and init on the client side as well (or state should be send as is).

After migration to new routing, we lose one "transitional" place to init such thing because +layout.js can be called after +page.js, and you must do await parent() in each page to avoid such race condition.

For server-side only init, we have a good place it's handle in hooks.js with event.locals it's an excellent way to init server only things like DB connection, but for things what should be a transition from SSR to CSR we have no place.

I also have a similar issue for i18n implementation project what I use cibernox/svelte-intl-precompile#55

Describe the proposed solution

Maybe it's possible to do it more straightforwardly, but at least we need a global hook for what will be called on SSR and on CSR, after +layout.server.js but before +layout.js and +page.js.

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

@dummdidumm
Copy link
Member

If I understand you correctly, it could work today for you, too, but you'd have to do await parent() in each layout/page, which is cumbersome. I had this concern, too. My idea was to introduce export const blocks = <boolean> to +layout.js (and possibly to layout.server.js) so you have to write it in one place only for situations like this.

@dummdidumm dummdidumm added feature request New feature or request router labels Aug 23, 2022
@stalkerg
Copy link
Contributor Author

stalkerg commented Aug 23, 2022

@dummdidumm right, but honestly, I suppose +layout.js it's not a good place, and for me, it just should be before +layout.js and +page.js (doesn't care who first) it's not directly tied with the layout. Also, because we can have a named layout, I don't want to duplicate this logic of how I should do it now.

@elliott-with-the-longest-name-on-github

This comment was marked as off-topic.

@stalkerg

This comment was marked as off-topic.

@elliott-with-the-longest-name-on-github

This comment was marked as off-topic.

@stalkerg

This comment was marked as off-topic.

@elliott-with-the-longest-name-on-github

This comment was marked as off-topic.

@stalkerg

This comment was marked as off-topic.

@benmccann benmccann changed the title Transitional init hook Setup each page - init hook, export const blocking, etc. Aug 24, 2022
@benmccann benmccann added this to the 1.0 milestone Aug 24, 2022
@cibernox
Copy link

cibernox commented Aug 24, 2022

I'm going to drop here some background on prior art on how other frameworks, in this case Ember.js, approached this problem of having code that needs to run before the continues the boot process and that code may or may not be blocking.

In Ember such code was put in /initializers and /instance-initializers. The functions there ran before the app booted and were typically used to setup critical initial configuration (i18n is an obvious example of setup that should run as early as possible because it will affect how everything else is rendered. Configuring user credentials if there's some kind of API wrapper singleton could be another one).
If those functions returned a promise, the app boot's process would block until that promise resolves. If no promise is resolved the boot process continues without delay.

The shade of meaning between initializers and instance-initializers was that initializers ran once in the entire lifetime of the app, even in SSR, while instance-initializers ran once in the browser but once per request in SSR.

Initializers also had names and accepted before/after: otherInitializersName in case there were many and the execution order was important, much like rollup plugins.

I just mention this because IMO Ember got this reasonably right. Sveltekit may decide to come up with a different abstraction, but it's good to know how others approached similar problems in the past.

@benmccann
Copy link
Member

export const blocking would work - though it'd also mean that you need an extra route group in most cases. E.g. a layout that just does the i18n setup discussed here in a blocking fashion, then a layout that loads the user in a parallel fashion and displays the layout UI. I'm not sure how annoying this would be in practice, but probably at least some

@Rich-Harris Rich-Harris modified the milestones: 1.0, post-1.0 Aug 27, 2022
@dummdidumm
Copy link
Member

Another thing coming up from a discussion: When doing this in the root layout and the setup depends on things that are causing the load function to rerun, the setup code might be rerun when you don't want that. #6294 could also help with that.

@sphinxc0re
Copy link
Contributor

sphinxc0re commented Sep 12, 2022

Having a hook like hooks/initialize.server.js and hooks/initialize.client.js would be extremely useful for injecting code to run "on boot". Many ORMs like TypeORM and MicroORM depend on you being able to run code on startup and those hooks would be a great way of adding this in a non-breaking way.

EDIT: Ruby on Rails even has a whole initializers folder containing many files that run on boot.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@sphinxc0re

No need for this. Back a while ago, we had #6179. This causes hooks.{js|ts} to be run whenever the app "starts" (this is a bit of an amorphous term when considering serverless stuff, but basically it's guaranteed to be run prior to responding to any requests). This means that any initialization logic can be run in the root of hooks and it'll run on startup.

@cibernox
Copy link

@tcc-sejohnson that is very interesting. Do you know if this is intended behaviour and expected to stay or it just happens to be that way because of an implementation detail?

If that's guaranteed I'll go ahead and update the documentation on my library, but I wouldn't like the rug to be pulled from under me.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@cibernox No it's intended -- I purposefully made that PR because of the same use-cases as @sphinxc0re. It was created in response to #6147.

@cibernox
Copy link

@tcc-sejohnson then you made my day! Thanks for the heads up!

@sphinxc0re
Copy link
Contributor

So this should be documented then, right?

@elliott-with-the-longest-name-on-github
Copy link
Contributor

So this should be documented then, right?

Hah, that would close my issue from a looong time ago: #1753

@stalkerg
Copy link
Contributor Author

@tcc-sejohnson thanks! I will try it, at least it sounds good.

@stalkerg
Copy link
Contributor Author

stalkerg commented Sep 14, 2022

@tcc-sejohnson, unfortunately, using hooks.client.js is not working for me because I need access to request (/page/?lang=en ) to get the language GET argument and init i18n. Yes, as you said, here #6756 (comment), it's only for the server. Basically, it's already described in the issue.
@cibernox it's not helping us :(

@sphinxc0re
Copy link
Contributor

@stalkerg there is a section about i18n in the docs: https://kit.svelte.dev/docs/accessibility#the-lang-attribute 🤷🏼‍♂️ maybe it helps

@dominikg
Copy link
Member

Here's what i've been experimenting with today:

lang.ts

import type { Handle, RequestEvent } from '@sveltejs/kit';
import {
	init,
	register,
	waitLocale,
	getLocaleFromAcceptLanguageHeader,
	getLocaleFromNavigator
} from 'svelte-intl-precompile';
register('en', () => import('$locales/en'));
register('de', () => import('$locales/de'));

const DEFAULT_LOCALE = 'en';

// add this hook to your hooks.server.ts sequence 
// and update app.html to use `<html lang="%lang%">`
export const setLocale: Handle = async ({ event, resolve }) => {
	const lang = await loadLocale(event);
	return resolve(event, {
		transformPageChunk: ({ html }) => html.replace('%lang%', lang)
	});
};

// call this function with await in hooks.client.ts
export async function loadLocale(event?) {
	let locale = event ? getSSRLocale(event) : getClientLocale();
	init({ initialLocale: locale, fallbackLocale: DEFAULT_LOCALE });
	await waitLocale(locale);
	return locale;
}

function getSSRLocale(event: RequestEvent) {
        // prefer stored user locale, fall back to accept header and default
	return (
		event.locals.user?.locale ||
		getLocaleFromAcceptLanguageHeader(event.request.headers.get('Accept-Language')) ||
		DEFAULT_LOCALE
	);
}

function getClientLocale() {
	// html lang attr is set by SSR hook, so we just reuse that
	// otherwise fall back to navigator or default
	return document?.documentElement.lang || getLocaleFromNavigator() || DEFAULT_LOCALE;
}

hooks.client.ts

import { loadLocale } from './lang';
await loadLocale();

hooks.server.ts

import { setLocale } from './lang';
export const handle: Handle = sequence(/*auth etc */, setLocale);

Is this a valid approach?

@dominikg
Copy link
Member

unfortunate side effect: you have to update your build target to es2022 for support of TLA

@PatrickG
Copy link
Member

Good solution @dominikg

unfortunate side effect: you have to update your build target to es2022 for support of TLA

If you don't want to use es2022, you could do this

hooks.client.ts

import { loadLocale } from './lang';
await loadLocale();

in the root +layout.js like this:

import { browser } from '$app/environment';
import { loadLocale } from '../lang';

export const load = async () => {
  if (browser) {
    await loadLocale();
  }
};

This should work.

@stalkerg
Copy link
Contributor Author

@PatrickG, as I mentioned in the issue, it will work only if you add await parent() for each page load function.

@stalkerg
Copy link
Contributor Author

stalkerg commented Sep 18, 2022

@dominikg thank you for the solution. But sorry, I disagree with @PatrickG ; it's a very complicated and sphagety-like solution. In the SveltKit we mustn't do it.

@PatrickG
Copy link
Member

PatrickG commented Sep 18, 2022

@PatrickG, as I mentioned in the issue, it will work only if you add await parent() for each page load function.

I was only talking about the solution @dominikg provided.

Edit: nevermind, I forgot about that

After migration to new routing, we lose one "transitional" place to init such thing because +layout.js can be called after +page.js, and you must do await parent() in each page to avoid such race condition.

@stalkerg
Copy link
Contributor Author

@sphinxc0re thanks for the advice, it does not solve this issue, but help to more accurate handle language even now.

@PatrickG
Copy link
Member

My proposal would be an exported init or initialize (can't think of a better name) function in hooks.client.js that gets called (and awaited) right after the init function in

Maybe the returned value could be the equivalent of locals in the client-side load functions or merged with data.

@ZetiMente
Copy link

@dominikg That is beautiful code. Thanks for providing that ! Going to try that out.

@stalkerg
Copy link
Contributor Author

it's still very actual, and even more, because load is async between load function execution and starting template rendering can be executed many other load functions from different requests that overwrite global stores and provide race-condition.
Basically, we have no current way to implement i18n per request on SSR.
We are also very limited in how we can pass down parameters to the template in the per-request model.

@elliott-with-the-longest-name-on-github
Copy link
Contributor

@Rich-Harris Seems like we should close this issue and break it out into multiple parts. Some pieces of this have been fixed (i.e. "global config" can run in the hooks.{server|client}.js and will be called at startup rather than on first request). It'd probably be good to generate more-specific requirements as well.

@dominikg
Copy link
Member

dominikg commented Dec 28, 2022

the TLA solution i drafted above for async client init doesn't really work great with bundling, so one of the requirements would be an async client init hook.

edit: this repo showcases it (still on sveltekit beta bit iirc nothing significant has changed related to this so it still holds true).

@stalkerg
Copy link
Contributor Author

@tcc-sejohnson my first message is very specific and still not solved. Maybe we must create a new task issues bounded to this one. IMHO this issue as feature request is good.

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

No branches or pull requests

10 participants