oscarhermoso commented 2 weeks ago

Context

Hi again. I am using sveltekit-sse from a layout load() function so that the store is accessible across my whole site via $page.data, but have run into issues after implementing the recommended reconnect code:

https://github.com/tncrazvan/sveltekit-sse?tab=readme-ov-file#reconnect

Steps to reproduce

Initialize SSE store in +layout.ts (see below code)
Post SvelteKit form action with default invalidateAll: true, returning redirect() method
Attempt to reconnect with basic close({ connect }) from docs

export const load = async ({ parent, data }) => {
    let tutor_queue;

    if (data.session?.user.id) {
        tutor_queue = source(
            `/queue?status=waiting`,
            {
                close({ connect }) {
                    console.log('reconnecting...');
                    connect();
                },
            },
        )
            .select('new_lesson')
            .json<TutorQueue>(({ error, raw, previous }) => {
                console.error(`Could not parse "${raw}" as json.`, error);
                return previous;
            });
    } else {
        tutor_queue = readable(null);
    }

    return {
        ...(await parent()),
        ...data,
        tutor_queue,
    };
};

Expected

sveltekit-sse json store reconnects without issue

Actual

sveltekit-sse stuck in reconnection loop, increasing at exponential rate

Screenshot from 2024-04-16 16-45-13

oscarhermoso commented 2 weeks ago

Also FYI - this JSDoc is out of date:

https://github.com/tncrazvan/sveltekit-sse/blob/98f0f8b7fce8122e4ae648e3995444eb2adbc0f3/src/lib/types.js#L24-L32

oscarhermoso commented 2 weeks ago

Created a repo to reproduce the issue -

https://github.com/oscarhermoso/sveltekit-sse/tree/reconnect-bug

Steps to reproduce

bun dev

Open dev tools
Click "Force reconnect" a few times
Watch console logs and network packets increase

tncrazvan commented 2 weeks ago

Hello @oscarhermoso , I just published version 0.8.17, please update to get the fix.

TL;DR

Use const instead of $: to assign your stores.

<script>
  import { enhance } from '$app/forms';
  export let data;
  const quote = data?.quote;  // <===== Here, note the "const" instead of "$:"
</script>

<h3>A Loaded Cat Quote</h3>
<span>{$quote?.value ?? ''}</span><br />

<form use:enhance method="post">
  <button>Force reconnect</button>
</form>

You can find a repo with a working example here https://github.com/tncrazvan/sveltekit-sse-issue-33

Notes

A few notes on this though, as this issue was partly caused by svelte's reactivity system.

As mentioned in the home documentation: connections are cached.

This is so that you can safely connect to the same source in different components without having to worry about reaching the browser parallel requests limit.

When you invoke source().select() and subsequent transformers like .json(), you're given a Readable<T> store.
When you submit that form you showcased in your code and reproduction above, sveltekit triggers the reactive statement.
When you're given the new store, the old one is dealt with asynchronously by svelte, meaning it unsubscribes from that store, as it should, that's what the $ syntax does when you use it in your markup.
When the store you're given has 0 subscribers and there are no other stores connected to the same source endpoint, then the global connection of that source is shutdown.\ The connection will reopen when the store is requested by at least 1 subscriber.

The problem here is the reactive statement When this recalculates a new store is given to you, but the old one still exists, and has 0 subscribers, which means the connection will attempt to shutdown.

While the new store is just connecting to the source, the old one is trying to kill the connection.\ So with this 0.8.17 update, neither the old store nor the new store are properly connected to the source after this interaction.

I think it's worth keeping this behavior, otherwise you would have to clean your connections manually by adding code like this to every component that's using an sse source.

onDestroy(()=>connection.close())

The solution is to use an actual constant to get your store instead of a dynamic statement. So something like this

<script>
  import { enhance } from '$app/forms';
  export let data;
  const quote = data?.quote;
</script>

<h3>A Loaded Cat Quote</h3>
<span>{$quote?.value ?? ''}</span><br />

<form use:enhance method="post">
  <button>Force reconnect</button>
</form>

Let me know if this fixes your problem.

oscarhermoso commented 1 week ago

Hi @tncrazvan, thankyou for your detailed reply.

I have updated to 0.8.17 and tried the fix as you suggested.

Unfortunately, const causes a different set of issues - for our use case, we need to reactively update the store data when the page URL changes. To give some more context, see example code:

