Can't get nested components to work

[jonase]

Is this something that should work or am I missing something:

(defn my-li [props s]
  [:li s])

(defn my-list [props elems]
  [:ul {:style {:background-color "red"}}
   (seq elems)])

(defn ^:export start []
  (r/render-component
    [my-list
      [my-li "foo"]
      [my-li "bar"]]
    (by-id "app")))))

Note that if I use :li directly instead of [my-li ...] everything works as expected.

[holmsand]

Yes, that's probably the part of the API I'm least happy with...

Currently, the "children" parameter is always a vector (or nil, if there are no children), which makes your my-li function return [:li ["foo"]], which is kind of bad...

There are of course several ways to make that work: for example (into [:li] s), [:li (s 0)], [:li (first s)], or [:li (seq s)].

But I totally agree that you should be able to write exactly the code that you've written. The question is just how to achieve that.

One way would be just to accept ["foo"] as valid (the rule would be that vectors starting with a keyword or plain function are components, otherwise they are a series of components or values).

But then the question becomes what such a vector should mean: should it be equivalent to a seq (so you would get React's warning about missing :key values, i.e a "dynamic" list), or should [:li ["foo"]] in effect be flattened to [:li "foo"]?

Or should "children" perhaps be a seq to start with? That has the downside that seqs are a bit slower both to create and to deal with (subvec is crazy fast).

Or (alternative 3): should single children be returned as-is? That would make your example work, but has the downside that children in general become more difficult to deal with.

Or (last alternative): keep things as they are, but provide a less moronic error message than the current assertion...

Any thoughts?

[jonase]

Now that I see what's happening I think I prefer the last alternative, i.e. keep things as they are. As you said, there are many ways to solve this:

(defn my-li [props [s]]
  [:li s])

[holmsand]

Jonas, since you have some experience:

Do you have an opinion on what kind of error message you would have liked to see, to make this behavior clearer?

I'm thinking of something along the line of "["foo"] is not a valid Hiccup form", or somesuch.

And do you think it would be better to just issue a warning (and drop the mal-formed vector), or should an exception be thrown?

[jonase]

Hi Dan.

I can see no reason to not change my program if I make this mistake so I definitely would prefer an exception. Dropping the mal-formed vector is a recipe for more confusion IMO.

"["foo"] is not a valid Hiccup form" is fine. An alternative would perhaps be "["foo"] is not a component"

[ptaoussanis]

I'd vote for throwing an exception anywhere an unambiguously invalid argument is received - be it props, children, etc.

If props always need to be a map or nil, I'd assert that wherever it's relevant. If children always needs to be a vector or nil, I'd assert that. Etc.

Esp. since we're in the client, I'd be happy to fail fast - and this way we can fail with clear error messages "malformed props (expected map or nil)", "malformed children (expected hiccup form or nil)", etc.

[holmsand]

Yes, I agree on the exception-throwing.

The counter-argument that got me thinking is that React sometimes is a bit less than robust when it comes to exceptions. React may get into an undefined state, and then your entire UI pretty much toast. Not a problem during development, but maybe a downside in production (if you hit some obscure bug in your code; not that it ever happens to me).

Maybe a combination of assert + warn-but-continue-to-work with :elide-asserts true would be ideal.

Btw: there isn't a whole lot of other ways to get the Hiccup syntax wrong. Since the props map is optional, it will always be a map if it is there... But maybe we should disallow passing maps as children to native components? That would catch the case where you get the props in the wrong position.

[ptaoussanis]

Maybe a combination of assert + warn-but-continue-to-work with :elide-asserts true would be ideal.

That sounds perfect to me.

Btw: there isn't a whole lot of other ways to get the Hiccup syntax wrong.

Had in mind things like component fns since I recall originally trying to pass a non-map arg to a component fn and being surprised that it was silently failing. Wasn't sure what other cases like that there might be.

[holmsand]

Had in mind things like component fns since I recall originally trying to pass a non-map arg to a component fn and being surprised that it was silently failing. Wasn't sure what other cases like that there might be.

Aaah, that's probably likely to happen.

But also a bit hard to fix: I think it's nice to use the same syntax for "custom" components as for the standard ones, i.e with optional props. So [my-component "x"] is perfectly valid.

Maybe a warning if the arity of the component function is less than 2 and there are children would help? Perhaps something like "Warning: You are ignoring the children"? :)

