Warn user when calling subscribe outside of a reactive context #752
Conversation
Calling subscribe outside of a reactive context will cache the subscription, but there will be no ratoms around to remove the subscription from the cache when the ratom is disposed. This can lead to memory leaks where subscriptions stay in the cache indefinately. This should fix issue day8#740
|
I put this in the issue, but wanted to drop it here too. It may be worth figuring out how to change re-frame such that this warning is not necessary. I made this branch a while back to explore what seems to be a reasonable solution to the problem: In short, when the current call to subscribe is not happening in a reactive context and a subscription is not already cached, then bypass the subscription cache. Just return a new reaction. If the subscription was already cached then any code can grab and derefence that cached subscription (which is now only cached in reactive contexts, so something will eventually clean it up). |
|
Thanks @dannyfreeman 👍🏻 Code is good quality. Please raise a new issue for the solution described in #752 (comment) as my understanding is it would be best separated as a distinct future change to this even if it'll replace this warning. |
|
Thank you @superstructor |
After this update re-frame prints warnings to console log regarding subscription leaks. The change was introduced in day8/re-frame#752 and released in https://github.com/day8/re-frame/releases/tag/v1.3.0 and quite many components in REMS are affected. shadow-cljs update *should* be harmless.
I saw the help wanted tag on issue #740 and decided to open a PR for it
This will log warnings to the console every time a call to subscribe is made in a place it shouldn't be called, like event handlers.
Here is what the warning looks like in firefox:
Right now it is printed as a warning. It may be more useful to print it as an error so the user can see the stack trace and find what code triggered the message.