cjohansen / portfolio

Eclipse Public License 1.0
222 stars 13 forks source link

Reagent form-3 component cannot be called with :params containing r/atom without remounts on state change #27

Closed Reefersleep closed 2 weeks ago

Reefersleep commented 2 months ago

When setting up a form-3 component in a scene, if you use a r/atom in :params, the component will be re-mounted every time the r/atom state changes.

My motivation for using components this way is to pass reagent cursors (which behave like ratoms) to components to "delegate" from a global r/atom, and let the subcomponents set up their own state within the cursor, and this seems to work in vanilla reagent use - but it doesn't gel with :params in Portfolio.

Additionally, clicking back and forth between two such scenes causes a rendering problem.

More details in the code example below.

(ns example-ns
  (:require [portfolio.reagent :refer-macros [defscene]]
            [reagent.core :as r]))

(defn example-of-form-3-component
  [{:keys [person-name
           state-atom]}]
  (prn "mounted")                                           ;;Should only be called once on visiting a UI containing this component.
  (let [initial-state {:num-clicks  0                       ;;These things in the let will be closed over.
                       :person-name person-name}]
    (reagent.core/create-class                              ;;This is the form-3 bit.
      ;;Read more about Reagent form-3 components at https://github.com/reagent-project/reagent/blob/master/doc/CreatingReagentComponents.md#form-3-a-class-with-life-cycle-methods
      {:display-name "example-of-form-3-component"
       :constructor  (fn [_] (reset! state-atom initial-state))
       :reagent-render
       (fn [_]
         [:section
          [:div "Hello " (:person-name @state-atom)]
          [:div "Different name? " [:input {:type      :text
                                            :value     (:person-name @state-atom)
                                            :on-change (fn [e]
                                                         (prn ;;To see that there's actually a change
                                                           (swap! state-atom assoc :person-name (-> e .-target .-value))))}]]
          [:div "Number of clicks " (:num-clicks @state-atom)]
          [:input {:type     :button
                   :value    "Click me!"
                   :on-click (fn [_]
                               (prn                         ;;To see that there's actually a change
                                 (swap! state-atom update :num-clicks inc)))}]])})))

(defscene working-form-3-component-test
  :params nil
  [_]
  ;;This usage of the component seems to work;
  ;;Every update to state-atom does not cause a re-mount,
  ;;and the state seems to propagate properly to the UI.
  ;;This calling format also works as expected in
  ;;vanilla reagent use of the component.
  [example-of-form-3-component {:state-atom  (r/atom nil)
                                 :person-name "Megatron"}])

(defscene non-working-form-3-component-test
  :params {:state-atom  (r/atom nil)
           :person-name "Megatron"}
  ;;This usage of the component does not work;
  ;;Every update to state-atom causes a re-mount,
  ;;which means state-atom is reset!, giving
  ;;the appearance that UI interaction caused
  ;;no change in the UI.
  [params]
  [example-of-form-3-component params])

;;Additionally, clicking back and forth between these two scenes
;;causes an error when going 'back' to the non-working-form-3-component-test scene;

;Failed to render 'Non form 3 component test'
;Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
;
;Component params
;{:state-atom #<Atom: {:num-clicks 0, :person-name "Megatron"}>,
; :person-name "Megatron"}
;
;Error: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.
;    at Object.insertBefore (http://localhost:3449/js/compiled/out-scenes/dumdom/snabbdom.js?zx=l1eikzeiv3sz:19:16)
;    at updateChildren (http://localhost:3449/js/compiled/out-scenes/dumdom/snabbdom.js?zx=l1eikzeiv3sz:326:25)
;    at patchVnode (http://localhost:3449/js/compiled/out-scenes/dumdom/snabbdom.js?zx=l1eikzeiv3sz:369:21)
; ... and so on
cjohansen commented 2 months ago

Portfolio is designed to view snapshots of your UI components, primarily with stateless components in mind. It has some support for atoms to enable scenes to visualise transitions, but in general you should not expect to run stateful apps in scenes.

Portfolio will watch any atom (specifically: instances of cljs.core/IWatchable) in :params, and re-render the scene when it changes. If you do not want Portfolio to re-render your scene, then you should not pass atoms via :params.

I don't know how Reagent works. I see that you ask Portfolio to render something with a function that creates a component class. Is that really necessary? It's not clear to me what it achieves.

I'll look into the exception, that shouldn't happen.

Reefersleep commented 1 month ago

I don't know how Reagent works. I see that you ask Portfolio to render something with a function that creates a component class. Is that really necessary? It's not clear to me what it achieves.

I'm trying to achieve the following pattern; A. A reagent component function maintains its own state. This can be achieved with form-2 components or use of with-let to set up essentially a closure over a r/atom that is referenced within the lexical scope of the component function for the lifetime of the component (meaning; as long as the component remains mounted). B. But I want to have my cake and eat it, too. I want the state to be "delegated" from global state, by passing the component function a r/cursor pointing to some specific path in the global r/atom. The cursor acts as a "local r/atom - any changes to its state are really changes to the state at the end of that specific path in the global r/atom. This pattern works fine in reagent without complicating the code particularly. C. Additionally, I'd like for my component to set up its initial state by itself, too, by calling reset! or swap! on its iDeref, whatever it may be, in the "do-once" part of the with-let or form-2 pattern. This works fine with a local r/atom, but if you do it on a r/cursor or r/atom that is provided for the fn, you can run into a situation where your update will force reagent to call the component again with the updated arguments, and your setup will run, and it'll be called again, and the setup will run, and so on in a loop.

I didn't understand why C. was not possible with either a with-let or form-2 component, because setting up initial state exactly once is kind of their point. So, I tried using a form-3 component, which is what you see above, using the reagent.core/create-class function to gain fine-grained control over lifecycle functionality of a component. I thought that this would be more powerful somehow.

And it did seem that this was the case; it worked as I had hoped, except in Portfolio. Hence this Github issue.

However, on retesting, it seems like using with-let/form-2 does allow me to achieve what I want. Something must have been off in my initial attempt, or maybe my current attempt doesn't capture the real-life app situation properly. Further testing will reveal whether I can achieve what I want without resorting to form-3 components.

Aside from the above, I take away 1 thing from your comment; Reagent form-3 components are currently not supported in Portfolio, at least not at the top level, it seems.

This is too bad, as there's no knowing if we'll have to use more form-3 components in my work project or not. We do, currently, for very few components, which are currently not part of our Portfolio stable.

As an aside, I've also found the re-rendering of Portfolio :params atoms stunted for my particular use case, at least when I last tried; e.g., writing in a text field, the entirety of the component would be re-rendered on each update, meaning that I'd have to click or otherwise give the text field focus again after each button press. So I refrain from using atoms in :params, instead setting up "outer" state in the component calling code, e.g.

(defscene something
  [_]
  (r/with-let [state (r/atom nil)
               on-change (fn [name] (reset! state name))]
    [my-component {:value @state
                   :on-change on-change}]))

as this seems to work.

Reefersleep commented 1 month ago

Ah, re-reading your comment, I think maybe I conflated my initial observation of my particular Github-issue-example-specific usage of the form-3 component not working, with form-3 components not working at all in Portfolio. To be honest, I don't know if this is the case. Additional testing needed :)

cjohansen commented 2 weeks ago

I'm having trouble wrapping my head around all the nuances of reagent's inner workings.

In any case, I did find that Portfolio was too eagerly unmounting reagent scenes. I fixed this in 2024.06.30. With this fix I am at least able to navigate between the two scenes in your example without exceptions.