Don't know how reliable .length of Clojurescript functions is, though.

[ptaoussanis]

So [my-component "x"] is perfectly valid.

Sorry, my understanding on most if this is still very muddy. Where/when would this be valid? I was under the impression that all component fns must be of the form (fn [& [props-map-or-nil children-vec-or-nil this]]) always. Is that not accurate?

Maybe a warning if the arity of the component function is less than 2 and there are children would help? Perhaps something like "Warning: You are ignoring the children"? :)

:-)

[holmsand]

Yes, component fns get [props children this].

But they can be used just like built-in components.

For example:

(defn my-div [props children]
  (into [:div props] children))

will always behave exactly like :div. So if you have

[my-div "x"]

my-div will be called with [nil, ["x"]].

And if it's [my-div {:class "foo"}], it will be called with [{:class "foo"} nil]. Etc.

So the arguments to component functions are normalized, to make handling easier, and work pretty much the same way as this.props and this.props.children in plain React.

This obviously needs to be documented better (or changed?).

[jonase]

Is the this argument necessary? I don't think I've seen it being used anywhere.

If that arg can be dropped it would perhaps be possible for children to be varargs:

(defn my-component [props child-1 child-2 child-3]
  ...)

;; First arg is not a map here so props will be nil
[my-component [:div 1] [:div 2] [:div 3]]

and

(defn my-component [props & children] ...)

[my-component {:some props} [:div 1] [:div 2]]

[ptaoussanis]

