I'm running into an issue with rerendering. The code looks roughly like this:

(into [:div]
           (for [item items]
             ^{:key (:id item)}
             [item-ui item]))

arghh I was just typing this 😄

skip the into

The bug is the following:

- View1: the component shows elements A, B - View2: the component rererenders, now it shows elements C, D (so far so good) - Back to View1: the component shows C, A, B (not so good!)

Expectation: - Component shows A, B again Actual behavior: - Component shows C, A, B. That is, it shows a remnant of what was previously shown

In fact if you switch back and forth between View1 and View2, you can reliably add more and more Cs

We've narrowed down the problem to this specific view function

Two funny observations: - If you look at the React devtools in the final state, there you only see A, B, even though C is clearly in the DOM - If you remove the 3rd line (`^{:key (:id item)}`), we see the expected behavior

Anyone have an hunch what the issue could be? Feels like it should be: (a) a React bug, (b) a Reagent bug or (c) a misunderstanding of React keys work on our part

@juhoteperi have you seen this kind of issue before? I may be missing something obvious

What happens if you place the loop directly into [:div ...]? Without realizing the lazy seq, as lilactown suggested. Or maybe even into [:<> ...] if you don't need the div element.

still happens if key is present

yeah, same thing unfortunately with [:div (for [item items] ^{:key (:id item)} [item-ui item])]

the fact that number of DOM nodes and React element instances is different says it’s a bug in React, imo

are your keys not unique?

they are unique

I'm verifying this right now

OK, so I'm printing our keys now, and lo and behold, in View2 we have 1 duplicate key

yep

here’s a repro

My personal rule number one of debugging Reagent stuff: print everything, even if you think it's not needed.

by adding a :key, you are telling React to optimize the re-rendering of the list. but if the keys are not unique, it will confuse React

is this list actually dynamic enough to warrant using keys?

e.g. are you adding and removing individual items from the list? or do you just blow it away and re-render a unique list of items (e.g. A + B, or rendering C + D)

@pesterhazy we also are seeing constant increase of Dom nodes by a random number, could it be that 1 duplicate key is causing this?

the repro I pasted above has that same behavior @roman01la

Oh nice, thanks!

@lilactown that's very helpful, thanks

https://github.com/Lokeh/reagent-list-broken

for ease. neat finding

how do I run shadow?

run npm install in the repo and then npx shadow-cljs watch app

go to

thanks 🙂

So I did know that keys need to be unique, but for extra context a few things confused me - misconception about our own data, assuming that there wasn't a possibility of dupes :face_palm: - the fact that there's no warning or error but instead faulty behavior. This may be because we're seeing this in a prod build (the problem only occurred with prod data)

I'd keep the warning in prod, consequences are super bad

@lilactown your minimal repro case is neat

@pesterhazy & co. Good to see this solved. Extra elements in the dom is usually result from non-unique keys. Reagent doesn't check uniqueness of the keys, as React already does this (`Encountered two children with the same key`), on the dev builds.

Yeah I remembered that there was a check. But the problem was reproducible only in prod, where the checks are elided

Not really surprising. Gnarly bugs often only surface with real-world data

Yeah. Would be cool to have options in Reagent to control checks such as this more closely, as having them enabled by default is not feasible for optimized builds. Maybe at some point.

Makes sense

Quick idea of what might be possible to implement to control checks and also Hiccup transformations (similar to Hicada compiler options):

cljs
;; Take Renderer object as optional parameter for render & as-element.
;; The renderer controls "everything" done by Reagent to convert Hiccup,
;; keep track of RAtoms etc.
;; If different options need to be provided to some components, as-element can be
;; used to select different renderer for a tree.

(def renderer
  (r/renderer
    {:checks {:key-uniquenes true}
     :hiccup-options {:custom-elements {:text-input rn/TextInput}
                      :key-to-element ...
                      :prop-name-fn ...}}))

(defn main []
  [:div
   [:h1 "Example"]
   (r/as-element renderer-2 [foo])])

(r/render renderer [main] el)

that would be pretty great

That’s pretty cool, yeah

In this case though it may not have helped because in prod we would turn that check off as well

Whereas in dev, React would have already caught the issue

As for custom elements, where do you see the advantage over referencing the component function?

The component is more direct than the keyword and with functions, the compiler warns you about typos

I don't see really advantage in such simple case, it was just quick example what could be there. Probably custom elements could be implemented through (Hicada options) transform-fn or create-element option or such.

Having some customizable transform step could be useful for example when porting web code to React native. I recently implemented such case in one project where we had 3000 line form page in web, and I was able to just take the whole code and put in a React-native app by creating function that converts Hiccup forms from DOM elements to RN components. It worked fine, but I had to call the function in several places.

