sveltejs / svelte

Cybernetically enhanced web apps
https://svelte.dev
MIT License
78.73k stars 4.13k forks source link

Svelte 5: Dynamic Components do not update correctly after SSR #13517

Open frschi opened 4 days ago

frschi commented 4 days ago

Describe the bug

Dynamic components in Svelte 5 + Sveltekit do not update correctly after SSR. The state updates, but the rendered component doesn't reflect this change:

store.ts:

import { browser } from "$app/environment";

const someStore = () => {
  let someCondition = $state(false)

  if(browser){
    someCondition = true
  }

  return {
    get someCondition () {
      return someCondition
    }
  }
}

export const store = someStore()

+page.svelte:

<script lang="ts">
    import A from './A.svelte'
    import B from './B.svelte'
    import {store} from './store'

    $inspect(store.someCondition)

    const SelectedComponent = $derived(store.someCondition ? A : B)
</script>

<!-- should display A, but displays B -->
<SelectedComponent />

{store.someCondition}

$inspect() and {store.someCondition} both show true. SelectedComponent should render A, however it renders B.

its kinda like https://github.com/sveltejs/svelte/issues/12333 just for components.

with this workaround in the store.ts it correctly updates to A:

    if (browser) {
        setTimeout(() => {
            someCondition = true;
        }, 0);
    }

Reproduction

in the repl its working correctly. however locally in a fresh sveltekit + svelte 5 project its not.

https://svelte-5-preview.vercel.app/#H4sIAAAAAAAAA4WSwW6DMBBEf2VrRQIkFO4IUkE-IcfSA8LryhWsLdskrRD_XhkISZo0PSEz4-edsQcmZIuWpW8Do7pDlrJCaxYz9639wh6xdchiZlVvGv8ns42R2u0qAgCQnVbGQQHCqA6CbVJs5y3BjV6uevlQH6xTBsfVNS0X5_bTBhXN9o0kq7Fx4WJQHe4VcemkouhsahRZBwdssXHI96rTipAc5LDhaOQR-aPt8AoFpFBGFWXJmrGi7B6UTMLwADJWxGLWKS6FRM5SZ3oc47Xa61iXjj_tdb8VzfN77sH7IYcwgnwHg4_X4ixd5s5hY13tMBR1a3FpQYrQ05WAkySuTvCS5xD0xFFIQh5Ew9zVb5Qf2CvjjDHoekOwmD_uzg6js7Z6bwyzNi7ECYpf040vIZeAa9gwet5g8fxp6pp2RZZM32eY8n9M-SfmffwB1kZLRjYDAAA=

Logs

No response

System Info

System:
    OS: Linux 6.5 Ubuntu 23.10 23.10 (Mantic Minotaur)
    CPU: (24) x64 AMD Ryzen 9 3900X 12-Core Processor
    Memory: 21.84 GB / 31.26 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 20.11.1 - /usr/local/lib/nodejs/node-v20.11.1-linux-x64/bin/node
    Yarn: 1.22.22 - /usr/local/lib/nodejs/node-v20.11.1-linux-x64/bin/yarn
    npm: 10.2.4 - /usr/local/lib/nodejs/node-v20.11.1-linux-x64/bin/npm
    bun: 1.1.30 - ~/.bun/bin/bun
  Browsers:
    Brave Browser: 129.1.70.123
    Chrome: 129.0.6668.89
  npmPackages:
    svelte: ^5.0.0-next.1 => 5.0.0-next.262

Severity

annoyance

PatrickG commented 4 days ago

Minimal reproduction

paoloricciuti commented 4 days ago

Hydration does happen, (if you try to add a button with a different onclick on the client and on the server the client one will be executed) but the text doesn't change. I think i have an idea as to why but i think this kinda falls in line with {@html} discourse.

However since components changing from client to server can be more usual maybe we want to fix this?

dummdidumm commented 3 days ago

If this doesn't add mountains of complexity I think we should handle this

paoloricciuti commented 3 days ago

If this doesn't add mountains of complexity I think we should handle this

I think too tbh

jsudelko commented 1 day ago

Snippets too:

<script lang="ts">
    import { browser } from '$app/environment';
</script>