Hmm, okay - so would it be correct to rather describe a component fn as: (fn [& [props? children this])? (i.e. the props arg is optional in the same way doc-string is optional for def?

I.e. valid calling signatures are: ([] [props] [children] [props children] [props children this]). Is that right?

(fn [& [props children this]]) incorrectly implies that one needs to provide a props arg if one also wants to provide children.

[ptaoussanis]

@jonase Can't comment about the this (also curious to hear a use case since I haven't had one yet myself) - but I would suggest keeping children as a vector for performance + composibility reasons. &args can become a pain with composition since they require the use of apply, etc. It's also faster to pull arbitrary elements out of a vector than an &arg seq.

[holmsand]

@jonase Well, it is exactly this that is the problem. You would need it to for example get the DOM node with reagent.core/dom-node, or to get to the React-style state (that may be useful to provide something from the lifecycle callbacks).

Of course it might be possible to get at that via some dynamic variable, for example, instead (but it sure is nice to keep everything lexical...).

It would probably be a lot easier to explain the calling conventions if arguments were passed by [my-comp foo] exactly like (my-comp foo), but it might be a little worse to use (and a tiny bit slower).

@ptaoussanis I think the confusion comes from the "normalization" of the props/children parameters. Given [my-comp x], my-comp will be called with non-nil props (i.e the first parameter) if-and-only-if (map? x) is true. Otherwise, children will be [x] if x is not nil (then children would be nil).

The idea is to make props/children match roughly what React would have provided, modulo the html -> hiccup change.

It's just that I'm so terrible at explaining it all...

[jonase]

@ptaoussanis In the general case I completely agree but components are already variadic. It's not like [:ul] takes a fixed number of [:li] arguments... the question is how this should be modeled.

When I'm using reagent the most common mistake for me is that I read [my-component ...] as (my-component ...) and this is also what happened when I opened this issue. I think that is why we should not dismiss varargs just because they are often overused in Clojure code:

;;; Too much of this stuff
(apply my-fn [...])

;;; But this does not even make sense:
[apply my-component [...]]

[jonase]

@holmsand Maybe this could be merged into props. That would mean that props is never nil.

[ptaoussanis]

@holmsand

It's just that I'm so terrible at explaining it all...

No, no - it's just inherently a little subtle. Think it's a credit to the design that most stuff just "works" without necessarily needing to understand what's going on under the covers.

But to confirm my understanding now - are these the correct calling signatures: ([] [props] [children] [props children] [props children this]) where props is a map or nil, and children a vector or nil?

If so, don't think there's anything wrong with that - it's quite sensible + convenient. Would just need to be documented. Again, I'd describe it as (fn [& [props? children this]) to match defn's notation. I'd also assert on the expected types once we've determined which signature is in play.

@jonase Okay! I think I see what you're saying. Can I paraphrase to confirm?

The confusion is coming because usually when we call (my-fn {} :a :b :c), we're expecting my-fn to be defined as (fn [m arg1 arg2 arg3]).

But [my-fn {} :a :b :c] is actually supposing that my-fn is defined as (fn [m [arg1 arg2 arg3]]). So there's a subtle difference between the () and [] calling forms.

Is that correct?

[holmsand]

@jonase That would be a bit expensive, I think (with allocation of a new map for possibly every call). And it would mean that you have to be careful if you pass the props on to someone else.

Also, I really like that the parameters you give to a component are exactly the same as the component gets (if not necessarily in the same order...).

One possible solution would be to make this simply inaccessible to "ordinary component functions", but to provide it to the :render function given to reagent.core/create-class. That would perhaps be a way to keep the simple stuff really simple (and to mark them as pure functions), and to keep the fancy, more object-oriented stuff separate.

[ptaoussanis]

One possible solution would be to make this simply inaccessible to "ordinary component functions", but to provide it to the :render function

For what it's worth, I've actually been using a wrapper myself since I like to have the node available to the render fn:

(defn component
  "Experimental! A Reagent 'component' is a (fn [& [props children this]]) that:
    * May have special metadata for React lifecycle methods.
    * Returns either Hiccup data, or a nested (usu. post-setup) component[1].

  This util makes writing Reagent components a little more convenient:
    (component
      :render (fn [node_ & cmpt-args]) -> Hiccup data, or nested component[1].
      ;; Additional methods optional:
      :did-mount    (fn [node])
      :did-update   (fn [node])
      :will-unmount (fn [node]))
    -> (fn [& cmpt-args]) component.

  [1] Nb note that nested component fns are _only_ called when one or more of
      their input args have actually changed!!"

  ;; Alternative to render node_: (-> % .-target .-value)

  [& lifecycle-methods]
  (let [methods (apply hash-map lifecycle-methods)
        node_   (atom nil)]
    (assert (ifn? (:render methods)) "No :render lifecycle method provided")
    (assert (every? #{:render :did-mount :did-update :will-unmount} (keys methods))
            "Bad lifecycle method key(s) provided")

    (with-meta
      (partial (:render methods) node_) ; [& cmpt-args] -> [node_ & cmpt-args]
      (merge methods

        ;; [rootNode] -> [node] + maintain node_:
        {:component-did-mount
         (fn [node*]
           ;; (debugf "component-did-mount: %s" node*)
           (let [node (reagent/dom-node node*)]
             (reset! node_ node)
             (when-let [mf (or (:component-did-mount methods)
                               (:did-mount methods))]
               (mf node))))}

        ;; [prevProps prevState rootNode] -> [node]:
        (when-let [mf (or (:component-did-update methods)
                          (:did-update methods))]
          {:component-did-update
           (fn [_ _ node*]
             ;; (debugf "component-did-update: %s" node*)
             (let [node (reagent/dom-node node*)]
               (mf node)))})

        ;; [] -> [node]:
        (when-let [mf (or (:component-will-unmount methods)
                          (:will-unmount methods))]
          {:component-will-unmount
           (fn []
             (let [node @node_]
               ;; (debugf "component-will-unmount: %s" node)
               (mf node)))})))))

[jonase]

@ptaoussanis

So there's a subtle difference between the () and [] calling forms.

My point is that [] is not a calling form (its not a function invocation) but I often interpret it as such. I think this will happen to others as well. Maybe the root problem is that components should not be created with defn? Edit: Maybe something like @ptaoussanis component function.

[holmsand]

@ptaoussanis

To confirm my understanding now - are these the correct calling signatures: ([] [props] [children] [props children] [props children this]) where props is a map or nil, and children a vector or nil?

Yes, except that you would never pass this yourself, that is done for you.

So here are the possible alternatives:

[my-comp] -> (my-comp nil nil this) [my-comp {}] -> (my-comp {} nil this) [my-comp {} "a"] -> (my-comp {} ["a"] this) [my-comp {} "a" "b"] -> (my-comp {} ["a" "b"] this) [my-comp "a"] -> (my-comp nil ["a"] this) [my-comp nil "a"] -> (my-comp nil ["a"] this) [my-comp nil nil] -> (my-comp nil [nil] this)

where "a" could be anything that is not a map.

I.e the first child is removed iff it is map?/nil?, and passed as props.

Re: your wrapper: That seems like a good case for using create-class instead (and for me documenting that).

@jonase Yes that is exactly the problem - and as long as you stick to passing a single map to them, they work that way as well. I think there is great value in keeping the defn form of simple components (not least because of testability), but maybe there should be a difference between "simple/pure-function components" and "advanced/oo ones".

[jonase]

One possible solution would be to make this simply inaccessible to "ordinary component functions", but to provide it to the :render function given to reagent.core/create-class.

This sounds like a good idea IMO. If I were to create a branch where I did this and additionally was able to show that children-as-varargs doesn't hurt performance too much would you consider accepting a pull request?

[holmsand]

If I were to create a branch where I did this and additionally was able to show that children-as-varargs doesn't hurt performance too much would you consider accepting a pull request?

Absolutely. But let's think a little while more about what it is we really want.

Right now I think that "simple components" should get exactly the args that are passed to them, and that "advanced components with explicit render function" should get [this props children](instead of [props children this]), to be more similar to the rest of the React-style functions. Or maybe even just [this], and have them call (reagent/props this) and (reagent/children this), a bit like you would in stock React.

However you do, you would have to keep track of which render fn is in use in component.cljs somehow.

But I think performance could be quite good if you avoid apply as much as possible, and special-case say 0-3 args.

Or something entirely different, since I just realized that my brain has quit for today :)

Anyhow, if you want to dig into the code, just ping me whenever the horror you see overcomes you...

[ptaoussanis]

Throwing out another idea:

What if we just swap the component argument order so that component fns are expected to be defined as (fn [this ?props & children])?

Cons:

• This is still a compromise since this becomes a magic argument that folks need to remember to include in their component definitions even though it isn't visible in the [f <args>] form.
• Still not easy to throw warnings for (?).
• It'd be a breaking change.

Pros:

• We retain the ability to define all components as simple fns, which I like - even if we sometimes choose to use an alternative method like a wrapper or create-class.
• The children arg is provided in an &args form, so matches expectations from the [f <args>] form.
• It seems relatively more intuitive/unsurprising that this would be a special case than that children would.
• Slightly better performance than other obvious fn-centric alternatives like requiring component fns to be of the form (fn [{:keys [this props children]}]).
• The very simplest component fns can still be written as (fn []).

Will continue to think about alternatives but it occurs to me that whatever we settle on my end up involving some compromise, so it may turn out to be a case of deciding which is most palatable.

[holmsand]

Yes, that is definitely a possibility.

But I'm starting to think that the "passing this by dynamic variable" might actually work well enough, specially since this is used quite seldom in simple component functions. A typical component using that would have to look something like this:

(defn my-comp [arg1 arg2]
  (let [this (reagent/current-component)]
    [:div "....."]
    ;;; or (fn [arg1 arg2] ... )
))

[ptaoussanis]

I'm starting to think that the "passing this by dynamic variable" might actually work well enough

I'd be quite satisfied with that, I think. @jonase?

Not sure if I'd prefer a dynamic binding or explicit-but-magic arg. Where do you lean Dan? I guess this hinges on how often folks will realistically be using this. If it's fairly frequent, an explicit arg may be better? How often do you tend to use this in practice?

[holmsand]

Yes the dynamic binding is ugly, but maybe one piece of ugliness might be ok :)

Anyhow, right now I'm thinking that the right thing to do is to have two ways of providing rendering:

• a simple component function, defined by defn as now, but except that [comp-fn args...] and (comp-fn args...) become equivalent in most cases (modulo performance). I think the terminology is quite nice: a component function is simply a function that can be called as a component...
• a React-style render method, by giving {:render (fn [this]} to create-class

The :render version would have to access props and children with the accessors (reagent/props this) and (reagent/children this), analogous to this.props and this.props.children in plain React.

I'm also thinking that adding a (reagent/argv this) accessor, that would return the entire hiccup form passed to the component, might be a good idea. That way, you could implement simple component-calling in terms of the more orthodox render function efficiently, as well as the other way around.

That way, the choice of whether to use component function or render method becomes mainly one of style and taste, but they would be equivalent in terms of functionality. Presumably, you would use render methods if you rely heavily on this access, and component functions if you don't use this at all, or only need the occasional dom-node.

Cons:

• dynamic ugliness and associated foot-shooting risk
• a bit bigger API
• reagent.core/set-props and reagent.core/replace-props would have to go, since they wouldn't make sense for component functions with arbitrary arguments. They could be replaced with e.g set-argv, though. And I don't think they are particularly useful, anyway.
• the semantics of the default should-component-update become a bit tricky. Right now I think the right thing to do would be to keep the current semantics, i.e. treat maps in the first position specially, and compare the rest of the args with a simple identical?. This is just a heuristic anyway.
• signatures of some of the lifecycle callbacks would have to change, replacing props and children with the more general argv.

Now we have

{:get-initial-state (fn [this])
:component-will-receive-props (fn [this new-props])
:should-component-update (fn [this old-props new-props old-children new-children])
:component-will-mount (fn [this])
:component-did-mount (fn [this])
:component-will-update (fn [this new-props new-children])
:component-did-update (fn [this old-props old-children])
:component-will-unmount (fn [this])
:render (fn [props children this])}

which would become

{:get-initial-state (fn [this])
:component-will-receive-props (fn [this new-argv])
:should-component-update (fn [this old-argv new-argv])
:component-will-mount (fn [this])
:component-did-mount (fn [this])
:component-will-update (fn [this new-argv])
:component-did-update (fn [this old-argv])
:component-will-unmount (fn [this])
:render (fn [this])}

Some more pros:

• really simple components could be made a bit faster, especially if they only take a single argument, since you don't have to wrap that argument in a map.
• component construction would become a little faster and leaner (we would just have to pack a single vector into the js props React expects).
• should-component-update could become a tiny bit faster as well.

The last two should lead to an overall performance gain, I guess, since they are pretty much the only thing called for the common case where a component is unchanged between renders.

Anyway, I might have missed something fundamental, so I'll also keep mulling over this a while...

[jonase]

@holmsand

I'm struggling a bit with the implementation details. I think I understand the basic approach but lot's of things are not clear yet.

• What is the base abstraction? I'm confused about the fact that reagent.impl.component/create-class seems to return a component function and not React component. That would suggest that the "component function" is the base abstraction, but component functions are also "compiled" with create-class in impl/template. There is also a circular dependency (reagent.core -> reagent.impl.template -> reagent.core) which makes it more difficult for me to see what's built on top of what.
• The [props? children] heuristics is implemented both in create-class and when compiling hiccup forms. Maybe the more primitive abstraction (which I still suspects is create-class) should require that all arguments passed to the :render function must be specified? i.e. :render (fn [this props vec-of-children] ...)

I'll keep on digging. It's all very interesting stuff!

[holmsand]

@jonase Great questions and observations!

create-class is modeled on (and wraps) React.createClass, so it returns a constructor function that in turn returns a React component. That React component could be passed directly to render-component, etc.

In other words, if you have this:

(def foo (r/create-class {:render
                          (fn [props]
                            [:div "Foo says: " (:foo props)])}))

(defn main []
  [:div
   "Testing"
   (foo {:foo "hello"})
   [foo {:foo "hello"}]])

these two ways of calling foo would produce identical results.

But that "convenience constructor" is never used if you use Hiccup forms all the way, its just there to match what React's createClass produces.

Instead, in Hiccup forms, as-class sees that the foo function (for example) has a .-cljsReactClass field, containing a function that is called directly with "cljsProps", "cljsChildren", and "cljsLevel", packed into the js props that React's constructor expects. That is much faster than calling the convenience constructor directly, since it has to use varargs.

The circular dependency: Yes, that bugs me tremendously. But I couldn't figure out how to get rid of it...

In template.cljs, the code that interprets Hiccup forms has to construct new React classes whenever it encounters a plain function it hasn't seen before. Hence the call to create-class. On the other hand, components defined by component.cljs need to ask template.cljs to interpret the Hiccup forms it returns from render...

I'm sure there is an elegant way to get around the circularity, its just that I haven't seen the light yet :)

So, long story short, the props? children fiddling in create-class is never used in "normal" code, it's just there in case you'd want to integrate Reagent code with plain old React code. Come to think of it, it could/should probably be replaced with a call to as-component to avoid the duplication. Then it would become

(defn create-class
  [body]
  (assert (map? body))
  (let [spec (cljsify body)
        res (.createClass React spec)
        f (fn [& args]
            (tmpl/as-component (apply vector res args)))]
    (set! (.-cljsReactClass f) res)
    (set! (.-cljsReactClass res) res)
    f))

A lot cleaner. Even if it adds to the circularity...

Anyway, thanks for doing the digging! Even if rewriting the guts of pretty much everything is an absolutely terrible first task to take on :)

