inertiajs / inertia

Inertia.js lets you quickly build modern single-page React, Vue and Svelte apps using classic server-side routing and controllers.
https://inertiajs.com
MIT License
6.56k stars 435 forks source link

Deferred svelte component renders the default slot too early #2012

Closed saml-dev closed 1 month ago

saml-dev commented 1 month ago

Version:

Describe the problem:

There is a bug with the Deferred svelte component. It will render the components of the default slot before the props for the Svelte component have been updated. This is because the $page.props store is updated first, or at least a render happens after the store is updated but before the regular export let prop is updated.

Since the Deferred component decides what to render based on the $page store, it renders the default slot too early.

Note that passing a primitive as the deferred prop does not throw an error on the frontend, because it will just render undefined for that single render cycle. But when you pass an object as a deferred prop and try to access its properties, it throws when trying to access a property on undefined.

Steps to reproduce:

I created a repro here: https://github.com/saml-dev/inertia2-repro It includes the .env and database.sqlite to make your lives easier

  1. clone above repo
  2. npm i
  3. composer install
  4. npm run dev (and php artisan serve if not using herd/valet :D )
  5. open the app to / and use the two links to see how one works and one doesn't

Relevant files in the code are:

saml-dev commented 1 month ago

I don't have time tonight to dig into Inertia to see where the bug is, but I'll try to do that tomorrow if I have time!

pedroborges commented 1 month ago

Hey @saml-dev! Thanks a ton for sharing the repro steps—that really helps.

So, what’s going on is a reactivity issue similar to what I tackled in #1969. Back then, the fix needed a small breaking change, and we decided to hold off on that for version 1.3. I kept digging and came up with another solution that fixes the cases I could replicate.

What’s Happening:

I tried delaying the page update using queueMicrotask and it worked. This makes sure the page component and the page store update at the same time. I’m planning to test this more next week to make sure it works smoothly.

If you’re up for testing this out, just replace the node_modules/@inertiajs/svelte/dist/page.js file in your project with the following:

import { derived } from 'svelte/store';
import store from './store';
let initialized = false;
const page = derived(store, ($store, set) => {
    if (initialized) {
        window.queueMicrotask(() => set($store.page));
        return;
    }
    set($store.page);
    initialized = true;
});
export default page;

I think this won't work in SSR, FYI. Let me know if this fixes the issue for you!

saml-dev commented 1 month ago

Ah thanks for the info! That's interesting. I tried the fix and I now have basically the opposite problem. This code throws on the initial render:

let user = $page.props.auth.user;

because $page is undefined :P

pedroborges commented 1 month ago

@saml-dev I had edited page store code above to set the initial value immediately and only delay updates—did you test the updated version? That should take of this problem.

saml-dev commented 1 month ago

Ah I missed that but good idea that's an easy fix. It works! Interestingly, my console log message was called twice in the same render. You can see that in the logs below where theres 4 lines with the same timestamp instead of just 2. I suppose that's because both the $page store and the local prop were registered as listeners so in that one render cycle it saw them both change. Which also technically proves that this change works! 😁

Do you think this is the right fix? It's maybe a bit hacky but does work 🤷🏼‍♂️

1728675928989 'message.title = undefined'
1728675928989 '$page.props.message.title = undefined'
1728675928993 'message.title = undefined'
1728675928993 '$page.props.message.title = undefined'
1728675929087 "message.title = I'm a deferred prop! wow!"
1728675929087 '$page.props.message.title = undefined'
1728675929087 "message.title = I'm a deferred prop! wow!"
1728675929087 "$page.props.message.title = I'm a deferred prop! wow!"
saml-dev commented 1 month ago

To fix SSR, I changed \ if (initialized) {\ to\ if (initialized && typeof window !== "undefined") {

saml-dev commented 1 month ago

I created a PR with this fix 😄

saml-dev commented 1 month ago

@pedroborges It just occurred to me that with all the rendering changes in svelte 5, I should test there. No bug! Even without changing anything else other than the Svelte version and the small change in app.js that is described here.

So I guess whether you merge the PR or not may depend on the level of svelte 4 support you all are going for 🤷🏼‍♂️

saml-dev commented 1 month ago

Closed as this is fixed now. Thanks for your hard work!