-
-
Notifications
You must be signed in to change notification settings - Fork 98
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
Bug: Unsubscribe not called for synced observable #396
Comments
Ah interesting! It's supposed to unsubscribe when no longer observed. So I'll look into that. I have a feeling it may be caused by strict mode. I'll let you know. |
Hey :) |
ok here is an out of react example: const syncedObservable = observable(
synced({
get: () => 'foo',
subscribe: () => {
console.log('subscribe');
return () => console.log('unsubscribe');
},
})
);
const unsubscribe = observe(() => {
console.log(syncedObservable.get());
});
setTimeout(unsubscribe, 500); |
Hmm, I think maybe the issue is that stopping observing does not (currently) immediately call the unsubscribe function. On the next update it will see that it's not currently being observed and then call unsubscribe. So I think if you set it to something after unsubscribing you'll see that "unsubscribe" log. But I can see how that's not the ideal behavior! I'll see if I can improve that. |
That does not appear to make a difference unless I understood it wrong but I tried 2 things:
in both cases value updates while subscribed correctly but if called after unsubscription happened does not call the unsubscription callback |
Ok, I'll look into it more and get back to you. |
Hi 👋
I am trying to integrate 3rd party subscription into legend observable, I need to know when the data is observed to make sure I can manage the other subscription accordingly, tho I saw unsubscribe is never called for my case.
Below is a simple repro unsubscribe log is never called (running
3.0.0-beta.17
):Unsubscribe is called if I use
useObservable(synced
directly, tho it does not really satisfy my case.Thanks
Great lib btw 🔥
The text was updated successfully, but these errors were encountered: