fulcrologic / fulcro

A library for development of single-page full-stack web applications in clj/cljs
http://fulcro.fulcrologic.com
MIT License
1.55k stars 140 forks source link

Add `listener-id` option to `add-component!` and `add-root!` #505

Closed zeitstein closed 2 years ago

awkay commented 2 years ago

Your docstrings leave something to be desired...read those as if you don't know what you're doing, and also correlate them with the function they are in. What you wrote doesn't make a lot of sense.

for example, the first one should probably be something like "The ID used for the render listener. Defaults to the root-key, but you can override it with this option".

Then in remove root we have an issue...it's ident by default? What happened to root-key? Seems I messed up something, but still the docstring you're using there should be more like "the render listener id that was used for this root. Defaults to the ident, but if you overrode it when adding then you'll need to pass your value for it with this option".

zeitstein commented 2 years ago

Will improve the docstrings. I think you're overlooking 'line truncation', because you seem to be referencing changes made to add-component! in you last paragraph?

About remove-root! and remove-component!. I debated whether to introduce changes to them. Something like:

(defn remove-root!
  "Remove a root key managed subtree from Fulcro. Does not garbage collect, just stops updating the callback."
  [app root-key-or-listener-id]
  (remove-render-listener! app root-key-or-listener-id))

(defn remove-component!
  "Remove a root key managed subtree from Fulcro. Does not GC the state, just stops sending props updates on render."
  [app component-or-listener-id]
  (if-not (comp/component-class? component-or-listener-id)
    (remove-render-listener! app component-or-listener-id)
    (let [initial-entity (comp/get-initial-state component-or-listener-id {})
          ident          (comp/get-ident component-or-listener-id initial-entity)]
      (remove-render-listener! app ident))))

;; also change their docstrings

But I opted not to do this, since remove-render-listener! exists. There is certainly an argument to do this, though. So let me know what you prefer and I'll submit a new commit.

awkay commented 2 years ago

You're right that I was probably getting confused by the diff.