Clojurians Log v2

that has changed: https://github.com/clj-commons/secretary

I believe the current merge-with spec was too general and accepted inputs like (merge-with assoc {:a 1} [:a :b]) which isn’t valid in clj + cljs. This PR updates the spec: https://github.com/slipset/speculative/pull/100 I couldn’t come up with an example where we needed associative? instead of map?

oh wow, secretary now works on a cljs version from 2014, thanks mfikes 🙂

I tried updating its dependencies... that's a much bigger project 🙂

so (seqable? nil) was false in v339 of clojurescript, but this got fixed in v439. I suggest not accounting for bugs in spec itself and always assume the newest version.

https://github.com/slipset/speculative/pull/103

or maybe we could do a goog-define/version thing like @mfikes suggested earlier. not sure how that works

we could do something like:

(when  (compare-fn *clojurescript-version ...)
  (defn seqable? [v] (or (nil? v) (clojure.core/seqable? v))))

If you ever need to compare versions you can do something like

(goog.string/compareVersions *clojurescript-version* "1.10.339")

It would be super nice if there were a way to further use this to DCE

This probably has to be a runtime thing, though

So, if you want to use that, you can compose predicates like pos?, nat-int?, etc. with the result to take decisions at runtime.

ah, meanwhile I came up with:

#?(:cljs
   (let [cljs-version (mapv js/parseInt
                            (str/split *clojurescript-version* #"\."))]
     (when (pos? (compare [1 10 439] cljs-version))
       (println "redefining seqable?")
       (defn seqable? [v]
         (or (nil? v)
             (clojure.core/seqable? v))))))

I guess you should also be prepared the possibilty that *clojurescript-version* is nil in case someone is using a non-built ClojureScript (i.e. as a :local/root or Git Dep)

the code of compareVersion sure looks a lot more involved 🙂

but I’ll use that, it’s probably more robust

and account for nil

Yeah, the goog thing can handle weird crap like (goog.string/compareVersions "1b" "1a")

and we cannot assume clojurescript won’t ever adopt this

how about this then?

(ns speculative.core
  (:refer-clojure :exclude [seqable? reduceable?])
  (:require [clojure.spec.alpha :as s]
            [clojure.spec.gen.alpha :as gen]
            [goog.string]))

#?(:cljs
   (when-let [cljs-version *clojurescript-version*]
     (when (pos? (goog.string/compareVersions "1.10.439" *clojurescript-version*))
       (defn seqable? [v]
         (or (nil? v)
             (clojure.core/seqable? v))))))

I guess if you exclude seqable? you may want to define it in the else branches?

yeah, I refer to clojure.core/seqable? in the one other place it’s used

wait that doesn’t make sense, sorry 🙂

(if (pos? (goog.string/compareVersions "1.10.439" *clojurescript-version*))
       (defn seqable? [v]
         (or (nil? v)
             (clojure.core/seqable? v)))
       (def seqable? clojure.core/seqable?))

Even though that happens to work, each of the versions passed to that fn are supposed to be either a string or number

yeah I have a nil condition around this code

Right, then you may not have a seqable? Maybe (and (some? *clojurescript-version*) ...

(ns speculative.core
  (:refer-clojure :exclude [seqable? reduceable?])
  (:require [clojure.spec.alpha :as s]
            [clojure.spec.gen.alpha :as gen]
            #?(:cljs [goog.string])))

#?(:cljs
   (if (and *clojurescript-version*
            (pos? (goog.string/compareVersions "1.10.439" *clojurescript-version*)))
     (defn seqable? [v]
       (or (nil? v)
           (clojure.core/seqable? v)))
     (def seqable? clojure.core/seqable?))
   :clj (def seqable? clojure.core/seqable?))

Yeah

pasted to soon

And, if someone depends on and older Git Dep, perhaps all bets are off

git dep?

(You'd then be forced to check seqable? nil at runtime to detect the behavior).

Yes, if you have a deps.edn based project, you can specify any hash of ClojureScript to use, as a Git Dep, so you could be getting the older seqable? or the newer one, and in both cases *clojurescript-version* is nil. Perhaps that's too much to worry about though.

if they are doing that, they can do a (set! *clojurescript-version* "1.0.0") I guess, maybe with goog-define

Yeah, it is just a general problem. If you have version-dependent code, and there is no version, you are a bit stuck.

right. we assume it is there, if not, we assume >= 439.

Seems reasonable.

git deps are a relative new feature anyway. people who are into this must be willing to upgrade 😉

Interestingly, if *clojurescript-version* were not simply a def but instead a goog-define, you could use identical? in places where you'd like DCE to occur. But, that would seem to be intractable because you would end up with a gigantic cond listing every single version you want to be conditional on.

well, this is only some code at the top of the namespace, not something you pay for each time you run something

I guess your :else branch could fall back to runtime version checking.

runtime version checking?

Right... was just thinkng about the general problem.

it would be even greater if reader conditionals supported clojure versions 😉

or use a C preprocessor 😉

Oh, by that I meant....

(cond
  (identical? *clojurescript-version* "1.10.339") :alpha
  (identical? *clojurescript-version* "1.10.439") :beta
  :else (sometthing with (goog.string/compareVersions *clojurescript-version* "1.10.439"))

With code like the above, I would expect it to compile down to the keyword constructor for :beta in ClojureScript 1.10.439

But, *clojurescript-version* would have to be a goog-define instead of just a lowly def

JIRA? 🙂

Yeah, if you'd like the feature, it might be worth seeing if David is opposed to it.

Well, I don’t think it buys us much performance, this is only a one time check during the loading of the namespace, probably not noticeable

Right, and generally we don't find ourselves writing conditional code based on the version of the compiler in use

I guess that could have been used in secretary as well. iff the goog-define had been there from the start

Yeah, we don't have a time machine

I guess we could still make the optimization in secretary. (defn map-entry [k v] (if 439 (MapEntry. k v nil) (first {k v])))

Yeah, I actually wrote it that way the first time, even using interop to avoid an analysis warning when it compiles the then branch on older versions

Something like (fn [k v] ((.. js/cljs -core -->MapEntry) k v nil))

hah, right

It compiles to the same JavaScript as (fn [k v] (->MapEntry k v nil)), but without involving analysis warnings 🙂

I guess a rat hole on that kind of stuff is whether you can eliminate the call that gets generated (you'd want to actually call the MapEntry ctor in a static way if you wanted it to really fly

would there have been anything against adding a -key and -val prop to vectors so it didn’t break anything?

I don't recall that concept being debated at the time

well, right now we better not, since it would complicate the specs between clj and cljs more 😉

Yeah, if we can get away with it, it is better to align with Clojure and not look back

I think one problem with secretary running on newer version of clojure is that the macro produces bad code for paths that are actually not taken. E.g.:

user=>   (s/defroute #"/([a-z]+)/search" [letters {:keys [query-params]}]
  #_=>       [letters query-params])

Syntax error macroexpanding clojure.core/let at (form-init2041262501447576588.clj:1:1).
{:keys [query-params]} - failed: ident? at: [:bindings :form :map-destructure :keys] spec: :clojure.core.specs.alpha/keys
{:keys [letters {:keys [query-params]}]} - failed: simple-symbol? at: [:bindings :form :local-symbol] spec: :clojure.core.specs.alpha/local-name
{:keys [letters {:keys [query-params]}]} - failed: vector? at: [:bindings :form :seq-destructure] spec: :clojure.core.specs.alpha/seq-binding-form

user=> (macroexpand ' (s/defroute #"/([a-z]+)/search" [letters {:keys [query-params]}]
  #_=>   #_=>       [letters query-params]))
(let* [action__11080__auto__ (clojure.core/fn [params__11081__auto__] (clojure.core/cond (clojure.core/map? params__11081__auto__) (do (clojure.core/let [{:keys [letters {:keys [query-params]}]} params__11081__auto__] [letters query-params])) (clojure.core/vector? params__11081__auto__) (clojure.core/let [[letters {:keys [query-params]}] params__11081__auto__] [letters query-params])))] (secretary.core/add-route! #"/([a-z]+)/search" action__11080__auto__) (fn [& args__11079__auto__] (clojure.core/apply secretary.core/render-route* #"/([a-z]+)/search" args__11079__auto__)))
user=> (let [{:keys [letters {:keys [query-param]}]} {:letters {:query-param 1}}] [letters query-param])

it’s making something like this: (let [{:keys [letters {:keys [query-param]}]} {:letters {:query-param 1}}] [letters query-param]) which was fine on clojure 1.6.0, but in clojure 1.9.0 it’s rejected

A minimal repro of that kind of problem: (let [{:keys [{}]} {}])

yep

I’m not sure how it works, because including it in a project with 439 does “just work”

or maybe it just fails with that one example. yeah, I think that’s it, because I have one specific test failing when I upgrade things

Maybe it is a bad test, with {:keys [query-params]} on the right? (I don't know what defroute would want.)

this one: https://github.com/clj-commons/secretary/blob/master/test/secretary/test/core.cljs#L162

yeah, maybe the test is incorrect.

This subsequent test passes interestingly:

(is (= (s/dispatch! "/abc/search?flavor=pineapple&walnuts=true")
           ["abc" {:flavor "pineapple" :walnuts "true"}])))

I never got that far. When I run lein run-tests when commenting out the failing test, I get:

$ lein run-tests
Compiling ClojureScript...
Compiling ["target/js/test.js"] from ["src/" "test/"]...
Can't open 'Successfully compiled ["target/js/test.js"] in 9.499 seconds.'
Successfully compiled ["target/js/test.js"] in 9.499 seconds.

better luck with tools.deps / cljs.main

if I define the test as:

(s/defroute #"/([a-z]+)/search" {:as params}
      (let [[letters {:keys [query-params]}] params]
        [letters query-params]))

These commits are interesting https://github.com/clj-commons/secretary/commit/b5aecd84e930781ac32a18dd822e937acb63e83f https://github.com/clj-commons/secretary/commit/e9cfae86f6654eb3a8052cc01736bbaa89f2f05f

And code just like that test is in the documentation here (2nd example down) https://github.com/clj-commons/secretary#query-parameters

I guess there should be a secretary channel to discuss this kind of stuff 🙂