reagent-project / reagent

A minimalistic ClojureScript interface to React.js
http://reagent-project.github.io/
MIT License
4.75k stars 414 forks source link

Misunderstanding something about :key prop passing #34

Closed ptaoussanis closed 10 years ago

ptaoussanis commented 10 years ago

Hi Dan!

(defn foo  [i] [:li {:key (str "foo" i)} (str i)])
(defn foo1 []  [:li {:key "foo1"} "1"])

(reagent/render-component
  [:div "Hello,"
    ;; [:li {:key "foo1"} "1"] ; Gets :key
    ;; (foo1)                  ; Gets :key
    ;; (foo 1)                 ; Gets :key
    [foo1]     ; Doesn't get :key
    ;; [foo 1] ; Doesn't get :key
  [:li {:key "foo2"} "2"]]
  container)

So it seems that () component-calling forms keep their :key props, whereas [] component forms don't. Is that the expected behaviour?

My understanding was that the only difference between the () and [] calling forms was that the latter caches itself for rendering when its input args don't change. Am I missing something obvious here?

Thanks a lot, cheers! :-)

holmsand commented 10 years ago

Well, it is the behaviour I would expect :)

The difference between () and [] in this context is that the second form creates a new component instance, whereas calling with () just inlines whatever the component returns.

In all of your examples, keys will actually be generated, but at different depths. So when you have [foo1], you will get a "foo1" component without any key, but with a :li child that has a key.

