Sequences and om/react keys

[aiba]

I'm rendering table headers with om and sablono using a for loop:

(html [:table
       [:tbody
        [:tr (for [k [:last-name :first-name :date]]
               [:th (name k)])]]])

This generates a react warning, "Each child in an array should have a unique \"key\" prop."

But in this case, I think I just want a TR with some TH children, not a react "array". Is there a way to not generate react components for each element of a for loop, in the case of static data such as above?

Or should I be assigning a unique key to each TH, even in the case of an unchanging sequence?

[the-kenny]

I think adding :key to the attribute-map of is the correct way of
solving this.

Aaron Iba [email protected] writes:

I'm rendering table headers with om and sablono using a for loop:

(html [:table
       [:tbody
        [:tr (for [k [:last-name :first-name :date]]
               [:th (name k)])]]])

This generates a react warning, "Each child in an array should have a unique \"key\" prop."

But in this case, I think I just want a TR with some TH children, not a react "array". Is there a way to not generate react components for each element of a for loop, in the case of static data such as above?

Or should I be assigning a unique key to each TH, even the case of an unchanging sequence?

---

Reply to this email directly or view it on GitHub:
#40

Moritz Ulrich

[aiba]

@the-kenny, I'm not sure adding :key attributes is the best solution here. Why should it be necessary in the above example but not when writing:

(html [:table
       [:tbody
        [:tr [:th "last-name"] [:th "first-name"] [:th "date"]]]])

Nor is it necessary when writing:

(html [:table
       [:tbody
        (into [:tr]
              (for [k [:last-name :first-name :date]]
                [:th (name k)]))]])

In all 3 ways of writing this, I don't think the :key attribute is relevant to React.

[mbme]

@aiba in the first case you have something like

React.DOM.tbody(null, 
    React.DOM.tr(null, [
        React.DOM.th(null, "last-name"), 
        React.DOM.th(null, "last-name"), 
        React.DOM.th(null, "last-name")
])

(note, that tr receives an array of dom elements as second parameter)

In the second and third case:

React.DOM.tbody(null, 
    React.DOM.tr(null, 
        React.DOM.th(null, "last-name"), 
        React.DOM.th(null, "last-name"), 
        React.DOM.th(null, "last-name")
))

In this case tr receives 4 parameters, not the array.
That's the difference: you should use key attribute if you pass child elements as list.

[aiba]

@mbme thank you for clarifying how the forms translate to React calls. That all makes sense.

Sometimes it is desirable to pass sub-elements to the React.DOM function as an array, and sometimes it is desirable to pass as function arguments (see my example above). (Server-side hiccup does not have this distinction and treats e.g. [:p (list "a" "b" "c")] the same as [:p "a" "b" "c"]).

Interestingly, on the sabolono main README file, the first example [:ul (for [n (range 1 10)] [:li n])] might be better off translated to arguments rather than an array.

I think sablono ought to allow users to specify whether they want arguments or array. One way to accomplish this would be to provide a function sabolono.core/splice that puts a list of elements into arguments. This would allow you to write [:ul (splice (for [n (range 1 10)] [:li n]))] and not get a React missing key warning.

How does that sound?

[benmoss]

Yeah, just was getting this error message as well and was wondering what the solution would be. As was mentioned, hiccup ignores the extra list generated by the for call, it seems to me like Sablono should do the same thing. I don't know of any cases where you'd actually want to pass a data structure like [:ul '([:li 0] [:li 1])].

[sritchie]

I think unwrapping is the right way to go here. I'm also getting this error.

[dustingetz]

You could try unquote-splice

user> [:div (map (fn [v] [:span v]) (range 3))]
[:div ([:span 0] [:span 1] [:span 2])]
user> `[:div ~@(map (fn [v] [:span v]) (range 3))]
[:div [:span 0] [:span 1] [:span 2]]

[jmatsushita]

I fell into this trap as well (in addition to #57) and it is not very elegant. I suppose that if server side hiccup does the smart thing, it would be great if sablono did the same! This also seems to be a duplicate of #48 and #21.

[timothypratley]

I would love to see a fix for this, as it causes me to use (into) a whole bunch. Is this recognized as an issue? Should I submit a pull request?

[r0man]

@timothypratley patch welcome!

[l1x]

Anybody who got here and looking for a workaround:

Change this:

[:table {:class "table"}
      [:thead
        [:tr 
          [:th "origo"]
          (for [w header]  [:th w])]]

To that:

[:table {:class "table"}
      [:thead
        [:tr 
          [:th "origo"]
          (for [w header] ^{:key w} [:th w])]]

Kudos to Frozenlock on #clojurescript on freenode

[blissdev]

@l1x That does not work for me. Perhaps it is because I'm putting it inside a parent that has an option
map already? Or maybe misinterpreting the example.

 [:label 
   [:span "Filters"]
   [:select
     {:value (get context "filter_id")
      :onChange #(handle-change % context "filter_id" owner)}
     [:option "None Selected"]
     (for [[ref option] (:filters meta)]
       ^{:key ref}
       [:option {:value ref} (get option "label")])]]

I still get:
Warning: Each child in an array or iterator should have a unique "key" prop. Check the React.render call using <select>. See https://fb.me/react-warning-keys for more information.

[l1x]

@blissdev I see, I am not sure why it is behaving differently for you.

[Peeja]

As I understand it, Sablono is doing the right thing here, and the warning is warranted. The reason for the warning is that React needs a way to figure out which element is which when the list changes. In the original example in this issue, the for is only used as a syntactic shortcut:

(html
 [:table
  [:tbody
   [:tr
    (for [k [:last-name :first-name :date]]
      [:th (name k)])]]])

Here, the sequence of values is a compile-time constant, [:last-name :first-name :date]. But in my experience, it's far more common to use for with incoming data:

(html
 [:table
  [:tbody
   (for [person people]
     [:tr
      [:td.name (:name person)]])]])

Here, if the first element of people is removed, we want React to remove the first tr from the DOM. Without giving the trs keys, though, it will instead remove the last tr and change the text of the other trs to be the names of the remaining people, which is less efficient and ruins transitions. In this case, we want the warning from React.

I think the best solution is to explicitly opt into splicing for the cases where you're using for as a syntactic convenience, as @aiba suggests.

To those who are comparing Sablono's interpretation with Hiccup's, unless I'm missing something, this isn't a meaningful distinction in Hiccup. It's not a question of just doing what Hiccup does, as Hiccup doesn't compile to React.DOM calls.

[timothypratley]

@Peeja I agree with you. Excellent explanation thanks.

[Peeja]

Having said all that, I do think there's a reasonable case where you don't want the warning:

(html
 [:div
  "submitted on "
  (:submission-date data)
  (when-let [submitter (:submitter data)]
    (list
     " by"
     (om/build submitter-link submitter)))])

Here we're using a list just to group two elements into a single value which when-let can guard. It's not really a set of dynamic children like the examples above which use for. Considering one of the elements is a string, there's no good way to give them keys, nor would that be useful. We really want to splice the elements into the arg list in this case.

[Peeja]

The good news is that these cases appear to always use an explicit invocation of list, whereas the cases where we want React to warn us use constructs like map and for. That means we've got an easy place where we could use a function called splice or group instead:

(html
 [:div
  "submitted on "
  (:submission-date data)
  (when-let [submitter (:submitter data)]
    (sablono/splice
     " by"
     (om/build submitter-link submitter)))])

That could return either a list with metadata or a record which the interpreter could treat differently and splice into the list of children.

We could also ask people to supply metadata themselves, but they'd have to use a vector:

(html
 [:div
  "submitted on "
  (:submission-date data)
  (when-let [submitter (:submitter data)]
    ^:splice
    [" by"
     (om/build submitter-link submitter)])])

That can work, since Sablono only considers a vector a React element if the first element in the vector is a keyword, but it still feels icky to use vectors like this. I recommend the first option.

[Peeja]

Actually, that's not quite right. There's one more use case: passing children into another component/element:

(defn alert [props & children]
  (html
   [:.big-alert-box {:class (:type props)}
    [:i.alert-icon]
    children]))

I'm not sure, but I believe React itself solves this by making this.props.children a special kind of object, and not simply an array.

The upshot is that it would be nice to be able to splice a seq of elements that are already bound to a name. You could actually do that using the proposed sablono/splice:

(defn alert [params & children]
  (html
   [:.big-alert-box {:class (:type params)}
    [:i.alert-icon]
    (apply sablono/splice children)]))

I'm not sure how bizarre that would be.