cjohansen / portfolio

Eclipse Public License 1.0
236 stars 14 forks source link

Defscene does not pass :params to Reagent form-2 components #14

Closed Reefersleep closed 10 months ago

Reefersleep commented 11 months ago

I've illustrated this issue in this "PR" in a fork of the sasha repository: https://github.com/cjohansen/sasha/pull/2

Form 2 components are functions that return functions. They are typically used for setting up state or for other on-first-mount side effects.

An example (from the PR):

(ns sasha.components.button-scenes
  (:require [portfolio.reagent :refer-macros [defscene]]
            [sasha.components.button :refer [Button]]))

(defn form-1-component [{:keys [text] :as m}]
  [:button m text])

(defn form-2-component [_]
  (let [add-num #(str % (rand-int 100))]
    (fn [{:keys [text] :as m}]
      [:button m (add-num text)])))

(defscene form-2-button
  :params {:text "Click it"
           :href "#"}
  form-2-component)

(defscene form-1-disabled-button
  :params {:text "Click it"
           :disabled true}
  form-1-component)

(defscene form-1-button-with-text-added
  :params {:text (str "Click it" (rand-int 100))}
  form-1-component)

The two defscene calls to form-1-component both display fine, but the call to the form-2-component acts as if :params was never passed.

image
Reefersleep commented 11 months ago

So, I tried reading up on the Portfolio scene rendering code as well as the reagent rendering code. Loads of indirection in both libs, had my head swirling :D And lots of implementation details, hard to keep up on a cursory read.

I thought I'd hit a wall of no understanding when one of the lines I'd blown past in Portfolio came back to me. I think the issue might be that the :params are used in a closure with the reagent component, either here: https://github.com/cjohansen/portfolio/blob/ab8744ff16823bb9b2885a02cf9486fb8bc13a71/src/portfolio/ui/canvas.cljs#L77

Or here: https://github.com/cjohansen/portfolio/blob/ab8744ff16823bb9b2885a02cf9486fb8bc13a71/src/portfolio/ui/scene.cljs#L48

This would have the effect of the call to render being in the shape of (something like)

(reagent.dom/render el [(partial my-component params)])

rather than

(reagent.dom/render el [my-component params])

And I think this is why params is not passed on to the inner fn; reagent does not see params, and so, cannot pass them off to the inner fn when recursively resolving my-component.

Forgive me if I'm completely off; this is a brain dump late at night before I forget everything :)

cjohansen commented 11 months ago

Thanks for digging around! I now understand what the problem is, will try to find a fix.

cjohansen commented 11 months ago

I took a look at this, and I'm not sure how this really should work... First off, this does work:

(defn form-2-button [_]
  (let [add-num #(str % (rand-int 100))]
    (fn [{:keys [text]}]
      [:button.button (add-num text)])))

(defscene form2-component
  :params {:text "I am a Reagent \"form 2\" button component"}
  [params]
  [form-2-button params])

Portfolio supports a few different ways to create scenes. This is the one you used:

(defscene form2-component
  :params {:text "I am a Reagent \"form 2\" button component"}
  scene-f)

In this case, Portfolio expects scene-f to be a reference to a function that accepts one or two arguments, and that will return a component. In other words, Portfolio doesn't really expect to find a component here, but a regular function. In order to support this being a reagent component, portfolio.reagent/defscene would need a separate implementation that would to behave differently from the other ones. I'm skeptical if that's a good idea.

Would the example above work for you?

Reefersleep commented 11 months ago

I took a look at this, and I'm not sure how this really should work... First off, this does work:

(defn form-2-button [_]
  (let [add-num #(str % (rand-int 100))]
    (fn [{:keys [text]}]
      [:button.button (add-num text)])))

(defscene form2-component
  :params {:text "I am a Reagent \"form 2\" button component"}
  [params]
  [form-2-button params])

Portfolio supports a few different ways to create scenes. This is the one you used:

(defscene form2-component
  :params {:text "I am a Reagent \"form 2\" button component"}
  scene-f)

In this case, Portfolio expects scene-f to be a reference to a function that accepts one or two arguments, and that will return a component. In other words, Portfolio doesn't really expect to find a component here, but a regular function. In order to support this being a reagent component, portfolio.reagent/defscene would need a separate implementation that would to behave differently from the other ones. I'm skeptical if that's a good idea.

Would the example above work for you?

Yes, definitely, it's great :)

I must say that when I discovered the "reference-a-component-directly" method, I was wondering "But how does it know how to pass the parameters in the correct manner?" E.g. passing [my-component a b c] rather than [my-component [a b c]]. Perhaps you've got a :parameters convention for all relevant parameters-passing scenarios, but it did tickle my "can this really work"-spider-sense :)

I think the difference between the two calling methods should be highlighted in the documentation.

Reefersleep commented 10 months ago

I'll close this issue as the

(defscene my-scene
  :params :x
  [component params])

solution is fine by me.