@juhoteperi you mention hicada, which does compile time parsing of hiccup to JS interop. are you thinking of something similar with reagent in the future?

@lilactown Compile time parsing is impossible with Reagent API. If one wants to do it, one can wrap hiccup in Hicada macro calls. In theory I'd like to have an optional macro in Reagent to do the same, but not sure if it is feasible.

yeah, I know it’s impossible with current API just wasn’t sure if that’s where you thinking of migrating towards

Maybe the Renderer thing could be implemented in Cljc and could be shared for macro compile time and runtime.

what’s the biggest blocker to being all compile-time?

reaction tracking?

Hiccup compiler part doesn't need to care about reactions.

You can see the complex part in Hicada code: https://github.com/rauhs/hicada/blob/master/src/hicada/compiler.clj

It has to implement a simple Clojure compiler so it can transform hiccup inside for loops and such forms.

That would break e.g. if using core.match macro: (html [:div (match [a b] [0 0] [:div "0!!"])]) because it doesn't see the hiccup form inside unknown match macro.

right. I think it’s a case of trying to be a bit too clever

Alternative is that one has to wrap nearly every Hiccup form in a macro call: (html [:div (for [i (range)] (html [:div i]))])

yeah

one thing I’ve been experimenting is using tag literals

#hic [:div
      (for [i (range)]
        #hic [:div i])]

it reads a bit nicer

Yeah, if every form was tagged in some way, the macro could just walk the whole form and wouldn't need to care about other macros.

you don’t even need a macro if you use tag literals 🙂

the reader can output equivalent react/createElement calls

Aha, yes, interesting.

the other issue that comes up is dynamic props, which can also potentially be solved with some additional syntax

ex. [:div x y] is ambiguous whether x is props or a child

I’ve put most of this in an experimental lib if you are curious: https://github.com/Lokeh/thump/

it has some bugs… someone told me they couldn’t get it to work in their app a little while ago so YMMV. but it has the general thoughts written out in the README at least 😅

The reader literal idea is nice

the reader literal idea is horrible 😛

lol

thought you might chime in thheller. we shall see!

hiccup is fundamentally syntax, not data. it makes sense to treat it as such IMO

#h/e [:div "foo"] vs (h/e [:div "foo"]) (with h/e being a macro)

don't know why you'd chose a data literal for what macros can do better 😛

IMO the additional parens reduces readability, but that might be subjective

I think people have a pretty regular reaction when you get to the cases where you need to nest: "ugh, too many parens"

to me the syntax just looks "off" with literals

As far as I can tell: Literals: Pros: - 1 symbol less Cons: - Impossible to compose - Impossible to reuse directly without knowing how they're implemented - Impossible to thread with -> and the like Marcos: All opposite.

I'm not sure what you mean by composition / reusing

I don't really get the threading con either

And it's not just 1 less symbol. The effect is 1 level less visual nesting, which I find significant when I read code

Again, subjective

With macros, I can write (a (b (c ...))). With reader literals, I can go take a look at how it's implemented and then maybe somehow achieve the same. My argument is completely invalid if it's possible to implement a reader literal in such a way so that it's possible to use it as #h/e (into [:div] ...) or something like that. I don't understand the argument about visual nesting - the data has the exact same left boundary in terms of characters.

You wouldn't write code like that

My general hypothesis is "hiccup is syntax, not data"

when is syntax yet you don't see us writing #when 😉

hiccup is data as it is. you are only doing the literals because of performance

It should be written literally. 90% of the cases when I see people doing operations like into/concat/etc. It's unreadable and usually do to some hindrance in the hiccup API irself

The nice thing about the approach I'm taking with thump is that it exposes a reader tag, macro, and runtime interpreter

IMO hiccup is 100% data. Especially when we have freedom to define components that can be used in any form other than [component arguments? children*] (e.g. [component arg1 arg2 arg3 children]). I do lots and lots of data processing with hiccup before sending it to Reagent.

So for the cases when you want some more dynamism, you can fall back to macro or runtime. But it's explicit and will stand out

the bad part is the compilation aspect. you can use #h/e technically without a :require. if you do the compiler cannot ensure that the code supporting #h/e is actually "ready"

so technically you are forced to add the :require just to avoid weird compilation results

well you can do whatever you like. I feel like we had this discussion before 🙂

This is avoided with thump by requiring the user to include a hiccup-element var which should be provided by a ns that requires the core tag literal ns

I know 🎃 I like discussing it with you and everyone. It helps me figure things out