Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The 0.1.9 causes regression #20

Open
niwinz opened this issue May 12, 2021 · 5 comments
Open

The 0.1.9 causes regression #20

niwinz opened this issue May 12, 2021 · 5 comments

Comments

@niwinz
Copy link
Collaborator

niwinz commented May 12, 2021

Hi @rauhs

At penpot, today we upgraded from 0.1.8 to 0.1.9 and found a little regression introduced in 6f3df03

diff --git a/src/hicada/compiler.clj b/src/hicada/compiler.clj
index 99167cd..60b5159 100644
--- a/src/hicada/compiler.clj
+++ b/src/hicada/compiler.clj
@@ -17,8 +17,13 @@
     [hicada.normalize :as norm]
     [hicada.util :as util]))
 
-(def default-handlers {:> (fn [_ klass attrs & children]
-                            [klass attrs children])
+(def default-handlers {:> (fn
+                            ([_ klass]
+                             [klass {} nil])
+                            ([_ klass attrs & children]
+                             (if (map? attrs)
+                               [klass attrs children]
+                               [klass {} (cons attrs children)])))

When we build props as a javascript object and then pass it as props, the result is that hicada does not identifies that is a map (because at compile time is just a symbol) and passes it together with children.

The workaround is just overwrite :> handler on our codebase. But still wanted open the issue because this commit changes the previous behavior.

Edit: wording.

@rauhs
Copy link
Owner

rauhs commented May 12, 2021

Argh! I def. didn't want that. I actually say that in my inital blog post on medium. That you can use :> to make sure the first argument is interpreted as the props!

I accepted the PR without much thought since I have since moved on from Clojure. I'd certainly accept a PR restoring the old functionality. Even though I don't like flip/flopping on this behavior. :/

Not sure what to do here. Either way sucks.

@rauhs
Copy link
Owner

rauhs commented May 12, 2021

Thinking about it. Gonna leave it like that. Breaking it again would suck for the 0.1.9 users. Better only break it once than twice, right?

But glad that there is workaround, that was actually one bit motivation: Don't provide a macro, just a function and let users configure the behavior as much as they want.

So "won't fix" it is. Thanks for letting me know. And let me know if you'd like commit access to hicada... ;)

@niwinz
Copy link
Collaborator Author

niwinz commented May 12, 2021

After thinking a bit about this, I still think the revert is a better way to proceed. Because the :> handler was the unique approach for passing props dynamically hicada has, with the current situation we have the :> handler and the default one [:somekeyword are synonyms, leaving hicada without any dynamic extensibility/usage. Additionally, the default (previous behavior was documented) and the new one not.

And you of the past was right exposing the building block and not the final api was the right decision.

PD: i'm gladly can take care of the maintenance of hicada, just give me permissions to repository and if you prefer the clojars permission to hicada group (if not, i can publish it under funcool organization also...)

@rauhs
Copy link
Owner

rauhs commented May 13, 2021

You're the boss now, so feel free to revert it :)

I also gave you permission to the clojars, let me know if you need more access.

@niwinz
Copy link
Collaborator Author

niwinz commented May 13, 2021

Nice @rauhs, Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants