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

Fix preserve state and page store in Svelte adapter #522

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

reinink
Copy link
Member

@reinink reinink commented Feb 26, 2021

Fixes #476

This PR fixes two pretty big issues with the Svelte adapter:

Preserving state

The first issue is that the preserveState option was never implemented in the Svelte adapter, which ultimately meant that preserving state was always enabled. This is quite problematic, since it can cause stale local component data when visiting another page that uses the same page component. For example, when navigating from one edit user page to another.

This has been resolved by adding a component key to the page component when rendering it. This is exactly the same way that the Vue and React adapters work. However, adding the {#key} block was tricky, since the page and layout components were being dynamically rendered via the Render component. Which brings us to the next issue...

Page store

The second issue was related to accessing the global derived page store ($page) in non-page components, such as layouts. For some reason the store was updating prior to non-page components being unmounted, which was causing errors in those components if they depended on the data in that store. For example, in a layout component that expects an authenticated user ($page.auth.user), when you logout, the $page.auth was being set to null before the layout was being unmounted, causing an error. The workaround here was to always conditionally check for this data in those components, but that's annoying to have to do.

I was not able to determine exactly why this was happening, but I think it had to do with how we were using the <svelte:self> component within the Render component. This was a very clever hack by @pedroborges, which allowed us to have unlimited nested persistent layouts. However, I think this somehow messed with Svelte's rendering, causing these issues with the $page store.

To solve this I took a simpler approach, and just manually rendered the page and persistent layouts. This fixes the $page store issues, and also allowed me to add a component key to the page component. The tradeoff here is that you can no longer have unlimited nested persistent layouts. You can now only have five. I think this is an non-issue, since I'd be highly surprised if anyone ever had more than 2-3 nested layouts.

@ghost
Copy link

ghost commented Feb 26, 2021

This approach actually looks simpler to me. If I may however add one thing, the HTML with the {#if} chain could be replaced with the following code:

{#if $store.component}
  {#each $store.layout as layout, index}
    <svelte:component this={layout}>
      {#if index === $store.layout.length - 1}
        {#key $store.key}
          <svelte:component
            this={$store.component.default}
            {...$store.page.props}
          />
        {/key}
      {/if}
    </svelte:component>
  {/each}
{/if}

This does exactly the same thing. It just iterates over the layout array and with index === $store.layout.length - 1 we can check when we are at the end and lastly render the page. This would also allow unlimited layouts again.

@reinink
Copy link
Member Author

reinink commented Feb 26, 2021

@jordankaerim Are you sure this will nest the layouts within each other? I would think this would render them as siblings, not children.

Edit: Yeah, just tested this, and the layouts are rendered as siblings, not children.

@ghost
Copy link

ghost commented Feb 26, 2021

@reinink Oh... you're right. I totally missed that 🤦

@reinink reinink merged commit 590c932 into master Feb 26, 2021
@reinink reinink deleted the svelte-fix-preserve-state-and-page-store branch February 26, 2021 18:47
@mariojankovic
Copy link
Contributor

I imagine we'd be able to rewrite this into a recursive component instead of the if's. I'll test this out hopefully soon and do another PR.

@reinink
Copy link
Member Author

reinink commented Feb 27, 2021

@mariojankovic Feel free to give it a try! That's really what we had already (see the removed Render.svelte file in this PR), but it was causing the "Page store" issue above.

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.

Redirect broken if new page's shared data is missing objects from previous page
2 participants