If you don't mind I'll probably have a go myself at rewriting args-passing as outlined above, just to get a feel for how it would look, but I'll post something here as soon as I get to it.

And keep this kind of great comments coming!

[jonase]

@holmsand Thanks. That clears things up for me

If you don't mind I'll probably have a go myself at rewriting args-passing as outlined above, just to get a feel for how it would look, but I'll post something here as soon as I get to it.

Sure. Go for it! You have a much better understanding on how this stuff works.

re: circular dependencies.

I'm not sure about this specific case but if the namespace dependencies form a graph instead of a tree the reason is often that the names are partitioned incorrectly into namespaces. I've encountered at least two different cases:

• Too many small namespaces. Mutually dependent functions should live in the same namespace. Many small namespaces are not necessarily easier to maintain than a few bigger once IMO.
• A name is simply placed in the wrong namespace.

Looking at react.impl.template there are functions as-component and wrap-component etc. which might be a sign that these functions should live in react.impl.component? Maybe impl.template should only handle the parsing of hiccup forms into render functions which are passed (via reagent.core) to impl.component which is responsible for creating the actual React component.

[holmsand]

Ok, so I had a stab at implementing the ideas from a couple of comments ago. The results are here:

https://github.com/holmsand/reagent/tree/anyargs

It does look rather promising, I think. Performance seems to be at least as good as before.