This, of course, only matters when you use a dynamic list of components, i.e a seq. There the rule becomes that the top-level components contained in the seq must have keys (since React doesn't look any deeper than the first level).

Does that make sense?

And yes, the "key requirement" in React is probably its least elegant part. But it does make diffing incredibly fast even for long lists of components…

ptaoussanis commented 10 years ago

Really appreciate the quick response, thank you! I'm clearly still confused about some of the fundamental concepts here :-) Could I possibly check my understanding of a few things?:

Let's say that (defn my-function [s] [:div s]):

(for [text ["world" "universe" "multiverse"]] [my-function {:key text} text])

Do I have that all right?

So I was initially under the impression that [:div "hello" [my-function "world"]] would create two not three components, which is why the :key behaviour was surprising me. I figured there was a 1-to-1 relationship between components and dom elements, but that's not actually the case is it?

So my thinking should go - any time I see [<foo>, there's a component of type <foo> being created, even when <foo> is a function. If I want inlining behaviour, I need (<foo>.

One last question:

If [my-function <arguments>]'s rendering is cached, do we not face an issue where the cache just grows without bounds when the argument input domain is unbounded?

For example, what if the arguments contain something like the value of a user input text field? As the user types, more and more arguments will get cached, no?

Does that question make sense?

Again thank you so much for your assistance here, it's been a big help!

holmsand commented 10 years ago

Yes, you have that all right. Full points :)

The only possible exception is the caching behaviour (and, as a consequence of that, when you should prefer function calls over component creation (i.e () vs. []).

There's no risk of runaway cache growth, since there's just one value in the "cache" at any given time for a component. Caching works like this: when you re-render a component, React uses some heuristics to find the children that are "the same" (basically based on position and component type, i.e the first element in a hiccup form in Reagent's case, and based on the key value if it is a javascript array/seq). When React has found a matching pair of new component instance and an old one, it calls shouldComponentUpdate, with the old and new parameters to the component. Reagent's default implementation of shouldComponentUpdate is basically to compare all the elements of the hiccup form vector using identical? (except for maps, where all the keys/values are compared using identical?).

If shouldComponentUpdate returns true, render is called again (and any old values are thrown away), and the whole process is repeated for every child. Otherwise nothing else happens.

So the performance tradeoff between () and [] will depend on how costly the component function (i.e my-function here) is. In this case it is extremely cheap, so using () will probably be a tiny bit faster (due to a couple fewer memory allocations). But I guess it would be very difficult to come up with a real world case where you could actually notice the difference – so it might be easier to just stick to [] all over the place by default.

A couple of more points, while I'm at it:

Btw: very good questions! It is always very useful to have to explain the foundations of something, and in particular to discover what needs explaining and what not. And to re-discover the severe limitations of human telepathic capabilities – it is surprisingly easy to forget that people actually can't read your mind :)

ptaoussanis commented 10 years ago

Great, appreciate the clarifications. Managed to clear up the last of my missing :key warnings now that I've got a better idea of the actual component hierarchy.

Enjoy the rest of your weekend & keep kicking ass - Reagent continues to be an absolute pleasure to use. Cheers! :-)

ptaoussanis commented 10 years ago

Thinking this through some more, I got to wondering how you were identifying fn equality for the purposes of component identity:

(defn my-component []
  (let [state_ (reagent/atom nil)]
    (fn []
      (let [state' @state_
            my-input
            (fn [] [:input
                   {:value state'
                    :on-change (fn [ev]
                                 (reset! state_
                                   (-> ev .-currentTarget .-value)))}])]
        [:div [my-input]]))))

So in cases like this where the component fn is actually defined within the render call, we lose fn identity and the [my-input] component will actually get destroyed on every change.

I guess there's no way of avoiding that, right? I see that solutions here include using (my-input) instead, or pulling the my-input fn definition outside of the render scope to preserve its identity.

What would your usual approach here be? And I'm wondering, did you consider alternatives like maybe allowing fn metadata to convey identity like you've done with ^{:key _}?

Otherwise I'm seeing two possibilities for a mental model here:

  1. Use [my-component] for top-level (def) components only, otherwise prefer inlining.
  2. Always define components outside of rendering scope, as with reactive atoms.
holmsand commented 10 years ago

Yes, re-defining my-input like that on every render is a bad idea in general... Not only will it force a re-render (since there's no way of knowing that it is the "same" function, as you say), it will also trigger a new call to React's createClass on every render (since it is a brand new component class, not just a new component instance).

Allowing "identity by metadata" is an interesting idea. But that would require keeping some kind of registry of component classes, which might get hairy pretty quickly (and you'd have to be very careful not to get unlimited cache growth). I'd have to think some more about that...

Right now, I'm thinking that sticking to your two rules is better (both of them are valid, of course). In particular, using named functions as often as possible is probably a good idea in general.

Btw: the [:input ...] component in your example would always re-render anyway, since :on-change is always given a new value... Function equality is tricky in Clojure in general, and in ClojureScript in particular (since there are no vars).

But on the other hand, worrying too much about unnecessary re-renderings and such might be premature optimization. Unless your component function is particularly expensive (having thousands of children, or somesuch), most of the time you won't even notice the extra renderings. After all, plain React is quite fast, and it always re-renders by default.

ptaoussanis commented 10 years ago

Allowing "identity by metadata" is an interesting idea. But that would require keeping some kind of registry of component classes, which might get hairy pretty quickly (and you'd have to be very careful not to get unlimited cache growth). I'd have to think some more about that...

Okay, sure. Probably not worth the effort then, I think the current alternatives are quite reasonable.

Btw: the [:input ...] component in your example would always re-render anyway, since :on-change is always given a new value... Function equality is tricky in Clojure in general, and in ClojureScript in particular (since there are no vars).

Re-rendering is fine (and I'd agree with you that most of the time the performance difference won't be significant). The component destruction can have other undesirable effects though, like losing the input focus in this example. That's something I'd definitely want to try avoid.

Right now, I'm thinking that sticking to your two rules is better

Will do, thanks!

holmsand commented 10 years ago

Re-rendering is fine (and I'd agree with you that most of the time the performance difference won't be significant). The component destruction can have other undesirable effects though, like losing the input focus in this example. That's something I'd definitely want to try avoid.

Oh, absolutely. I was just thinking about the :on-change part. And yes, inputs have to be treated extra gently :)

yayitswei commented 10 years ago

I believe this is a related question. I was trying to figure out why my component got destroyed on every change, and this thread helped clarify- it's because I've lost function identity by defining the function within the render call.

;; child component
(defn list-element [id]
  (with-meta
    (fn [] [:li (:id @list)]) ;; @list is a map of ids to values
    {:component-did-mount (fn [] (println "component mounted." id))}))

;; parent component
[:ul (for [[id _] @list] ^{:key id} [(list-element id)])]

Can you suggest an alternate implementation where the component doesn't get destroyed every change, yet still lets me access id inside its lifecycle functions?

holmsand commented 10 years ago

That was a very cool solution (even if it, as you say, will be quite inefficient)!

I would do something like this instead:

(def alist (atom {"id1" "foo", "id2" "bar"}))

(defn list-element [id]
  [:li (@alist id)])

(def list-element'
  (with-meta list-element
    {:component-did-mount
     (fn [this]
       (let [[_ id] (reagent/argv this)]
         (println "component mounted. " id)))}))

(defn parent-component []
  [:ul (for [[id _] @alist]
         ^{:key id} [list-element' id])])

:component-did-mount gets a reference to the mounted component (a.k.a this), which is passed to reagent.core/argv to get at the argument vector (i.e [list-element' id] in this case). Definitely more boring than your solution, but it works :)

yayitswei commented 10 years ago

Darn, accidentally deleted my last comment..

After reading up on React, your solution now makes sense to me :)

The key for me was understanding the mounted component this, along with reagent/props and reagent/state. Might make sense to call them out in the README documentation as I wasn't aware until I dug through the geometry example. I ended up modifying your solution above to use props:

(for [[id value] @alist] ^{:key id} [list-element` {:id id :value value}])

and calling reagent/props in the lifecycle functions.

Thanks for making Reagent and being responsive on these threads. Very fun to use!