hoplon / javelin

Spreadsheet-like dataflow programming in ClojureScript.
803 stars 44 forks source link

Destructuring namespaced keyword keys with cell-let #29

Closed onetom closed 7 years ago

onetom commented 7 years ago

In ClojureScript 1.9.227 & Javelin 3.8.4

(cell-let [{:keys [user/email]} {:user/email "x@y.z"}]
        (js/console.log email))

Fails with

No such namespace: user, could not locate user.cljs, user.cljc, or Closure namespace "" at line 42 src/page/index.cljs Use of undeclared Var user/email at line 42 src/page/index.cljs

(cell-let [{:keys [:user/email]} {:user/email "x@y.z"}]
        (js/console.log email))

Fails with

Use of undeclared Var page.index/email at line 43 src/page/index.cljs

While

(cell-let [{email :user/email} {:user/email "x@y.z"}]
        (js/console.log email))

or

(let [{:keys [user/email]} {:user/email "x@y.z"}]
        (js/console.log email))

Works as expected.

sbondaryev commented 7 years ago

Hello, I’ve looked into the problem a bit: not working code :

(cell-let [{:keys [user/email]} {:user/email "x@y.z"}]
        (js/console.log email))

macroexpand:

(let* [
    vec__22770 (javelin.core/cell-map clojure.core/identity ((javelin.core/formula (clojure.core/fn [{:keys [:user/email]}] [])) {:user/email "x@y.z"}))]
        (js/console.log email))

working code:

(cell-let [{email :user/email} {:user/email "x@y.z"}]
        (js/console.log email))

macroexpand:

(let* [
    vec__22780 (javelin.core/cell-map clojure.core/identity ((javelin.core/formula (clojure.core/fn [{email :user/email}] [email])) {:user/email "x@y.z"}))
    email (clojure.core/nth vec__22780 0 nil)]
        (js/console.log email))

As you can see we lost an email variable in the first case. In my opinion the problem is that we use a custom function bind-syms for a destruction https://github.com/hoplon/javelin/blob/master/src/javelin/core.clj#L28-L30

while let uses destructure function (which is much more complicated in fact) https://github.com/clojure/clojurescript/blob/b6c48c700a788d2b19513170e3231e043afe9752/src/main/clojure/cljs/core.cljc#L743-L753

I think we should reimplement the all the macros which use bind-sums (cell-let, cell-doseq by now) using destructure function.

I ran with this solution for cell-let (but I am not sure about correctness):

(defmacro cell-let [bindings & body]
   (let [distructed-bindings (destructure bindings)        
     celled-bindings (mapcat (fn [[b c]] `(~b (cell= ~c))) (partition 2 distructed-bindings))]
    `(let* [~@celled-bindings] ~@body)))

Another possibility is to copy the destructe function and modify it by adding a wrapper for binding forms

micha commented 7 years ago

These are new syntax introduced with Clojure 1.8.

The (destructure bindings) approach looks good to me. It makes sense to use destructure in macros that introduce new binding forms and walk the bindings. I don't think cell= for example has any issues with let because cell= fully macroexpands its arguments (the Clojure compiler then applies destructure to the bindings itself).

We can add some more cases to the Javelin tests for this.

micha commented 7 years ago

Fixed in #30 :fireworks: