Celestialme / leaderpack

0 stars 0 forks source link

SSR issue #1

Closed notramo closed 1 week ago

notramo commented 1 week ago

Continue the discussion from https://github.com/sveltejs/kit/issues/12941

Could you upload the latest changes so I can review it?

notramo commented 1 week ago

I guess the problem lies in the overuse of stores for stuff it's not meant to do.

I see you have a dynamic route component for the locale src/routes/[language]. Don't use a store in src/locales.ts, but instead make the entire locale an immutable object (Object.freeze()). No need to use a store for that. Change the functions (e.g. about()) to simple objects. You can then import the SvelteKit built-in $page store, and use $page.params.language in the Svelte page. E.g.

<script lang="ts">
  import { locales } from '@src/locales';
  import { page } from '$app/stores';
</script>

<p>
  {locales.ourCustomers[$page.params.language]}
</p>

For restricting the URL to the possible values, you can use param matchers.

Another reason for the SSR error is the contact_el and the products_el bindings. Don't bind elements to stores.

Also, don't use reactive statements ($:), it's deprecated in Svelte 5 because it's causing issues like the one here with SSR. Basically, reactive statements don't run during SSR if I recall correctly, but I'm too lazy to look it up as it don't matter anymore. Use $derived() instead.

Celestialme commented 1 week ago

yes ssr is not reactive. I have added debugging branch. transformed locales and used $page.params.language as language selector but behavior is exactly the same. also its not fault of contact_el or products_el because i have removed them just for checking but no luck. actually if you just open it (initial load from browser url) no flashing or flickering only happens during refresh.

notramo commented 1 week ago

Yes, the localisation is much cleaner this way.

Try removing all of the following code from src/store.ts

import type { Category } from './types';

export let products_el: Writable<HTMLDivElement> = writable();

export let language: Writable<'en' | 'ka'> = writable('en');
export let contact_el: Writable<HTMLDivElement> = writable();
export let header: Writable<HTMLDivElement> = writable();
export let categories: Writable<Category[]> = writable([]);

Any of these can cause issue by itself. Remove all usages of these, and rewrite all code that depend on these.

Then try to simplify reactivity everywhere. For example, in the Categories.svelte component, there is no need to apply $state() to $page.data.categories, as it's reactive by itself.

Remove this, and replace with onMount():

    page.subscribe((page) => {
        if (globalThis.gtag) {
            globalThis.gtag('set', 'page_path', page.url.pathname);
            globalThis.gtag('event', 'page_view');
        }
    });

I recommend running bun x sv migrate svelte-5 to convert the entire project to the Runes syntax, then removing any calls of the run() function, and replacing it with proper Svelte 5 solutions.

Then try running bun run check in the repo to discover any hidden error that remains after the refactor.

Also, see the browser console for any error or warning during hydration. I guess some hydration errors can happen when the Svelte runtime does not know which server-rendered element belongs to which JS-rendered element, and ends up rebuilding the entire page.

I have seen some other bad practices too, that may not cause the error, but worth correcting anyways. Some of them might cause errors. The Biome linter can help you catch many of these code smells. It has LSP integration too.

Celestialme commented 1 week ago

removed language also from store nothing changes. this leads me to nowhere so I will leave it as it is and probably use headless browser for bots since I don't expect much crawling. I will convert code to svelte 5 later but doubt it changes anything for this issue.

notramo commented 1 week ago

Obviously, I expected that removing the language store does not change anything. That's why I suggested a dozen other changes too. The code is full of bad patterns, and the reactivity system is used in ways it's not designed to be used. That's why I suggested a cleanup, because why apply dirty hacks when it can be solved with clean code too? Writing quality code is much simpler than setting up a headless browser for bots. I'm sure we could solve it, and if it's a Svelte bug, submit a bug report upstream (to Svelte, not SvelteKit) but they would say the same: provide a reproducible example with clean code.

Following good practices is also beneficial in many ways. For example, it helps to not leak your API keys, which you have leaked, because did not follow good practices. Put them into .env, and set up trufflehog early, to avoid leaking in Git history. Pairing it with lefthook is an efficient setup. Here's the command to run in lefthook:

trufflehog --no-update --fail --no-verification git file://.

yes i have modified those dozen things. Not cleaning rather straight up removing all the features using them. Removed store and any reactivity but still nothing, it was straight HTML without JavaScript but still same. That's why I don't care enough.

yes keys are in env, this are just experimental keys which are removed already. No leak it was intentional out of laziness. I don't need it i have git guardian which notified me about those keys, but why bother they are for testing.

Celestialme commented 1 week ago

fixed it, problem was very big inline svg which was blocking page render + preloading fonts alleviates FOUC and text flicker. it has nothing to do with reactivity, or stores.