lilactown / helix

A simple, easy to use library for React development in ClojureScript.
Eclipse Public License 2.0
631 stars 52 forks source link

Deprecate $$ #14

Closed lilactown closed 4 years ago

lilactown commented 4 years ago

Deprecate $$ and replace it with a CLJS function $ when it needs to be taken as a value.

darwin commented 4 years ago

I was able to convert all my code to use $ instead of $$ but I had to alter $ macro to be more flexible in dynamic case.

See: https://github.com/binaryage/cljs-react-three-fiber/blob/c79db0863ebba83ec18a2f6fcfaef86ffabf5215/examples/src/app/react_three_fiber/examples/lib/ui.clj#L25

This strategy is following:

  1. try to determine if props was given as static map
  2. if not, use dynamic code to test props argument during runtime: 2a) if it is a cljs map use it as props (and translate them using dynamic code path) 2b) else treat it as first child and pass it properly into create element
darwin commented 4 years ago

btw. My implementation of $ is just a quick and dirty replacement of helix $. I do transaltion to native props always, so there is missing original code doing ^:native heuristics. Also maybe there is a potential double evaluation of type or args. This should be written more carefully.

lilactown commented 4 years ago

@darwin you should be able to avoid doing dynamic testing at runtime by requiring dynamic props be passed to the & key always. This is the intended reason for introducing the & spread-props syntax.

($ :bufferGeometry {& geometry-props})

https://github.com/Lokeh/helix/blob/master/docs/creating-elements.md#dynamic-props

darwin commented 4 years ago

I'm aware of it. Static code path is preferrable. I just didn't rewrite my old code yet. This dynamic testing is there for correctness. If someone writes (merge props other-props) insterad of literal map, it must work (maybe a compile-time warning would be nice).

darwin commented 4 years ago

At least I believe this merge example cannot be determined statically. The expression result must be tested during runtime to figure out if it is a props map or a first child.

lilactown commented 4 years ago

I agree that it's not statically analyzable. I disagree that it must work. The $ macro is meant to have specific semantics to prevent having to branch at runtime.

I think adding a warning when goog.DEBUG is true would help detect errors and guide users to correct usage.

lilactown commented 4 years ago

Closed with 3a6cc3c2b0144a02626dbc482a2137b1d4df74f1 and ddacbc8c90ac25e3e388480829c109ca3d335d23