tncrazvan / sveltekit-sse

Server Sent Events with SvelteKit
https://www.npmjs.com/package/sveltekit-sse
MIT License
199 stars 7 forks source link

fix(source): fix duplicate connections - remove cache, connect as store #35

Closed oscarhermoso closed 3 weeks ago

oscarhermoso commented 3 weeks ago

Fixes #33

Changes

Remove cache from stream()

As discussed in #33 - there was an issue with duplicate connections being made. I have stripped out a lot of code out in stream.js, and this has resolved this issue.

I understand this also removes a feature. Previously, users were previously able to source() multiple streams from the same URL, without creating duplicate beacons/fetch requests.

Perhaps that functionality could be restored by adding a cache to the connectable() method, or fixing the intended behaviour in stream() - but it is not included in this PR. (My personal perspective is it should be users responsibility to make sure they don't create two sources... but I do understand the value of including this feature).

Rewrite internal connect() function to instead return readable()

This simplifies source(), and internally changes select() to return a derived store. Does not change the functionality for users.

Updated example app

Removed the {#key $navigating} - #key blocks are great for re-rendering small components, but wrapping an entire layout slot is a little bit spicy in larger web apps.

Instead, we can reactively bind the store to the page data, so the store reference is updated whenever the page data changes.

tncrazvan commented 3 weeks ago

Hey @oscarhermoso , first of all thank you for the PR.

Although this seems to fix the issue and is a great rewrite, the core problem is not fixed. As soon as we'll introduce the cached connections again the problem will appear again too. No matter how we spin the source(), connect() / connectable() or stream() functions, the problem still remains, which is: sveltekit won't unsubscribe from stores while it's caching pages, it just won't call the stop() function of any store.

I'll take the day to think about this, you might be right in that

it should be users responsibility to make sure they don't create two sources

Although I would like to solve that problem internally, caching in itself is complicated, and when you ammount more problems like these on top of it, it might not be worth it.

tncrazvan commented 3 weeks ago

Although this seems to fix the issue and is a great rewrite, the core problem is not fixed. As soon as we'll introduce the cached connections again the problem will appear again too. No matter how we spin the source(), connect() / connectable() or stream() functions, the problem still remains, which is: sveltekit won't unsubscribe from stores while it's caching pages, it just won't call the stop() function of any store.

It looks like the connectable store itself behaves correctly if cached. I think you might have solved it! :)

I'll merge this and come back with an update of an option to disable the caching feature.

tncrazvan commented 3 weeks ago

After playing with it a bit more it looks like more behaviors have been stripped out than just caching. Unmounting a component simply doesn't close the stream, it just keeps streaming even though there are no subscribers to the store.

This is an issue, things should disconnect and reconnect when components are destroyed and mounted again (if no other subscribers are watching the store). So something like this won't close the connection anymore when condition is false

{#if condition}
   <MyStreamingComponent />
{/if}

This must be supported out of the box, unlike caching, it is very important to work.

The issue is probably this

https://github.com/tncrazvan/sveltekit-sse/blob/0ef313ad746ab4bc21c0b9a08d326b7e6a804375/src/lib/source.js#L208-L212

It cannot be just a derived store, we need to disconnect on stop(), which you don't have access to in a derived store, it must use a readable or equivalent. That's the reason it was using readable before, instead of a derived store.

I'll need to make some changes before I publish a new version.

Edit

Using derived

https://svelte.dev/docs/svelte-store#derived

If you return a function from the callback, it will be called when a) the callback runs again, or b) the last subscriber unsubscribes.

Whereas readable's returned callback only runs when the last subscriber unsubscribes.

oscarhermoso commented 3 weeks ago

Ah, sorry about that. Please don't feel rushed/pressured to fix. Let me know if would like any assistance.