feat: implement global decorator config #30

taehee-sp · 2024-08-12T08:16:16Z

issue #28

I implemented global decorator support for portfolio.

here is current config api (can be changed)

(defonce app
  (ui/start!
	@@ -31,6 +32,9 @@

    :config
    {:css-paths ["/portfolio/demo.css"]
     :decorators {:reagent    reagent-decorator
                  :reagent-18 reagent-decorator
                  :react-18   react-18-decorator}
   }))

example decorator implementation.

(defn reagent-decorator [child]
  (createElement Provider #js{:value :red}
                 (r/as-element child)))

(defn react-18-decorator [props]
  (createElement Provider #js{:value :red}
                 (.-children props)))

here is consumer example scene.

(defn decorator-consumer-component []
  (let [theme (use-theme)]
    ($ :button {:style {:background (name theme)}}
       "current theme is " (name theme))))

(defscene decorator-consumer-demo
  :title "using global decorator context"
  ($ decorator-consumer-component))

Issue

I just read decorators config from @app atom. I know it's not ideal, but it's difficult for me to understand the whole structure. Can you give me a suggestion please?

TODO

add decorator docs
react-toastify, @tanstack/query decorator example.

cjohansen · 2024-08-15T19:52:54Z

I agree that accessing the app atom this way isn't ideal. I don't think it's necessary to add the decorators to the app state at all, since they are not data. The decorators are more a detail of the rendering mechanics, like the individual framework-specific frameworks. So my suggestion is to go with a more code-driven solution, something like:

(require '[portfolio.reagent :as pr])

(pr/set-decorator! reagent-decorator)

This will just be a convention followed in each adapter, and can be implemented like so:

(ns portfolio.reagent
  ,,,)

(def *decorator* nil)

(defn set-decorator! [decorator]
  (set! *decorator* decorator))

Then:

;; Replace this
(let [decorator (or (:reagent (:decorators @app)) identity)] ,,,)

;; ...with
(let [decorator (or *decorator* identity)] ,,,)

A note on code-style: I see a lot of the namespace forms have been reformatted in various ways. This makes it hard to gauge what changes you've made. Some of them are reasonable enough, but I'd prefer the PR to only have the minimal changes necessary to support your feature, or at the very least separate commits for stuff like formatting. I also prefer using :as when requiring namespaces, and not using:refer, as I find it makes the code much harder follow. I've used :refer-macros sparingly for things like defscene etc, but I don't want Portfolio's internals to use :refer for regular functions.

taehee-sp · 2024-08-28T04:25:41Z

Thank you for the comments. I understand the points.

I will change the PR as you said. (just pr/set-decorator! and use :as instead of :refer)

taehee-sp · 2024-09-19T09:26:33Z

Thank you for your patience, I would like to let you know that PR is ready to review.

taehee-sp · 2024-10-25T02:05:59Z

Just a friendly reminder. @cjohansen

cjohansen

Sorry for the very delayed response!

I like this API a lot better. It still needs to be defined for the remaining non-React adapters, but I can do that. They will probably just be function wrappers.

As I mentioned in the initial comment, there is a lot of unrelated and unnecessary changes to the code, mainly formatting and ordering. I would appreciate if you could revert these, I've highlighted the individual places where I want to keep the original formatting.

cjohansen · 2024-11-02T07:46:07Z

sample/src/portfolio/components/helix.cljs

-            [portfolio.react-18 :refer-macros [defscene]]
-            ["react" :as react]))
+            [portfolio.theme :as theme]
+            [portfolio.react-18 :refer-macros [defscene]]))



Please undo these unrelated changes.

cjohansen · 2024-11-02T07:46:29Z

sample/src/portfolio/components/helix.cljs

+              (render [this]
+                      (when (.. this -props -error)
+                        (throw (js/Error. "BOOOOOM!")))
+                      "Oh, nice!!"))
    ctor))


This code seems to have been reformatted with no real changes. Please undo it.

This one still needs fixing

cjohansen · 2024-11-02T07:47:01Z

sample/src/portfolio/components/reagent.cljs

@@ -4,7 +4,8 @@
  ;; For react versions 18+ use the following:
  ;; This is due to the new API https://www.metosin.fi/blog/reagent-towards-react-18/
  (:require [portfolio.components.reagent.component :as rc]
-            [portfolio.reagent-18 :refer-macros [defscene]]))
+            [portfolio.theme :as theme]
+            [portfolio.reagent :refer-macros [defscene]]))



Why did you change the React version in use here?

it's maybe lsp related mistake. i will revert it.

cjohansen · 2024-11-02T07:47:28Z

sample/src/portfolio/components/rum.cljs

+   [portfolio.theme :as theme]
+            [portfolio.react-18 :refer-macros [defscene]]
+            [rum.core :as rum] ;; If you are using an older version of react use the following:
+            ))



This seems to have been reformatted with no real changes? Please undo it.

i will undo it

cjohansen · 2024-11-02T07:47:48Z

sample/src/portfolio/components/uix.cljs

+  (:require [portfolio.theme :as theme]
+            [portfolio.react-18 :refer-macros [defscene]]
+            [uix.core :refer [$ defui use-state]]
+            [uix.dom]))



This seems to have been reformatted with no real changes. Please undo it.

I will undo it

cjohansen · 2024-11-02T07:51:33Z

src/portfolio/react_utils.cljs

-            [portfolio.adapter :as adapter]))
+            [portfolio.adapter :as adapter]
+            [portfolio.ui.actions :as actions]))
+


Please keep the previous formatting/ordering of this form.

cjohansen · 2024-11-02T07:53:07Z

src/portfolio/react_utils.cljs

-  (let [ctor (fn [])]
+  (let [ctor (fn [])
+        Decorator (or *decorator*
+                      (.-Fragment react))]


If I'm reading this right, this will end up defaulting to a fragment instead of a div as before? Seems like an improvement in itself 😊

cjohansen · 2024-11-02T07:53:36Z

src/portfolio/react_utils.cljs

+                                       react "div" #js {}
+                                       (if (o/getValueByKeys this "state" "error")
+                                         ""
+                                         (:component (get-scene this)))))))


I would prefer if you stuck to the existing formatting here.

cjohansen · 2024-11-02T07:55:39Z

src/portfolio/reagent.cljs

+       (if el
+         (do
+           (rd/render [decorator
+                       (if (fn? component)


This let introduced a lot of diff noise, and decorator is only used once. I suggest inlining (or *decorator* identity) here:

(rd/render [(or *decorator* identity) (if ,,, ))

cjohansen · 2024-11-02T07:56:22Z

src/portfolio/reagent_18.cljs

+  (:require [portfolio.adapter :as adapter]
+            [portfolio.data :as data]
+            [reagent.dom.client :as rdc]
+            [reagent.impl.template :as reagent])
  (:require-macros [portfolio.reagent-18]))


Please keep the original ordering here.

taehee-sp · 2024-11-15T08:36:15Z

it's done! please review @cjohansen :)

cjohansen

Hey, there are still some pending changes from the previous review. Fix 'em and I'll merge it 😊

cjohansen · 2024-11-19T20:32:42Z

sample/src/portfolio/components/helix.cljs

+              (render [this]
+                      (when (.. this -props -error)
+                        (throw (js/Error. "BOOOOOM!")))
+                      "Oh, nice!!"))
    ctor))


This one still needs fixing

cjohansen · 2024-11-19T20:33:20Z

sample/src/portfolio/components/uix.cljs

-      ($ :p "Count: " count)
-      ($ :button {:on-click #(set-count inc)} "Increase"))))
+       ($ :p "Count: " count)
+       ($ :button {:on-click #(set-count inc)} "Increase"))))



This one still needs fixing

cjohansen · 2024-11-19T20:33:32Z

sample/src/portfolio/scenes.cljs

@@ -40,7 +48,7 @@

     :viewport/defaults {:viewport/padding [16]
                         #_#_#_#_:viewport/width 390
-                         :viewport/height 400}
+                             :viewport/height 400}
     #_#_:canvas/layout {:kind :cols


This one still needs fixing

cjohansen · 2024-11-19T20:34:49Z

src/portfolio/react_utils.cljs

+                                 (when-let [actions (:report-render-error (:actions (get-scene this)))]
+                                   (actions/dispatch actions nil {:action/exception error
+                                                                  :action/info (js->clj info)
+                                                                  :action/cause "React error boundary caught error"})))


This is all reformatting I think? Please keep the original formatting

cjohansen · 2024-11-19T20:35:20Z

src/portfolio/react_utils.cljs

+                        react "div" #js {}
+                        (if (o/getValueByKeys this "state" "error")
+                          ""
+                          (:component (get-scene this)))))))


Please maintain the existing formatting here, that will make it easier to see what's actually changing.

taehee-sp · 2024-11-20T00:32:00Z

all done! (I checked my PR diff one by one) @cjohansen please review

feat: implement global decorator config

04fb082

feat: use dynamic var and namespace alias

8bde09d

taehee-sp marked this pull request as ready for review September 5, 2024 01:51

cjohansen requested changes Nov 2, 2024

View reviewed changes

taehee-sp force-pushed the feature/global-decorator-config branch from aa14dd2 to e427b55 Compare November 2, 2024 10:00

taehee-sp requested a review from cjohansen November 2, 2024 10:00

fix: undo unrelated format

d3dc27f

taehee-sp force-pushed the feature/global-decorator-config branch from e427b55 to d3dc27f Compare November 2, 2024 10:01

cjohansen requested changes Nov 19, 2024

View reviewed changes

fix: maintain existing format

5e0dd41

taehee-sp requested a review from cjohansen November 20, 2024 00:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement global decorator config #30

feat: implement global decorator config #30

taehee-sp commented Aug 12, 2024

cjohansen commented Aug 15, 2024

taehee-sp commented Aug 28, 2024

taehee-sp commented Sep 19, 2024

taehee-sp commented Oct 25, 2024

cjohansen left a comment

cjohansen Nov 2, 2024

cjohansen Nov 2, 2024

cjohansen Nov 19, 2024

cjohansen Nov 2, 2024

taehee-sp Nov 2, 2024

cjohansen Nov 2, 2024

taehee-sp Nov 2, 2024

cjohansen Nov 2, 2024

taehee-sp Nov 2, 2024

cjohansen Nov 2, 2024

taehee-sp Nov 2, 2024

cjohansen Nov 2, 2024

cjohansen Nov 2, 2024

cjohansen Nov 2, 2024

taehee-sp Nov 2, 2024

cjohansen Nov 2, 2024

taehee-sp commented Nov 15, 2024

cjohansen left a comment

cjohansen Nov 19, 2024

cjohansen Nov 19, 2024

cjohansen Nov 19, 2024

cjohansen Nov 19, 2024

cjohansen Nov 19, 2024

taehee-sp commented Nov 20, 2024

feat: implement global decorator config #30

Are you sure you want to change the base?

feat: implement global decorator config #30

Conversation

taehee-sp commented Aug 12, 2024

Issue

TODO

cjohansen commented Aug 15, 2024

taehee-sp commented Aug 28, 2024

taehee-sp commented Sep 19, 2024

taehee-sp commented Oct 25, 2024

cjohansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taehee-sp commented Nov 15, 2024

cjohansen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taehee-sp commented Nov 20, 2024