But the whole thing may require some more thinking about whether this is the Right Thing To Do...

[jonase]

@holmsand

I tried to use the anyargs branch and it works pretty well for me. No big surprises yet.

Here's a link if you're interested in trying (or improving!) it: https://github.com/jonase/reagent/commit/b25facab6322a720fddedee8d008af2078a6817a

[holmsand]

Oh, that looks very pretty! The anyargs thing does feel rather natural, doesn't it?

And it would be great to have an svg example, as well, so anytime you feel like it, fire a pull request away!

Just a couple of quick observations:

Mouse-up is difficult. As it is now, you won't get any mouse-ups if they happen outside the :svg element. Don't know if it is worth fixing...

It would be easy to do, though. Given require of [goog.events :as events] and import of [goog.events EventType], you could

  (doto js/window
    (events/listen EventType.MOUSEUP on-mouse-up)
    (events/listen EventType.MOUSEMOVE on-mouse-move))

Not sure the extra closure-related ugliness is worth it, though.

Also there's a reagent.core/next-tick [f] these days, that you could use instead of requestAnimationFrame. It will fall back to vendor-prefixed rafs or setTimeout, and looks a little prettier :)

[jonase]

The anyargs thing does feel rather natural, doesn't it?

Yes. I like it.

And it would be great to have an svg example, as well, so anytime you feel like it, fire a pull request away!

Sure. I'd like to make a more impressive demo with some more interactivity. Kind of a mini-port of http://geogebraweb.appspot.com/.

[holmsand]

Sure. I'd like to make a more impressive demo with some more interactivity. Kind of a mini-port of http://geogebraweb.appspot.com/.

Sounds good! Maybe a good idea though to not make it too impressive, so that it feels unapproachable for the reader...

[jonase]

Agreed. I can send you a PR once I've cleaned up the code a bit. And if I find time I can make a more advanced app as a seperate project

[holmsand]

0.4.0 allows arbitrary arguments to child components, and adds (reagent.core/current-component) that can be used together with reagent.core/props and reagent.core/children to replicate the previous behaviour.

[ptaoussanis]

Only (finally!) got a chance to play with this now - really enjoying the new API: a very welcome improvement.

Cheers :-)