{#snippet client()}client{/snippet}
{#snippet server()}server{/snippet}

<p>String: {browser ? 'client' : 'server'}</p>
<p>Html: {@html browser ? 'client' : 'server'}</p>
<p>Snippet: {@render (browser ? client : server)()}</p>
trueadm commented 1 day ago

I honestly don't think we should do this – and I have a ton of experience in this field having seen how first-hand this mismatch hell caused all sorts of problems for real-world users of Facebook.

This does add a significant amount of complexity, and more importantly it actually negatively affects the user experience of your users. If the SSR content is renderer and is significantly different from the client code, then people can interact with content that isn't actually meant to be there. This can impact interactions, accessibility, scroll position, text input and just make for a terrible experience. I mean, swapping out content during hydration for screen reader users is terrible – it literally breaks the two biggest screenreaders – NVDA and JAWS. NVDA gets stuck and you then have to abort content flow mode, and JAWS simply intercepts the old content as being combined with the new content if you were already focused in that area.

Furthermore, this will have an impact on your performance metrics when it comes to SEO – hydration is meant to be a quick process. It's intent is to hook up your SSR content with dynamic bindings from the client – such as event handlers and state. If the initial hydration process has lots of additional overhead, then the time-to-first-interaction will be significantly impacted. This is why other frameworks have moved away from trying to patch up things during hydration and now warn/error instead – the impact shown in research on Lighthouse scores and SEO actually have an impact, especially on mobile.

We do some work todo to patch up simple mismatches, such as text strings, which can be a common thing because of timestamps. We also do some work around some logic blocks that kind of fall into a grey area, however, they're an exception and I hope in the future we move away from doing these things in logic blocks too, or at least provide warnings when encountering such cases.

Either way, I've moving this to 5.x as this is a lot of work to do for something that we probably shouldn't be promoting to begin with.

paoloricciuti commented 1 day ago

I honestly don't think we should do this – and I have a ton of experience in this field having seen how first-hand this mismatch hell caused all sorts of problems for real-world users of Facebook.

This does add a significant amount of complexity, and more importantly it actually negatively affects the user experience of your users. If the SSR content is renderer and is significantly different from the client code, then people can interact with content that isn't actually meant to be there. This can impact interactions, accessibility, scroll position, text input and just make for a terrible experience. I mean, swapping out content during hydration for screen reader users is terrible – it literally breaks the two biggest screenreaders – NVDA and JAWS. NVDA gets stuck and you then have to abort content flow mode, and JAWS simply intercepts the old content as being combined with the new content if you were already focused in that area.

Furthermore, this will have an impact on your performance metrics when it comes to SEO – hydration is meant to be a quick process. It's intent is to hook up your SSR content with dynamic bindings from the client – such as event handlers and state. If the initial hydration process has lots of additional overhead, then the time-to-first-interaction will be significantly impacted. This is why other frameworks have moved away from trying to patch up things during hydration and now warn/error instead – the impact shown in research on Lighthouse scores and SEO actually have an impact, especially on mobile.

We do some work todo to patch up simple mismatches, such as text strings, which can be a common thing because of timestamps. We also do some work around some logic blocks that kind of fall into a grey area, however, they're an exception and I hope in the future we move away from doing these things in logic blocks too, or at least provide warnings when encountering such cases.

Either way, I've moving this to 5.x as this is a lot of work to do for something that we probably shouldn't be promoting to begin with.

This is not exactly the same thing but switching native controls for more advanced JS drived controls is key for progressive enhancement and i think we should support such cases. The UX can be made good even when there's a switch especially if the alternative is to render something that is unusable.

paoloricciuti commented 18 hours ago

We started kinda looking into this and i think @trueadm is actually right...considering that the static node will literally be placed in the template right now we will basically need to find a way to change that and probably heavily change hydration. Considering that this behaviour is achievable with

<script lang="ts">
    import Client from './Client.svelte';
    import Server from './Server.svelte';
    import { browser } from '$app/environment';

    let Component = $state(Server);

    $effect(()=>{
        Component = Client;
    })
</script>

<p>String: {browser ? 'client' : 'server'}</p>
<p>Html: {@html browser ? 'client' : 'server'}</p>
<p>Component: <Component /></p>

it's probably not worth the time.

However: can we easily add a warning for this?