Skip to content
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

Watched subscriptions are not disposed in inject-sub-cofx interceptor #683

Closed
dmitryn opened this issue Mar 19, 2021 · 1 comment
Closed

Comments

@dmitryn
Copy link

dmitryn commented Mar 19, 2021

Hi 👋

Issue is not directly related to re-frame, but to the re-frame-utils library which uses approach described in re-frame docs and in the comments of this issue

I also created an issue in re-frame-utils but it seems like library is not actively maintained so i'm writing here.

Here is the summary of an issue:

If you deref reagent atom outside of reagent rendering context, dependent subscription are not updated inside a watching list of parent subscription. So when you dispose! it, dependent subs are not getting disposed.
Instead of using deref, should reagent.ratom/run be used which properly updates ratom watching list.

Here is the code snippet which demonstrates the issue:

(ns inject-sub-cofx
  (:require [re-frame.core :refer [reg-sub subscribe]))


(reg-sub :bar
  (fn [db]
    (count db)))

(reg-sub :foo
  (fn [_]
    (subscribe [:bar]))
  (fn [a _] a))
             
             
(comment
 (def sub (subscribe [:foo]))
 (deref sub)
 (.-watching sub) ;; => nil
 (reagent.ratom/dispose! sub)
 (filter #(= :bar (get-in % [0 0 0])) @re-frame.subs/query->reaction) ; => ([[[:bar] []] #object[reagent.ratom.Reaction {:val 67}]]))
 (reagent.ratom/run sub)
 (.-watching sub) ;; => #js[#object[reagent.ratom.Reaction {:val 67}] #object[reagent.ratom.Reaction {:val 67}]]
 (reagent.ratom/dispose! sub)
 (filter #(= :bar (get-in % [0 0 0])) @re-frame.subs/query->reaction)) ;; => ()

We actively using re-frame-utils library in our project (forked version) and i think we need to apply proposed patch (using reagent.ratom/run instead of deref).

Please confirm or tell my assumption is wrong. Thanks.

@kimo-k
Copy link
Contributor

kimo-k commented Aug 13, 2023

This behavior is a consequence of creating both subscriptions outside of a reactive context. Reagent doesn't do any reference-counting in that case.

I agree that re-frame-utils should probably sync a reaction with the reactions it watches before disposing it.

I don't think we will merge re-frame-utils, so I can't directly do much. Calling run might come in handy to implement our future api, but that alone doesn't solve the broader issue.

We discuss the concern of subscription lifecycle in detail on #680. I hope to get a solution out soon. It should be more friendly & feature-complete than re-frame-utils.

@kimo-k kimo-k closed this as completed Aug 13, 2023
@kimo-k kimo-k closed this as not planned Won't fix, can't repro, duplicate, stale Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants