Allowing lifecycle metadata on inner component fns

[ptaoussanis]

Hi Dan, hope you're well!

Think this might be clearer to explain with an example. It's currently possible to write something like:

(with-meta
  (fn outer-component [arg1 arg2]
    (let [local-state (atom {})] ; Perform setup, etc.
      (fn inner-component [arg1 arg2]
        [:div (str arg1 "," arg2 "," @local-state)])))
  {:component-did-mount (fn [this])})

This works, but the :component-did-mount fn has to do w/o access to local-state.

It's be nice to support something like this:

(fn outer-component [arg1 arg2]
  (let [local-state (atom {})] ; Perform setup, etc.
    (with-meta
      (fn inner-component [arg1 arg2]
        [:div (str arg1 "," arg2 "," @local-state)])
      {:component-did-mount
       (fn [this]
         ;; Has access to local-state (but doesn't currently trigger)
         )})))

I.e. by allowing lifecycle metadata on the inner component, we gain the ability to share local setup state with the mount fn.

Does that make sense?
Think it'd be feasible?
Any downsides?
Would a PR be welcome?

Thanks a lot! No urgency on this at all, know you've been busy.

Cheers :-)

[mike-thompson-day8]

A similar request:
https://groups.google.com/forum/#!topic/clojurescript/rGcBOvLOhm4

[mike-thompson-day8]

For the record this is my current solution:

(defn main
   []    ;; could be  [x y z] here
   (let [some (local but shared state)      ;;   <--- closed over by all lifecycle fns
         can  (go here)]   
      (reagent/create-class
        {:component-did-mount
          #(println "main:  component-DID-mount")

         :component-will-mount
          #(println "main:  component-WILL-mount")

         :render
          (fn [this]                                ;; unfortunately, real params not supplied. 
              (let [[_ x y z] (reagent/argv this)]  ;; Ugly: get real params. Ignore the first
                   [:div
                    nil
                    (str x " " y " " z)]))})))


(reagent/render-component 
    [main 1 2 3]         ;; pass in x y z
    (.-body js/document))

Kinda okay, except for the ugly step in accessing reagent/argv.

[mike-thompson-day8]

A neater solution (at the uncomfortable expense of using an internal implementation detail):

(defn main
   []    ;; could be  [x y z] here
   (let [some (local but shared state)      ;;   <--- closed over by all lifecycle fns
         can  (go here)]   
      (reagent/create-class
        {:component-did-mount
          #(println "main:  component-DID-mount")

         :component-will-mount
          #(println "main:  component-WILL-mount")

         :component-function         ;; notice: using ':component-function' instead of ':render'
          (fn [x y z]                 ;; correct parameters - nice 
                   [:div
                    nil
                    (str x " " y " " z)]))}))


(reagent/render-component 
    [main 1 2 3]         ;; pass in x y z
    (.-body js/document))

@holmsand Any chance we could de-risk this approach by making :component-function officially part of the reagent API? (but perhaps with a more render-y name, like :reagent-render -- if so renames required here and here).

What we have above is so neat, and feels so right, compared to the with-meta approach which to me seems:

1. messy - code for one component spread across multiple functions
2. can't handle the scenario originally raised by @ptaoussanis in this issue.

[holmsand]

@mike-thompson-day8 I completely agree that :component-function should be documented (and probably also this "return results from create-class from render trick"). It is very elegant, and I didn't even know you could do it until you brought it up...

I added a couple of tests for the trick (and :component-function) in testreagent.cljs, to make sure that it keeps working.

About that name: I've been calling "functions that become components when called using [fun args] in Hiccup forms" for "component functions" in the docs, so at least :component-function is consistent with that.

An alternative/complementary approach to the "return-create-class-trick" to make sharing local state between different lifecycle methods easier is to expose the atom that supports set-state etc. You could get at that atom by calling, say, (r/state-atom this) or (r/state-atom (r/current-component)).

Any thoughts on that?

[skrat]

I don't think that's ((r/state-atom this) or (r/state-atom this)) very elegant. I think Reagent should strive for functional solution, that is, state in closures, or at least access to atom via closure (and across life-cycle functions). That means, both rendering and life-cycle functions, should have access to the same vars via closure, be it atoms or whatever else.

[mike-thompson-day8]

I'd too would vote for the "return-create-class-trick" (as shown above).

[Frozenlock]

👍 return-create-class-trick

Would it be possible/expensive to create classes on the fly each time one of :component-did-X is present in a component attributes?

;; could this...

(defn my-div
  [_]
  (reagent/create-class
   {:component-did-mount #(println "main:  component-DID-mount")    
    :component-will-mount  #(println "main:  component-WILL-mount")    
    :component-function  (fn [_]
                           [:div {:style {:border "1px solid black"}}
                                  "some div"])}))

;; become this?
(defn my-div [_]
  [:div {:style {:border "1px solid black"}
         :component-did-mount #(println "main:  component-DID-mount")
         :component-will-mount #(println "main:  component-WILL-mount")}
   "Some div..."])

[mike-thompson-day8]

@Frozenlock I'm not keen to see React lifecycle functions mixed in with style. I'd prefer to see the two worlds separate.

[Frozenlock]

style is one level nested into the attributes map, which already filters out unknown/incompatible items when rendering to DOM. (I learned that the hard way when some SVG properties were silently thrown out.)

Maybe it's because I've used the metadata approach up until now, but I've always found the lifecycle functions to be a hassle. At least in comparison to everything else which pretty much just works.

I think of it this way: what if the attributes were metadata up until now?

(defn my-div [a]
  [:div (str "my div :" a)])

(with-meta
  my-div {:style {:border "1px solid black"}}) ;; boooo!

;;; but hiccup had the brilliant idea of an optional
;;; attributes (properties) map

(defn my-div [a]
  [:div {:style {:border "1px solid black"}}
   (str "my div :" a)])  ;; yeah!

Anyways, I don't use the lifecycle functions enough to have any strong feelings about it.

[ilyabo]

I also find that :component-function is not the best name. :component-render would be much more descriptive and understandable.

[mike-thompson-day8]

I've created a Wiki page, so we have an explanatory resource to point to when this issue comes up again: https://github.com/reagent-project/reagent/wiki/Creating-Components
Any edits or clarifications on that Wiki page are most welcome.

[seancorfield]

Great wiki page @mike-thompson-day8 but it makes me wonder whether adding :component-render as an alias for :component-function (and perhaps deprecating the latter) would make usage more obvious.

In fact, why not use create-component and have it use aliases for all the lifecycle methods that drop the component- prefix? It would be less typing than the current approach and more consistent (especially since you are creating a component not a class?).

[holmsand]

Doesn't :component-render risk getting confusing? After all, :render renders a component as well...

How about :function-render, to (hopefully) highlight that this thing is called using "function conventions", rather than the boring old React way?

Or :fn-render? Or something better...?

[gadfly361]

👍 :function-render (but i think any of the suggestion are an improvement over :component-function)

[skrat]

Well, anything with function in it is a face palm. Let's see, what about...

(defn my-component
  [x y z]  
  (let [some (local but shared state)
        can  (go here)]   
     (reagent/create-class
       {:did-mount  #(println "Did mount")
        :will-mount #(println "Will mount")
        :render
          (fn [x y z]
            [:div (str x " " y " " z)]))}))

Because, component- prefixes are pointless. I also think that the argument repetition is a problem.

[seancorfield]

I'm suggesting mapping from "Reagent keys" which we choose and control to "React keys" rather than just slavishly following the React lifecycle names as keys.

As noted on @mike-thompson-day8 wiki page:

"Its a trap to mistakenly use :render because you won't get any errors, except the function you supply will only ever be called with one parameter, and it won't be the one you expect."

I think we can remove that mistake by making :render do the right thing by having create-class (or, better, create-component) map :render to what users would intuitively expect, as @skrat shows in an example.

Having to repeatedly type :component- for no good reason seems counter to the otherwise simple and elegant approach Reagent takes elsewhere.

[holmsand]

@skrat and @seancorfield Naming is hard :)

I agree that :render should have been the one with Reagent-style arguments, but I think it would be a bit nasty to change it now (without any way of issuing a warning).

As you say @seancorfield, switching to something like create-component with a new argument for :render is probably the way forward (create-class could then be deprecated with a suitable warning).

In the meantime, the-thing-that-is-now-component-function must be called something, and since there seems to be universal hate for the previous alternatives, how about these:

• :reagent-render (at least it sort of says what it does...)
• :run (short and descriptive, but maybe differs too much from the normal React terminology?)

That would maybe be a temporary solution anyway, if we go with create-component or some such, but it would sure be nice to get a 0.5.0 out of the door :)

[mike-thompson-day8]

+1 for reagent-render

[holmsand]

Reagent 0.5.0-alpha3 now supports :reagent-render. I still think that create-component is a good idea, though, but after 0.5.0.

[mike-thompson-day8]

I think this issue can be closed

[johanatan]

I kinda like the with-meta syntax better. Any reason why a change can't be made to allow it on inner components? It seems a bit inconsistent to use one method when outer and another when inner (and the reasons for this glaring inconsistency will not be immediately obvious on a new reader's first encounter with a reagent project). Given this, the only responsible way forward is to avoid with-meta entirely and explicitly use create-class everywhere.

[yogthos]

Actually, I believe this should still work if you wrap the inner component with a vector:

(fn outer-component [arg1 arg2]
  (let [local-state (atom {})] ; Perform setup, etc.
    [(with-meta
       (fn inner-component [arg1 arg2]
         [:div (str arg1 "," arg2 "," @local-state)])
       {:component-did-mount
        (fn [this]
          ;; Has access to local-state (but doesn't currently trigger)
          )})]))

[kangbb]

Actually, I believe this should still work if you wrap the inner component with a vector:

(fn outer-component [arg1 arg2]
  (let [local-state (atom {})] ; Perform setup, etc.
    [(with-meta
       (fn inner-component [arg1 arg2]
         [:div (str arg1 "," arg2 "," @local-state)])
       {:component-did-mount
        (fn [this]
          ;; Has access to local-state (but doesn't currently trigger)
          )})]))

I have try your answer, actually, pass arg1 arg2 to inner-component will make them undefined. If want work, we should do:

(fn outer-component [arg1 arg2]
   (let [local-state (atom {})] ; Perform setup, etc.
     [(with-meta
        (fn inner-component []
          [:div (str arg1 "," arg2 "," @local-state)])
        {:component-did-mount
         (fn [this]
           ;; Has access to local-state (but doesn't currently trigger)
           )})]))

But this is against with Form-2 that how to create a component, which is recommended by officials'.