Consider a dashboard for a team, with each team member having a queue of tasks. You can select a different team member from a drop-down menu, this will update the page's URL search parameters, to load different data.

<!-- layout.svelte -->
<script>
  import { goto } from '$app/navigation';
  import { page } from '$app/stores';
</script>

<select
  on:change={(user_id) => {
    const query = new URLSearchParams($page.url.searchParams.toString())
    query.set('user', user_id.toString())
    goto(`?${query.toString()}`)
  }
>
    <option value={1}>Alice</option>
    <option value={2}>Bob</option>
    <option value={3}>Charlie</option>
</select>

We want the dashboard to refresh in real time, so we also source() our data using sveltekit-sse

// +page.ts
export async function load({ url }) {
  const searchParams = new URLSearchParams(url.search);

  const tasks = source(`/tasks?${searchParams.toString()}`)
    .select('tasks')
    .json<Tasks>(({ error, raw, previous }) => {
      console.error(`Could not parse "${raw}" as json.`, error);
      return previous;
    });

  return { tasks };
}

Unfortunately, this means that the fix as you've described won't work.

<!-- page.svelte -->
<script>
  export let data;

  // const tasks = data.tasks;  // data won't refresh when the use dropdown changes if this is const
  $: tasks = data.tasks;
</script>

{#each $tasks as task}
  <!-- card that describes task details -->
{/each}

Possible solution?

Instead of forcing the store to be a const, is it possible for sveltekit-sse to be improved so that the automatic unsubscribe in Svelte files can clean up connections?

https://svelte.dev/docs/svelte-components#script-4-prefix-stores-with-$-to-access-their-values

I have also read through your read the source code - ~~I am fairly confident that instead of having a map of readables, you can have a single readable... similarly, it might be possible to make .json a derived store instead of a new readable.~~

https://github.com/tncrazvan/sveltekit-sse/blob/main/src/lib/source.js

I might be oversimplifying the situation (EDIT: I'm definitely oversimplifying), but if it is possible, the end result would be less moving parts - you should not need to maintain the reference.connectionsCounter any more.

oscarhermoso commented 1 week ago

I have just updated this GitHub working example to hopefully clarify the issue

https://github.com/oscarhermoso/sveltekit-sse/tree/reconnect-bug

tncrazvan commented 1 week ago

Instead of forcing the store to be a const, is it possible for sveltekit-sse to be improved so that the automatic unsubscribe in Svelte files can clean up connections?

That would be ideal, ofc.

a derived store instead of a new readable.

There are several issues with that approach, one of them being that svelte's default store behavior is to omit the change event of a store if the new value is identical to the previous one. It's not verbose.

But we need to be able to receive the same message multiple times, because it's not just a matter of just updating the user interface at this point, some use cases actually need to be able to receive the same message multiple times, for example ping events or timeout mechanisms.

you should not need to maintain the reference.connectionsCounter any more.

Currently I'm using reference.connectionsCounter to make sure I'm not closing the stream while someone is listening to it.

What you're proposing to do with a single readable could replace the counter. I would rather not use observables within the library itself, that would be pretty convoluted to maintain.

The reason the map exists is because the protocol allows for multiple events over the same stream. When you call select() you're creating a readable and then immediately caching it, so that the second time you call select() on the same event name, you're not creating a new readable again, but instead you're using the one that already exists.

Observables are a nightmare to maintain if you don't have specific syntax to deal with them, like .svelte files do with the $ syntax.

In the mean time I think I'm close to finding a better solution, but I've also found out some weird things with sveltekit itself.

It looks like it doesn't event bother unsubscribing from stores when $page updates, even if the stores are declared with const. I think it's got something to do with page caching.

I'll be back with an update.

tncrazvan commented 1 week ago

Hey @oscarhermoso , There's a bit to unpack, but I'll try my best to explain.

As mentioned before the main issue here is that svelte doesn't want to unsubscribe from the store whenever it updates it. I ran a few more tests to understand what's happening exactly with the life cycle.

The main culprit seems to be svelte's internal navigate(), which is triggered by both goto() and use:enhance. It seems like it's caching the page, probably for faster future navigations.

First of all I pushed a new version, please update to 0.9.0. There are some breaking changes, not many though. You can find more details here.

The update itself doesn't directly solve your specific problem, you will need to make 2 changes to your code for it to work as intended.

Mainly, you need to force svelte to invalidate the whole page. Either that or you can find your own way to invalidate specifically the readable stores returned by select(), json() or transform().

Using the example you provided in your fork, in order to solve the issue you must update your +layout.svelte file like so

<script>
  import { goto } from '$app/navigation'
  import { navigating, page } from '$app/stores'
  let lang = $page.url.searchParams.get('lang') || 'en'
</script>

<select
  bind:value={lang}
  on:change={async function pick(e) {
    $page.url.searchParams.set('lang', e.currentTarget.value)
    goto(`?${$page.url.searchParams}`, {
      // You need this!
      // Otherwise subsequent goto() calls will not invoke the load function.
      // If the load() function doesn't get invoked, then the #key below
      // will update the slot with the old source store, which will
      // be closed at that point.
      invalidateAll: true,
    })
  }}
>
  <option value="en">🇦🇺 English</option>
  <option value="it">🇮🇹 Italiano</option>
</select>
<br />
<br />
<!-- 
  Using goto() will not unsubscribe from your stores, 
  you need to force the client to unsubscribe using a #key. 
  The $page store works just fine as a key here.
  The $navigating store also works because 
  it is indirectly updated by goto().
-->
{#key $navigating}
  <slot />
{/key}

It took me a while to realize but we're witnessing the default behavior of svelte, which is to cache everything as soon as possible and switch to the client based router.

This behavior works great with static stores that are updated locally, but not when the stores themselves are updated from a remote source.

There are 2 key changes in the code above.

First the force invalidation

goto(`?${$page.url.searchParams}`, {
  // You need this!
  // Otherwise subsequent goto() calls will not invoke the load function.
  // If the load() function doesn't get invoked, then the #key below
  // will update the slot with the old source store, which will
  // be closed at that point.
  invalidateAll: true,
})

Like the comment says, it forces the load function to execute again, this needs to happen, I'll explain below why.

The second change is the #key

<!-- 
  Using goto() will not unsubscribe from your stores, 
  you need to force the client to unsubscribe using a #key. 
  The $page store works just fine as a key here.
  The $navigating store also works because 
  it is indirectly updated by goto().
-->
{#key $navigating}
  <slot />
{/key}

This will destroy and mount again your page when you navigate (both goto() and use:enhance will update this $navigating store when triggered) with goto() or use:enhance. But more importantly this will subscribe back to the correct stream.

However, if the load() function is not invalidated, the whole layout page will not render again, meaning your #key block will mount with the old stores.

You can actually test this yourself by changing your layout file like so:

<script>
  import { goto } from '$app/navigation'
  import { page } from '$app/stores'
  let lang = $page.url.searchParams.get('lang') || 'en'
</script>

<select
  bind:value={lang}
  on:change={async function pick(e) {
    $page.url.searchParams.set('lang', e.currentTarget.value)
    goto(`?${$page.url.searchParams}`, {
      // You need this!
      // Otherwise subsequent goto() calls will not invoke the load function.
      // If the load() function doesn't get invoked, then the #key below
      // will update the slot with the old source store, which will
      // be closed at that point.
      // invalidateAll: true,
    })
  }}
>
  <option value="en">🇦🇺 English</option>
  <option value="it">🇮🇹 Italiano</option>
</select>
<br />
<br />
<!-- 
  Using goto() will not unsubscribe from your stores, 
  you need to force the client to unsubscribe using a #key. 
  The $page store works just fine as a key here.
  The $navigating store also works because 
  it is indirectly updated by goto().
-->
<!-- {#key $navigating} -->
{(function inline() {
  console.log('hello world')
})()}
<slot />
<!-- {/key} -->

This is the worst case scenario, the page is neither invalidated nor the slot wrapper with a #key, and as you can see, the console.log triggers just once. Peek 2024-04-20 19-12

This will never work, both invalidation and the key are needed.

<script>
  import { goto } from '$app/navigation'
  import { navigating, page } from '$app/stores'
  let lang = $page.url.searchParams.get('lang') || 'en'
</script>

<select
  bind:value={lang}
  on:change={async function pick(e) {
    $page.url.searchParams.set('lang', e.currentTarget.value)
    goto(`?${$page.url.searchParams}`, {
      // You need this!
      // Otherwise subsequent goto() calls will not invoke the load function.
      // If the load() function doesn't get invoked, then the #key below
      // will update the slot with the old source store, which will
      // be closed at that point.
      invalidateAll: true,
    })
  }}
>
  <option value="en">🇦🇺 English</option>
  <option value="it">🇮🇹 Italiano</option>
</select>
<br />
<br />
<!-- 
  Using goto() will not unsubscribe from your stores, 
  you need to force the client to unsubscribe using a #key. 
  The $page store works just fine as a key here.
  The $navigating store also works because 
  it is indirectly updated by goto().
-->
{#key $navigating}
  {(function inline() {
    console.log('hello world')
  })()}
  <slot />
{/key}

Peek 2024-04-20 19-14

Fingers crossed, maybe Svelte 5 will bring more predictable behavior in situations like these. Other than that, there's not much else I can do in this situation.

You can find the full +layout.svelte file here

https://github.com/tncrazvan/sveltekit-sse/blob/510661fee5d1ad600b2fa76ec5963c9ffe3f3ca2/src/routes/%2Blayout.svelte#L1-L35

The +page.js file here

https://github.com/tncrazvan/sveltekit-sse/blob/510661fee5d1ad600b2fa76ec5963c9ffe3f3ca2/src/routes/%2Bpage.js#L1-L26

and the +page.svelte file here

https://github.com/tncrazvan/sveltekit-sse/blob/510661fee5d1ad600b2fa76ec5963c9ffe3f3ca2/src/routes/%2Bpage.svelte#L1-L13

Let me know if this fixes your issue.

oscarhermoso commented 1 week ago

Thank you again for the fast fixes and detailed reply.

You are correct about the invalidate being required alongside goto on a query string - that was something big that I missed. You are also correct that #key blocks are neccessary, when the store variable is a const.

However, I still did still run into the issues with duplicate connections being created on 0.9.0 of the code.

With that being said - the improvements that you added to 0.9.0 simplified a lot of the source code, and enabled me to make some changes that I had a lot of trouble implmenting on 0.8.17.

I have raised a PR that fixes the duplicate connections, and explained the changes here:

https://github.com/tncrazvan/sveltekit-sse/pull/35

tncrazvan commented 1 week ago

Hello again @oscarhermoso ,

As discussed in https://github.com/tncrazvan/sveltekit-sse/pull/35 , the changes are in and I've published a new version 0.9.1 where you can get them.

I've mentioned there were a few issues with these new changes.

Those should now be fixed.

The caching mechanism still exists and can be disabled through a cache:bool property like so

const channel = source(`/events`, { cache: false }).select('my-event')

I have no information to back this up, but I believe most use cases will benefit from this automation as a default behavior. I'm open to changing this in the future though.

This is not a 1.0.0 release yet so we can afford regular breaking changes like these with proper warnings ofc.

As a final note on this, the solution I wrote about above is still working with this new version of the library.

You can still isolate specific parts of the UI with #key. Even though it's dirty, it's either that, or disable sveltekit's hydration mechanisms.

I'm mentioning this because I've noticed that in your PR, this use case now does not work anymore:

Peek 2024-04-21 09-10

As you can see, switching the language a third time doesn't do anything.

This also happens in the new version, with or without caching

Peek 2024-04-21 09-22

And this brings it back to the navigate() issue I've showcased above using #key.

[!NOTE] You can also fix this by invalidating everything when using goto(), which has the same effect
goto(`?${$page.url.searchParams}`, { invalidateAll: true })

I'm assuming your PR covers your use case, and since all previous behavior still works, I see no harm in keeping the PR changes.

I'm leaving this issue open.

Update to the new version, check that it works as you intended it to work in your PR and close the issue if everything is ok.

oscarhermoso commented 1 week ago

Thank you again tncrazvan, I will get a chance to test the latest package version with my app next weekend

oscarhermoso commented 6 days ago

Thankyou - we are now deployed & running without issue on 0.9.1.

There were some difficulties getting it working as expected, due to different combinations of invalidateAll, cache: true/false, lock, etc. resulting in dropped connections that did not restart. I may raise separate issues once I have spent more time building with sveltekit-sse and understand the problems better :slightly_smiling_face:

The core problem is now solved though, so I am closing this ticket. Thank you for all the help :heart:

tncrazvan / sveltekit-sse

Exponential reconnections after implementing recommended reconnect #33

Context

Steps to reproduce

Expected

Actual

Steps to reproduce

TL;DR

Notes

Possible solution?