Closed oscarhermoso closed 7 months 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.
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.
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
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.
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.
Ah, sorry about that. Please don't feel rushed/pressured to fix. Let me know if would like any assistance.
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.source()
will always create oneconnectable()
, which will call onefetchWithProgress()
select()
/transform()
/json()
can be derived and subscribed from this sourceI 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 instream()
- 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 returnreadable()
This simplifies
source()
, and internally changesselect()
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.