<< Back to previous view

[CLJS-2472] ES6 Iterators should use IIterable if possible Created: 15/Jan/18  Updated: 15/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance


 Description   

Motivation:

ES6 Iterables: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Iteration_protocols

Use in React: https://github.com/facebook/react/blob/30dac4e78de02fb427ee82013160ae875128d7a2/packages/react/src/ReactElementValidator.js#L195-L204

(defn es6-iterator**
  [coll]
  (if (implements? IIterable coll)
    (let [it (-iterator ^not-native coll)]
      #js{:next (fn []
                  (if ^boolean (.hasNext it)
                    #js{:value (.next it), :done false}
                    #js{:value nil, :done true}))})
    ;; Fallback to naive first/next iterator:
    (ES6Iterator. (seq coll))))

;; Tests can use round trip:
(es6-iterator-seq (es6-iterator (hash-map 0 1 2 3)))

(defn es6-iter-consume
  [it]
  (while (not (.-done (.next it)))))

(dotimes [_ 3]
  (let [xs (vec (range 3000))
        runs 1000]
    (simple-benchmark []
      (es6-iter-consume (es6-iterator xs)) runs)
    (simple-benchmark []
      (es6-iter-consume (es6-iterator** xs)) runs)))

About a 4x speedup in Chrome. Also note that much less garbage is produced.






[CLJS-2471] ChunkedSeq should implemented ICounted Created: 15/Jan/18  Updated: 15/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

ChunkedSeq in clojure implements Counted, should do the same in CLJS. Implementation is straight forward.






[CLJS-2470] ArrayChunk.reduce doesn't honor end param Created: 15/Jan/18  Updated: 15/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Ex:

(reduce max (array-chunk #js[0 1 2 3] 0 1))
;; => 3

Currently not an issue since end is always arr.length for our usage of ArrayChunk, but since it's a public API users might pass a different end param.

This can easily be fixed after CLJS-2466 which properly implements the reduce for ArrayChunk.






[CLJS-2469] ChunkedCons chunk-next returns empty seq instead of nil Created: 13/Jan/18  Updated: 19/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: A. R
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2469.patch    

 Description   

There is a bug in ChunkedCons. In Clojure ChunkedCons (correctly) always calls seq in chunked-next. In CLJS it's not done. But since ChunkedCons has to be lazy it pretty much always gets an (empty) lazy seq as the "more" part.

Bug:

(-> (map inc (vec (range 64)))
    seq
    chunk-next
    seq
    chunk-next)

Returns and empty sequence instead of nil. This hasn't surfaced yet since nothing calls chunk-next on a ChunkedCons (yet).



 Comments   
Comment by A. R [ 13/Jan/18 7:26 AM ]

Found another bug that surfaced: Current implementation calls -seq on more which could be nil and this would blow up. So the patch also make a tiny change to -next also just calling seq on more. Pretty straight forward.

Comment by David Nolen [ 19/Jan/18 3:20 PM ]

This patch needs a test.





[CLJS-2468] Implement reduce for ChunkedCons Created: 12/Jan/18  Updated: 12/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Forked from CLJS-2466

Title says it all.






[CLJS-2467] Small PVs are never chunked Created: 12/Jan/18  Updated: 13/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-2467.patch    

 Description   

A PersistenVector that has <=32 elements and is seq'ed will return an IndexedSeq. Though IndexedSeq always fails the chunked-seq? check.

This means a:

(def xv (vec (range 32)))
(reduce + (map inc xv))

is about 4x slower than a map over a vector with 33 elements.

Options:
1. Return a ChunkedCons with the "rest" set to nil in PersistentVector.seq()
2. Implement IChunkedSeq for IndexedSeq:

(extend-type IndexedSeq
    IChunkedSeq
    (-chunked-first [x] x)
    (-chunked-rest [x] ())
    IChunkedNext
    (-chunked-next [x] nil)
    IChunk
    (-drop-first [coll]
      (if-some [n (-next coll)]
        n
        (throw (js/Error. "-drop-first of empty chunk")))))

I think option #2 is better since IndexedSeq is used quite a bunch throughout the code base, so the chunking will also kick in for many other code paths.



 Comments   
Comment by A. R [ 12/Jan/18 10:20 AM ]

Note:

This is different from Clojure (which does not consider an IndexedSeq a ChunkedSeq) since we use IndexedSeq a lot more where Clojure often uses a ChunkedSeq. For instance:

(apply (fn [& a] (type a)) [1 2 3 4 8])

will return IndexedSeq in CLJS but ChunkedCons in CLJ.

Since these IndexedSeq's will be passed around wildly in a normal CLJS app it makes sense to extend them as a IChunkedSeq since otherwise all these will never be chunked and get a slow first/next treatment.





[CLJS-2466] Faster reduce for lazy-seqs Created: 12/Jan/18  Updated: 13/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2466.patch    

 Description   

Lazy seqs that are built from vectors/chunked-seqs can sometimes take a slow first/next reduce.

Observation:

(def xs (vec (range 3000000)))

(time (reduce max xs)) ;; #1: 130ms, (reference)
(time (reduce max (lazy-cat xs))) ;; #2: 130ms, just as fast
(time (reduce max 0 (lazy-cat xs))) ;; #3: 500ms, 4x slower!!

;; Double them with concat and they're not 2x slower but 10x slower:
(time (reduce max (lazy-cat xs xs))) ;; #4: 1200ms
(time (reduce max 0 (lazy-cat xs xs))) ;; #5: 1200ms

Explanation for #3: The problem is that seq-reduce when called without init will properly call reduce again and take a fast path. With init given it won't ever escape to a faster reduce but will stick to first/next.

Note: In Clojure they scale "properly" (first 3 are ~45ms, last 2 are ~110ms).

The reason is that Clojure properly escapes to a fast path:

https://github.com/clojure/clojure/blob/2b242f943b9a74e753b7ee1b951a8699966ea560/src/clj/clojure/core/protocols.clj#L131-L143

An improved seq-reduce could look like this:

(defn seq-reduce
  ([f coll]
   (if-let [s (seq coll)]
     (reduce f (first s) (next s))
     (f)))
  ([f val coll]
   (loop [val val, coll (seq coll)]
     (if coll
       (if (chunked-seq? coll)
         (if (implements? IReduce coll)
           (reduce f val coll)
           (let [nval (reduce f val (chunk-first coll))]
             (if (reduced? nval)
               @nval
               (recur nval (chunk-next coll)))))
         (let [nval (f val (first coll))]
           (if (reduced? nval)
             @nval
             (recur nval (next coll)))))
       val))))

This reduces the times for #3: 160ms, #4: 380ms, #5: 380ms.

This is an RFC, since I'm not 100% certain about the implemenation. Need to be careful to not blow the stack here...

Questions:
1. Should ChunkedCons implement IReduce? I think so.



 Comments   
Comment by A. R [ 12/Jan/18 1:59 AM ]

Timings with advanced compilation:

CHROME:
"OLD"
#1: "Elapsed time: 21.870000 msecs"
#2: "Elapsed time: 25.485000 msecs"
#3: "Elapsed time: 134.900000 msecs"
#4: "Elapsed time: 317.155000 msecs"
#5: "Elapsed time: 313.225000 msecs"
"NEW"
#1: "Elapsed time: 23.290000 msecs"
#2: "Elapsed time: 19.445000 msecs"
#3: "Elapsed time: 20.075000 msecs"
#4: "Elapsed time: 102.895000 msecs"
#5: "Elapsed time: 100.430000 msecs"
"OLD"
#1: "Elapsed time: 19.585000 msecs"
#2: "Elapsed time: 19.475000 msecs"
#3: "Elapsed time: 87.805000 msecs"
#4: "Elapsed time: 303.340000 msecs"
#5: "Elapsed time: 291.680000 msecs"
"NEW"
#1: "Elapsed time: 20.760000 msecs"
#2: "Elapsed time: 19.690000 msecs"
#3: "Elapsed time: 18.960000 msecs"
#4: "Elapsed time: 101.385000 msecs"
#5: "Elapsed time: 101.290000 msecs"

FIREFOX:

"OLD"
#1: "Elapsed time: 7.880000 msecs"
#2: "Elapsed time: 7.820000 msecs"
#3: "Elapsed time: 69.460000 msecs"
#4: "Elapsed time: 197.800000 msecs"
#5: "Elapsed time: 209.300000 msecs"
"NEW"
#1: "Elapsed time: 7.380000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 8.100000 msecs"
#4: "Elapsed time: 110.640000 msecs"
#5: "Elapsed time: 89.960000 msecs"
"OLD"
#1: "Elapsed time: 6.520000 msecs"
#2: "Elapsed time: 7.360000 msecs"
#3: "Elapsed time: 106.920000 msecs"
#4: "Elapsed time: 252.300000 msecs"
#5: "Elapsed time: 249.800000 msecs"
"NEW"
#1: "Elapsed time: 7.360000 msecs"
#2: "Elapsed time: 6.940000 msecs"
#3: "Elapsed time: 6.880000 msecs"
#4: "Elapsed time: 99.380000 msecs"
#5: "Elapsed time: 53.220000 msecs"
Comment by A. R [ 12/Jan/18 2:14 AM ]

Impact on truly (unchunked) lazy-seqs is neglibile (hard to measure due to garbage collection causing a lot of variance).

Comment by A. R [ 12/Jan/18 10:22 AM ]

New ticket for reduce for ChunkedCons: CLJS-2468

Comment by A. R [ 13/Jan/18 7:00 AM ]

Depends on CLJS-2469 for tests to pass. Patch is attached.





[CLJS-2465] Using var-args as arguments name in variadic function overrides first argument Created: 11/Jan/18  Updated: 11/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The following returns (nil "b"):

((fn [& var-args]
   var-args) "a" "b")

var-args should to be shadowed by default.






[CLJS-2463] js calls are mistakenly treated as higher order invokes Created: 10/Jan/18  Updated: 10/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Code that invokes js/ a lot (like Sablono) will currently treat the JS invokes as higher order invokes and bind all arguments in a let beforehand. This generates a ton of IIFEs which are all unnecessary.

Fix:

HO-invoke? (and (boolean *cljs-static-fns*)
                        (not fn-var?)
                        (not (js-tag? f))
                        (not kw?)
                        (not (analyzed? f)))


 Comments   
Comment by A. R [ 10/Jan/18 2:11 PM ]

Actually this has a much larger scope. Currently also Math,goog (and more?) are treated like HO invokes. This also affects compile time when using these a lot since it generates a lot of new {{let}}s. Will probably also be affected by JS modules? So we should do a more careful check here to avoid this.

Comment by A. R [ 10/Jan/18 2:45 PM ]

Another one: all-values predicate should also include keyword? check, no reason to bind the argument when passing keywords.

To illustrate what the current code gen looks like:

(defn foooooooo
  [x y]
  (x :fooo :bar)
  (goog/mixin (js/fooo x) (js/bar y))
  (Math/floor (js/fooo x) (js/fooo x))
  (goog.mixin (js/fooo x) (js/fooo x)))

Generated:

foooooooo = (function (x,y){

var G__21625_21633 = cljs.core.cst$kw$fooo;
var G__21626_21634 = cljs.core.cst$kw$bar;
(x.cljs$core$IFn$_invoke$arity$2 ? x.cljs$core$IFn$_invoke$arity$2(G__21625_21633,G__21626_21634) : x(G__21625_21633,G__21626_21634));

var G__21627_21635 = fooo(x);
var G__21628_21636 = bar(y);
goog.mixin(G__21627_21635,G__21628_21636);

var G__21629_21637 = fooo(x);
var G__21630_21638 = fooo(x);
Math.floor(G__21629_21637,G__21630_21638);

var G__21631 = fooo(x);
var G__21632 = fooo(x);
return goog.mixin(G__21631,G__21632);

});

All of them don't need to bind the passed arguments.

Obviously in this simple case GCC will just inline everything and the cost is 0. But if any of these calls happen inside a function, like {{let [x (the-call...] ...}} then an IIFE is created that isn't broken up by GCC.





[CLJS-2461] Minor shadowing intricacies Created: 09/Jan/18  Updated: 09/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler


 Description   

Examples that currently fail:

(ns a-b.core)
(defn doit [])

(ns a-b.other
  (:require [a-b.core :as core]))

(let [a_b 2]
  (core/doit)) ;; FAILS, a_b isn't shadowed properly

;; Related:
(let [a_b 1, a-b 2] a_b) ;; Collide, returns 2!
(let [a-b 1, a_b 2] a-b) ;; Collide, returns 2!
(let [goog "2"] (js-obj goog 2) ;; Fails. Overrides goog!

Given the shadowing logic of the compiler is pretty expensive and further dynamically computing the munged name of each "first namespace segment" could potentially slow down the compiler by a factor of 2x (or more) we should probably re-introduce this approach:

https://github.com/clojure/clojurescript/commit/1c71ab3bff27dc4a099b06e122ec3fdf5a2a8ba8

Ie, maintaining a set of first ns segments and do s simple set lookup in shadow-depth. We should probably only store munged names and do the lookup after munging to fix the above bugs. We should also add "goog".






[CLJS-2459] Compiler should emit warning about namespace and/or non-existent var Created: 05/Jan/18  Updated: 06/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Michiel Borkent Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

Made project as described in quickstart.

hello-world/core.cljs

(ns hello-world.core
  (:require [hello-world.foo]))

(enable-console-print!)

(println "Hello world!")
(hello-world.foo/foo)
;; no warning

hello_world/foo.cljs

(ns hello-world.foo2)
(defn foo []
  (println "o no"))

Note that `foo.cljs` has a namespace name that is not consistent with the file structure.

I expect
1) a warning about `(:require [hello-world.foo])` not being able to find the namespace, and/or
2) a warning about `(hello-world.foo/foo)` being a non-existent var.






[CLJS-2454] Inconsistent macro require behaviour Created: 02/Jan/18  Updated: 03/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It is my understanding that a namespace has to :require-macros itself to have its macros accessible automatically but this no longer seems to be accurate.

Given this minimal example

;; src/demo/lib.cljs
(ns demo.lib)

;; src/demo/lib.clj
(ns demo.lib)

(defmacro foo [& args]
  `(prn ::macro))

;; src/demo/test.cljs
(ns demo.test
  (:require [demo.lib :as lib]))

(lib/foo 1 2 3)

I would expect a warning for lib/foo since demo.lib has no :require-macros for its macros and the demo.test did not use :include-macros or :refer-macros.

Compiling this via the build API does in fact produce the expected warning but only iff the demo.lib CLJ namespace was not required anywhere else.

WARNING: Use of undeclared Var demo.lib/foo at line 5 src/demo/test.cljs
;; build.clj

(require '[cljs.build.api :as cljs])
;; (require 'demo.lib) ;; requiring this here removes the warning

(cljs/build
  "src"
  {:output-dir "out"
   :optimizations :none
   :verbose true})

Enabling the (require 'demo.lib) in the build.clj causes the warning to disappear and the code uses the macro properly.

Some popular libraries (eg. devcards) do not self-require but still work if the macro file was at least required somewhere else.

Is this intended behaviour or just accidental?



 Comments   
Comment by David Nolen [ 03/Jan/18 8:44 AM ]

It's accidental in a sense - the macro resolution is simple to the point of being a bit naive. I'm not sure if we should do anything about it at this point. It would probably cause quite a bit of breakage and little benefit.





[CLJS-2446] with-meta doesn't work for variable arguments functions Created: 19/Dec/17  Updated: 19/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently with-meta doesn't work for varargs fns (which works in Clojure):

(apply (with-meta #(-> %&) {}) 0 1 2 (range 30))

Given JS is so flexible I'd propose the following implementation of meta fn:

(defn meta-fn
  [f m]
  (let [new-f (goog/bind f #js{})]
    (goog/mixin new-f f)
    (specify! new-f IMeta (-meta [_] m))
    new-f))

The goog/bind creates a copy, and the goog/mixin is just for performance reasons (copy any IFn protocol or applyTo).

Benefits:

  • Slightly faster
  • Much simpler and smaller code (the 20 arities of MetaFn are emitted 3 times (.call, .apply, prototype.IFn)
  • Works with varargs.





[CLJS-2444] Refering replaced cljs.core macro symbols Created: 17/Dec/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Hlöðver Sigurðsson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Lumo 1.8.0-beta, Clojurescript 1.9.946



 Description   

Overwriting symbols that are defined in cljs.core works fine and prints an expected warning. One case where this fails is when a symbol that was replaces was originally a macro and it's invoked without namespace qualification. An example:
code
;;;;;;; file a ;;;;;;
(ns file.a)
(defn + [& vals]
(prn "Called!")
(apply cljs.core/+ vals))
;;;;;;; file b ;;;;;;
(ns file.b
(:require [file.a :refer [+]]))
(+ 1 2 ) ;; Doesn't print "Called!"
(file.a/+ 1 2) ;; Prints "Called!"
(var ) => file.a/
(`+ 1 2) ;; Prints "Called!"
code

The same symbol `` alone by itself in file b calls cljs.core/ despite it being referred and all var resolution resolves to file.a/+. This seems not to be the case when overwriting functions from cljs.core, for example overwriting the function `map` works fine.
code
;;;;;;; file a ;;;;;;
(ns file.a)
(defn map [f coll]
(prn "map called!")
(cljs.core/map f coll))
;;;;;;; file b ;;;;;;
(ns file.b
(:require [file.a :refer [map]]))

(map inc [1 2 3]) => "map called!" (2 3 4)
code






[CLJS-2442] `set` and `vec` performance enhancements Created: 14/Dec/17  Updated: 14/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Alex Gunnarson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

On the master branch (and in all previous versions of ClojureScript I've seen), the version of `set` and `vec` in core.cljs do not do an instance check before creating a new set or vector. That is, `set` does not check (as in the Clojure 1.8 version) whether the input is already a set before creating a brand-new one, and `vec` does not check (as in the Clojure 1.8 version) whether the input is already a vector before creating a brand-new one. In addition, `set` appears not to support reducibles (`vec` does not share this defect).

The enhancement is short and simple: the addition of one `instance?` check per function.

Relates to, but does not duplicate, CLJS-1784.






[CLJS-2440] Re-running watch on CLJS source using native modules results in JS error Created: 12/Dec/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Dave Kozma Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None
Environment:

Mac OS X 10.13.1, ClojureScript 1.9.968-gebdaf6c0 running on Java SE 1.8.0_144, built with Maven 3.5.2



 Description   

I'm trying to use a native NPM module in my CLJS code (in this case, date-fns), and the initial time I try to watch, it works, however on successive watches, it fails with the following error in my console:

base.js:1357 Uncaught Error: Undefined nameToPath for date_fns
    at visitNode (base.js:1357)
    at Object.goog.writeScripts_ (base.js:1369)
    at Object.goog.require (base.js:706)
    at index.html:8

To reproduce, I've created a build script as such:

(require '[cljs.build.api :as b])

(b/watch "src"
  {:main 'npm.core
   :output-to "out/npm.js"
   :output-dir "out"
   :npm-deps {:date-fns "1.29.0"}})

as well as a minimal test file:

(ns npm.core
  (:require [date-fns :as dfn]))

(def now (js/Date.))
(.write js/document "Today is " (dfn/format now "dddd"))

I'm also testing with a local `package.json` file rather than using `install_deps true` - my `package.json` (generated by CLJS) looks like this:

{
  "dependencies": {
    "@cljs-oss/module-deps": "^1.1.1",
    "date-fns": "^1.29.0"
  }
}

When I run:

npm install
java -cp ../clojurescript/target/cljs.jar:src clojure.main watch.clj

everything works fine, however if I CTRL+C and run the same exact command again, I get the error outlined above.

However, if (and only if) I delete the `out` directory and run the command a third time, it works again.

Please let me know if you need any other details.



 Comments   
Comment by Dave Kozma [ 12/Dec/17 11:47 AM ]

Sorry, seemed to mess up on the title and can't seem to edit. The title should be "Re-running watch on CLJS source using native modules results in JS error"

Comment by Dave Kozma [ 12/Dec/17 11:49 AM ]

Additional environment info:

npm -v 5.5.1
node -v v8.4.0

Comment by Andy Parsons [ 22/Dec/17 12:13 PM ]

Noticed that this was switched to priority 4, but this issue is preventing us from using npm dependencies entirely. What does the lower priority reflect?





[CLJS-2439] FileNoteFoundException thrown when have URL configed in :foreign-libs/:file Created: 12/Dec/17  Updated: 12/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908, Next
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Shark Xu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, compiler
Environment:

OS: Ubuntu 16.04



 Description   

When I have URL configed in :foreign-libs/:file (via build option or deps.cljs), I get FileNoteFoundException.

URL example: https:/raw.githubusercontent.com/mrdoob/three.js/dev/examples/js/controls/OrbitControls.js

Error Log:

51. Unhandled java.io.FileNotFoundException
6 /home/shark/git/apps/https:/raw.githubusercontent.com/mrdoob/three.js/dev/examples/js/controls/OrbitControls.js
7 (No such file or directory)
8
9 FileInputStream.java: -2 java.io.FileInputStream/open0
10 FileInputStream.java: 195 java.io.FileInputStream/open
11 FileInputStream.java: 138 java.io.FileInputStream/<init>
12 io.clj: 238 clojure.java.io/fn
13 io.clj: 235 clojure.java.io/fn
14 io.clj: 69 clojure.java.io/fn/G
15 io.clj: 165 clojure.java.io/fn
16 io.clj: 69 clojure.java.io/fn/G
18 io.clj: 102 clojure.java.io/reader
19 io.clj: 86 clojure.java.io/reader
20 RestFn.java: 410 clojure.lang.RestFn/invoke
21 closure.clj: 422 cljs.closure/eval7540/fn
22 js_deps.cljc: 121 cljs.js_deps$eval2733$fn_2756$G2724_2765/invoke
23 closure.clj: 418 cljs.closure/eval7540/fn
24 js_deps.cljc: 121 cljs.js_deps$eval2733$fn_2756$G2724_2765/invoke
25 closure.clj: 1723 cljs.closure/write-javascript
26 closure.clj: 1699 cljs.closure/write-javascript
27 closure.clj: 1748 cljs.closure/source-on-disk
28 closure.clj: 1743 cljs.closure/source-on-disk
29 closure.clj: 2604 cljs.closure/build/fn
30 core.clj: 2646 clojure.core/map/fn
31 LazySeq.java: 40 clojure.lang.LazySeq/sval
32 LazySeq.java: 49 clojure.lang.LazySeq/seq
33 Cons.java: 39 clojure.lang.Cons/next
34 RT.java: 688 clojure.lang.RT/next
35 core.clj: 64 clojure.core/next
36 core.clj: 3033 clojure.core/dorun
37 core.clj: 3039 clojure.core/doall
38 closure.clj: 2604 cljs.closure/build
40 closure.clj: 2507 cljs.closure/build
41 api.clj: 205 cljs.build.api/build
42 api.clj: 189 cljs.build.api/build
43 api.clj: 192 cljs.build.api/build
44 api.clj: 189 cljs.build.api/build
45 REPL: 62 apps.cljs-rt-browser/-main



 Comments   
Comment by Shark Xu [ 12/Dec/17 8:30 AM ]

Add my config code:

1{:foreign-libs
2 [{:file "https://raw.githubusercontent.com/mrdoob/three.js/dev/examples/js/controls/OrbitControls.js"
3 :provides ["cljsjs.three-orbitcontrols"]
4 :requires ["cljsjs.three"]
5 }
6 {:file "https://raw.githubusercontent.com/dataarts/dat.gui/master/build/dat.gui.js"
7 :provides ["cljsjs.dat-gui"]
8 }
9 {:file "https://raw.githubusercontent.com/mrdoob/three.js/dev/examples/js/ParametricGeometries.js"
10 :provides ["cljsjs.three-parametricgeometries"]
11 :requires ["cljsjs.three"]
12 }
13
14 ]
15 }





[CLJS-2438] Self-host: script/test-self-parity abends sometimes Created: 08/Dec/17  Updated: 08/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Mike Fikes
Resolution: Unresolved Votes: 0
Labels: bootstrap


 Description   

Sometimes the script/test-self-parity tests terminate early with

Testing with Node

Testing self-host.test
WARNING: double already refers to: cljs.core/double being replaced by: cljs.user/double at line 1

Ran 33 tests containing 206 assertions.
0 failures, 0 errors.
x

Full transcript:

$ script/test-self-host
Analyzing file:/Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/core.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/core.cljs, elapsed time: 2548.700649 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/inspect.cljs to builds/out-self/cljs/tools/reader/impl/inspect.cljs
Compiling builds/out-self/cljs/tools/reader/impl/inspect.cljs, elapsed time: 32.942144 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/string.cljs, elapsed time: 67.24495 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/utils.cljs to builds/out-self/cljs/tools/reader/impl/utils.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/env.cljc, elapsed time: 7.9361 msecs
Compiling builds/out-self/cljs/tools/reader/impl/utils.cljs, elapsed time: 52.978771 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/reader_types.cljs to builds/out-self/cljs/tools/reader/reader_types.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/set.cljs, elapsed time: 63.896603 msecs
Compiling builds/out-self/cljs/tools/reader/reader_types.cljs, elapsed time: 65.685336 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/errors.cljs to builds/out-self/cljs/tools/reader/impl/errors.cljs
Compiling builds/out-self/cljs/tools/reader/impl/errors.cljs, elapsed time: 47.949386 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/impl/commons.cljs to builds/out-self/cljs/tools/reader/impl/commons.cljs
Compiling builds/out-self/cljs/tools/reader/impl/commons.cljs, elapsed time: 26.952248 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader.cljs to builds/out-self/cljs/tools/reader.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map/base64.cljs, elapsed time: 4.063721 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map/base64_vlq.cljs, elapsed time: 15.036982 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/source_map.cljs, elapsed time: 125.606578 msecs
Compiling builds/out-self/cljs/tools/reader.cljs, elapsed time: 213.744986 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/tools.reader-1.1.0.jar!/cljs/tools/reader/edn.cljs to builds/out-self/cljs/tools/reader/edn.cljs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/clojure/walk.cljs, elapsed time: 15.457761 msecs
Compiling builds/out-self/cljs/tools/reader/edn.cljs, elapsed time: 87.207366 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/reader.cljs, elapsed time: 69.046241 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/tagged_literals.cljc, elapsed time: 11.240989 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/gen/alpha.cljs, elapsed time: 260.765109 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/spec/alpha.cljs, elapsed time: 947.753769 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/analyzer.cljc, elapsed time: 1202.162467 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/nodejs.cljs, elapsed time: 3.422987 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/pprint.cljs, elapsed time: 1300.527383 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/compiler.cljc, elapsed time: 403.391288 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/test.cljs, elapsed time: 143.651189 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/cljs/cljs/js.cljs, elapsed time: 1554.368126 msecs
Compiling src/test/self/self_host/test.cljs, elapsed time: 1493.203925 msecs
Compiling /Users/mfikes/Projects/clojurescript/src/main/clojure/cljs/core.cljc, elapsed time: 7060.124722 msecs
Compile sources, elapsed time: 14324.531439 msecs
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/base.js to builds/out-self/goog/base.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/string/string.js to builds/out-self/goog/string/string.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/debug/error.js to builds/out-self/goog/debug/error.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/dom/nodetype.js to builds/out-self/goog/dom/nodetype.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/asserts/asserts.js to builds/out-self/goog/asserts/asserts.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/array/array.js to builds/out-self/goog/array/array.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/object/object.js to builds/out-self/goog/object/object.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/structs/structs.js to builds/out-self/goog/structs/structs.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/functions/functions.js to builds/out-self/goog/functions/functions.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/math.js to builds/out-self/goog/math/math.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/iter/iter.js to builds/out-self/goog/iter/iter.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/structs/map.js to builds/out-self/goog/structs/map.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/uri/utils.js to builds/out-self/goog/uri/utils.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/uri/uri.js to builds/out-self/goog/uri/uri.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/integer.js to builds/out-self/goog/math/integer.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/string/stringbuffer.js to builds/out-self/goog/string/stringbuffer.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/reflect/reflect.js to builds/out-self/goog/reflect/reflect.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/math/long.js to builds/out-self/goog/math/long.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/util.js to builds/out-self/goog/labs/useragent/util.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/platform.js to builds/out-self/goog/labs/useragent/platform.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/engine.js to builds/out-self/goog/labs/useragent/engine.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/crypt/crypt.js to builds/out-self/goog/crypt/crypt.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/labs/useragent/browser.js to builds/out-self/goog/labs/useragent/browser.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/useragent/useragent.js to builds/out-self/goog/useragent/useragent.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/useragent/product.js to builds/out-self/goog/useragent/product.js
Copying jar:file:/Users/mfikes/Projects/clojurescript/lib/google-closure-library-0.0-20170809-b9c14c6b.jar!/goog/crypt/base64.js to builds/out-self/goog/crypt/base64.js
Applying optimizations :simple to 55 sources
Optimizing with Google Closure Compiler, elapsed time: 17638.02089 msecs
Optimizing 55 sources, elapsed time: 18894.849293 msecs
Testing with Node

Testing self-host.test
WARNING: double already refers to: cljs.core/double being replaced by: cljs.user/double at line 1

Ran 33 tests containing 206 assertions.
0 failures, 0 errors.
x





[CLJS-2433] HTTP server in cljs.browser.repl doesn't serve files with extensions other than specified in ext->mime-type Created: 04/Dec/17  Updated: 26/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Yegor Timoshenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-2433-fallback-to-send-static-use-host-to-determ.patch     Text File 0001-CLJS-2433-use-JVM-to-deduce-MIME-type-send-files-via.patch     Text File 0002-CLJ-2433-send-fils-via-send-and-close.patch    

 Description   
$ cat deps.edn
{:deps {org.clojure/clojure {:mvn/version "1.9.0-RC2"}
        org.clojure/clojurescript {:mvn/version "1.9.946"}}
$ touch hello.xml
$ clojure -m cljs.browser.repl &
$ curl localhost:9000/hello.xml
<html><body><h2>Page not found</h2>No page /hello.xml found on this server.</body></html>

Adding an entry to cljs.repl.browser/ext->mime-type superficially fixes that.

1) Currently used MIME type identification is very simplistic and doesn't return correct MIME types for most files. Instead, this should be delegated to the host:

(ns cljs.repl.browser
  (:import [java.nio.file Files Paths]))

(defn get-content-type [path]
  (Files/probeContentType (Paths/get path (into-array String []))))

2) Since both files and sockets are abstractions over bytes, cljs.repl.server/send-and-close should be agnostic to file encoding and just send whatever is in the file. Currently this is handled via cljs.repl.browser/mime-type->encoding.

I will prepare a patch by tomorrow.



 Comments   
Comment by Yegor Timoshenko [ 25/Dec/17 10:16 AM ]

I disagree with this being a minor defect. Currently most files are not served by the built-in HTTP server, which means that it's impossible to develop an application that has files other than HTML/CSS/JS.

Comment by David Nolen [ 26/Dec/17 7:03 AM ]

The builtin webserver is not an important component of ClojureScript. It exists to aid tutorials.

Comment by Yegor Timoshenko [ 26/Dec/17 7:10 AM ]

I like it much more than figwheel because it's simpler, and it seems to be feature-complete other than this bug. Also, this is the first in-browser REPL that comes to mind when using new `clojure` tooling.

Also, previous version of this patch didn't correctly count Content-Length, changed arity of send-and-close function, and didn't work for favicons. I'm attaching a new patch that this time should be complete and cause zero breakage.

New patch is called 0001-CLJS-2433-use-JVM-to-deduce-MIME-type-send-files-via.patch in the attachments above. Link: https://dev.clojure.org/jira/secure/attachment/17586/0001-CLJS-2433-use-JVM-to-deduce-MIME-type-send-files-via.patch

Here's a GitHub link (for code review/explanations): https://github.com/hackberryhike/clojurescript/commit/ea8336da3a779cb5982875d243dc6d40abd0d3ba

Comment by Yegor Timoshenko [ 26/Dec/17 7:18 AM ]

Also, if you don't like something about this patch, do tell me and I'll fix it right away. I'm very interested in getting this merged.





[CLJS-2424] Improve compiler munge performance Nr 2 Created: 28/Nov/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJS-2424.patch    
Patch: Code

 Description   

This is similar to CLJS-2065 and further improves the performance by avoiding reduce and using a key iterator instead.

Results for a large CLJS project with lots of namespaces are:

  • Initial compile (cold) Old: 11.4s New: 11.2s
  • First full recompile: Old: 6.8s New: 5.9s
  • After a few full recompiler (warmed up JVM): Old: ~6.1s New: 5.1s

lein count:

Ext Files Lines of Code Nodes
cljs 138 23388 424745


 Comments   
Comment by David Nolen [ 22/Dec/17 2:24 PM ]

No difference for compiling ClojureScript tests, I will give a try with something else.





[CLJS-2412] Consuming npm module Created: 22/Nov/17  Updated: 22/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Jan Břečka Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When trying to :require the uuid (https://www.npmjs.com/package/uuid) npm package:

(:require ["uuid/v4" :as uuid-v4])

compiled javascript is failing on Reference error: require is not defined. It's caused because the library itself requiring two files:

var rng = require('./lib/rng');
var bytesToUuid = require('./lib/bytesToUuid');

While the require('./lib/bytesToUuid') call is properly replaced with "goog provide", the require('./lib/rng') is not touched at all. This is part of the package.json the library provides:

"browser": { "./lib/rng.js": "./lib/rng-browser.js", "./lib/sha1.js": "./lib/sha1-browser.js" }

The problem is that this "browser" alternative is ignored.



 Comments   
Comment by Thomas Heller [ 22/Nov/17 12:14 PM ]

There are 2 pending pull requests for the Closure Compiler related to this.

https://github.com/google/closure-compiler/pull/2622
https://github.com/google/closure-compiler/pull/2627





[CLJS-2410] Side-effectful assignments are wrongly eliminated by GCC under :advanced Created: 22/Nov/17  Updated: 08/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

CLJS 1.9.946



 Description   

DEMO REPO: https://github.com/tonsky/closure-elimination-bug

Given this code:

(ns bug.core
  (:require
    [goog.object :as gobj]))


(defn get-obj []
  (js/document.querySelector "body"))


(defn ^:export aaa []
  (js/console.log "aaa start")
  (gobj/set (get-obj) "className" "+++aaa+++")
  (js/console.log "aaa end"))


(defn ^:export bbb []
  (js/console.log "bbb start")
  (set! (.-className (get-obj)) "+++bbb+++")
  (js/console.log "bbb end"))


(defn ^:export ccc []
  (js/console.log "ccc start")
  (aset (get-obj) "className" "+++ccc+++")
  (js/console.log "ccc end"))

gets compiled to

// Compiled by ClojureScript 1.9.946 {:static-fns true, :optimize-constants true}
goog.provide('bug.core');
goog.require('cljs.core');
goog.require('cljs.core.constants');
goog.require('goog.object');
bug.core.get_obj = (function bug$core$get_obj(){
return document.querySelector("body");
});
bug.core.aaa = (function bug$core$aaa(){
console.log("aaa start");

var G__4263_4266 = bug.core.get_obj();
var G__4264_4267 = "className";
var G__4265_4268 = "+++aaa+++";
goog.object.set(G__4263_4266,G__4264_4267,G__4265_4268);

return console.log("aaa end");
});
goog.exportSymbol('bug.core.aaa', bug.core.aaa);
bug.core.bbb = (function bug$core$bbb(){
console.log("bbb\u00A0start");

bug.core.get_obj().className = "+++bbb+++";

return console.log("bbb end");
});
goog.exportSymbol('bug.core.bbb', bug.core.bbb);
bug.core.ccc = (function bug$core$ccc(){
console.log("ccc start");

(bug.core.get_obj()["className"] = "+++ccc+++");

return console.log("ccc end");
});
goog.exportSymbol('bug.core.ccc', bug.core.ccc);

(fine so far). When we compile that with :optimizations :advanced, assignments are lost:

l("bug.core.aaa", function() {
  console.log("aaa start");
  return console.log("aaa end");
});
l("bug.core.bbb", function() {
  console.log("bbb start");
  return console.log("bbb end");
});
l("bug.core.ccc", function() {
  console.log("ccc start");
  return console.log("ccc end");
});

Expected: assignments are not eliminated.

Might be a GCC bug, but I can't build a minimal use-case to reproduce. Upgrading to [com.google.javascript/closure-compiler-unshaded "v20171023"] did not helped (same behaviour).



 Comments   
Comment by A. R [ 22/Nov/17 8:23 AM ]

Duplicate: CLJS-2031

TLDR: Add `:closure-warnings {:check-types :warning}` to your compiler options fixes this (which I've had on all my projects since that ticket)





[CLJS-2409] Eliminate cljs.core/divide Created: 21/Nov/17  Updated: 14/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2409-2.patch     Text File CLJS-2409-3.patch     Text File CLJS-2409.patch    
Patch: Code and Test

 Description   

The cljs.core/divide inlining macro was introduced to work around an inability to specify cljs.core//.

For example: https://github.com/clojure/clojurescript/blob/d45012273029bd5df3973ea716394f368e1c44cc/src/main/cljs/cljs/core.cljs#L2593

This can now evidently be removed and thus avoid things like CLJS-594 and divide showing up as a completion in IDEs, etc.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 9:38 PM ]

CLJS-2409-2.patch adds unit tests.

Comment by Mike Fikes [ 14/Dec/17 2:34 PM ]

Adding re-baselined patch.





[CLJS-2406] ^:export not supported on defrecord Created: 21/Nov/17  Updated: 24/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Víctor M. Valenzuela Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

^:export doesn't appear to have an effect on defrecord - I only see Closure emitting the original symbols for def and defn.

I guess that similarly, deftype, defprotocol etc also would be reasonable targets.

In any case, it would be good to explicitly state what works and what does currently not (advanced-compilation.adoc), so one doesn't find himself blindly trying out things.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 7:00 AM ]

You can export protocol methods. Proposed site change: https://github.com/clojure/clojurescript-site/pull/148

Comment by Mike Fikes [ 21/Nov/17 9:20 AM ]

^:export works on (and only on) Vars.

Protocol methods are Vars, so the above site change would help clarify how to export them.

The remaining Vars under consideration would be the map and positional factory functions and constructors that are synthetically generated for defrecord and deftype.

Perhaps a change could be accepted such that (defrecord ^:export Foo []) would export map->Foo, ->Foo, and Foo. And likewise (deftype ^:export Bar []) would export ->Bar and Bar.

Comment by Mike Fikes [ 22/Nov/17 6:10 AM ]

Workaround: Use goog/exportSymbol:

(ns my-ns.core)

(defrecord Foo [])
(goog/exportSymbol "my_ns.core.map__GT_Foo" my_ns.core.map__GT_Foo)
(goog/exportSymbol "my_ns.core.__GT_Foo" my_ns.core.__GT_Foo)
Comment by Víctor M. Valenzuela [ 23/Nov/17 3:34 PM ]

Hi Mike,

thanks so much for clarifying and taking care of this one! Appreciated.

^:export works on (and only on) Vars.

This would be an excelent piece of knowledge to include in the documentation IMO. Why not tell the whole story in a single place?

The original concern that made me open this issue was logging/debugging.

Say an application has a series of singleton defrecord instances, as part of a 'component' system (Sierra style).

Then, for logging, I might be tempted to (js/console.log (type the-instance)), so I can identify e.g. a buggy component.

But without language support, one is unable to get that (or something similar) to work without lots of boilerplate, or perhaps a hacky macro.

Would it be possible to make (type some-instance-of-a-defrecord-or-deftype) print the original name under advanced compilation?

Comment by Mike Fikes [ 24/Nov/17 9:16 AM ]

Hi Victor,

Thanks for the ticket intent clarification. ^:export is not for debugging, but is for making un-renamed symbols available to JavaScript. In fact, it doesn't change renamed and optimized JavaScript, but instead simply makes additional un-renamed aliases available which point into optimized / renamed code. (Thus, generally, debugging deals with the renamed / optimized code, even if there are un-renamed ^:exports sitting off to the side.)

Fortunately, :pseudo-names exists to help with debugging optimized code: https://clojurescript.org/reference/compiler-options#pseudo-names

For your example involving logging the type of the instance, instead of, for example [Function: ze] (under Node.js), you would get [Function: $my_ns$core$Foo$$] if :pseudo-names is enabled.

I've proposed revisions to the site: https://github.com/clojure/clojurescript-site/pull/150

If :pseudo-names satisfies the needs of this ticket, perhaps this ticket can be closed.





[CLJS-2402] Duplicate source files passed to Closure Compiler causes ModuleLoader#resolvePaths to throw "Duplicate module path after resolving" Created: 19/Nov/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Corin Lawson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: modules, npm-deps

Attachments: Text File CLJS-2402.patch     File patch    
Patch: Code

 Description   

I have a small (not minimal) repo here: https://github.com/au-phiware/boot-parsets

The repo includes a es6 module and a cljs file that both depend on a node module, which produces the following error.

Writing main.cljs.edn...
Compiling ClojureScript...
• main.js
                                       java.lang.Thread.run                  Thread.java:  748
         java.util.concurrent.ThreadPoolExecutor$Worker.run      ThreadPoolExecutor.java:  617
          java.util.concurrent.ThreadPoolExecutor.runWorker      ThreadPoolExecutor.java: 1142
                        java.util.concurrent.FutureTask.run              FutureTask.java:  266
                                                        ...                                   
                        clojure.core/binding-conveyor-fn/fn                     core.clj: 2022
                              adzerk.boot-cljs/compile-1/fn                boot_cljs.clj:  160
                                   adzerk.boot-cljs/compile                boot_cljs.clj:   72
                                          boot.pod/call-in*                      pod.clj:  413
                                                        ...                                   
org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke  ClojureRuntimeShimImpl.java:  102
org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke  ClojureRuntimeShimImpl.java:  109
                                                        ...                                   
                                          boot.pod/call-in*                      pod.clj:  410
                                      boot.pod/eval-fn-call                      pod.clj:  359
                                         clojure.core/apply                     core.clj:  657
                                                        ...                                   
                         adzerk.boot-cljs.impl/compile-cljs                     impl.clj:  151
                                       cljs.build.api/build                      api.clj:  205
                                         cljs.closure/build                  closure.clj: 2595
                             cljs.closure/handle-js-modules                  closure.clj: 2496
                            cljs.closure/process-js-modules                  closure.clj: 2389
                            cljs.closure/convert-js-modules                  closure.clj: 1680
                com.google.javascript.jscomp.Compiler.parse                Compiler.java:  995
          com.google.javascript.jscomp.Compiler.parseInputs                Compiler.java: 1731
      com.google.javascript.jscomp.deps.ModuleLoader.<init>            ModuleLoader.java:   92
com.google.javascript.jscomp.deps.ModuleLoader.resolvePaths            ModuleLoader.java:  276
java.lang.IllegalArgumentException: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
        clojure.lang.ExceptionInfo: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
    from: :boot-cljs
        clojure.lang.ExceptionInfo: Duplicate module path after resolving: /home/corin/Projects/Demos/boot-parsets/node_modules/d3/d3.js
    line: 33

Run `boot cljs` to reproduce the issue.

The patch attach removes duplicates from the set of input source files before they are preprocessed. With this patch the repo compiles correctly.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 6:05 PM ]

Hi Corin,

  1. Have you signed the CA? (Your name doesn't appear on https://clojure.org/community/contributors)
  2. Can you provide a minimal repro that doesn't employ downstream tooling? (Bug-filing details are at https://clojurescript.org/community/reporting-issues)
Comment by Corin Lawson [ 19/Nov/17 6:27 PM ]

1. I have signed the CA (I did it after filing this bug).
2. Not a problem, I'll get on to that later today. Is it okay to link to the project on GitHub, or should I upload a tarball?

Comment by Mike Fikes [ 19/Nov/17 6:48 PM ]

Hi Corin, it is not OK to link to GitHub, and any repro should not make use of any downstream tooling (Leiningen, Boot, etc.)—this means that a repro would ideally depend only on the shipping cljs.jar, executable like the examples at Quick Start https://clojurescript.org/guides/quick-start

Comment by Mike Fikes [ 19/Nov/17 6:59 PM ]

Hey Corin, you may want to submit the patch using the instructions at https://clojurescript.org/community/patches (your current patch won't apply using git am, which is what I suspect David uses in the end.

Comment by Mike Fikes [ 19/Nov/17 7:24 PM ]

lein test is failing when applying patch

FAIL in (commonjs-module-processing) (module_processing_tests.clj:54)
processed modules are added to :libs
expected: (= {:foreign-libs [], :ups-foreign-libs [], :libs [(test/platform-path "out/src/test/cljs/reactJS.js") (test/platform-path "out/src/test/cljs/Circle.js")], :closure-warnings {:non-standard-jsdoc :off}} (env/with-compiler-env cenv (closure/process-js-modules {:foreign-libs [{:file "src/test/cljs/reactJS.js", :provides ["React"], :module-type :commonjs} {:file "src/test/cljs/Circle.js", :provides ["Circle"], :module-type :commonjs, :preprocess :jsx}], :closure-warnings {:non-standard-jsdoc :off}})))
  actual: (not (= {:foreign-libs [], :ups-foreign-libs [], :libs ["out/src/test/cljs/reactJS.js" "out/src/test/cljs/Circle.js"], :closure-warnings {:non-standard-jsdoc :off}} {:foreign-libs [], :closure-warnings {:non-standard-jsdoc :off}, :libs ["out/src/test/cljs/Circle.js" "out/src/test/cljs/reactJS.js"], :ups-foreign-libs []}))
Comment by Corin Lawson [ 19/Nov/17 7:48 PM ]

Thanks Mike,

I will make an effort follow all those instructions, but I do not see how I should provide the repro (no links, no attachments)... should it be inline code blocks?

Also, I shall be sure to run lein test before submitting the next patch. I note that the difference is only the order of the items in :libs vector, can you advise if order is important?

Comment by Mike Fikes [ 20/Nov/17 8:11 AM ]

Hi Corin,

Yes, inline code blocks are great. Anything minimal that doesn't depend on more than the shipping cljs.jar to demonstrate the issue (either directly in its REPL or via compiling using the build API). Here is a recent example using the build API: https://dev.clojure.org/jira/browse/CLJS-2397?focusedCommentId=47278&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-47278

There are actually other tests as well beyond lein test. See https://clojurescript.org/community/running-tests

I haven't looked into the details of this issue, so can't speak to to whether order of items is important.

Comment by Corin Lawson [ 20/Nov/17 8:47 AM ]

Steps to reproduce the problem.

Consider the following three source files:

src/distinct_inputs/core.cljs
(ns distinct-inputs.core
  (:require [d3]
            [circle :refer [circle]]))

(-> d3
    (.select "body")
    (.append "svg")
    (.attr "width" 200)
    (.attr "height" 200)
    (.call circle "steelblue"))
es6/circle.js
import * as d3 from 'd3';

export function circle(sel, color) {
  return sel
    .append("circle")
    .attr("cx", "100px")
    .attr("cy", "100px")
    .attr("r",  "100px")
    .attr("fill", color);
}
build.clj
(require 'cljs.build.api)

(cljs.build.api/build
  "src"
  {:main 'distinct-inputs.core
   :output-to "out/main.js"
   :install-deps true
   :foreign-libs [{:file "es6"
                   :module-type :es6}]
   :npm-deps     {:d3 "3.5.16"}})

Execute cljs:

java -cp cljs.jar:src clojure.main build.clj

Expected outcome

cljs should produce nothing to the standard output, exit cleanly and write the following files (approximately).

out/main.js
var CLOSURE_UNCOMPILED_DEFINES = {};
var CLOSURE_NO_DEPS = true;
if(typeof goog == "undefined") document.write('<script src="out/goog/base.js"></script>');
document.write('<script src="out/goog/deps.js"></script>');
document.write('<script src="out/cljs_deps.js"></script>');
document.write('<script>if (typeof goog == "undefined") console.warn("ClojureScript could not load :main, did you forget to specify :asset-path?");</script>');
document.write('<script>goog.require("process.env");</script>');
document.write('<script>goog.require("distinct_inputs.core");</script>');
out/distinct_inputs/core.js
goog.provide('distinct_inputs.core');
goog.require('cljs.core');
goog.require('module$distinct_inputs$node_modules$d3$d3');
goog.require('module$distinct_inputs$es6$circle');
module$distinct_inputs$node_modules$d3$d3.select("body").append("svg").attr("width",(200)).attr("height",(200)).call(module$distinct_inputs$es6$circle.circle,"steelblue");

//# sourceMappingURL=core.js.map
out/es6/circle.js
goog.provide("module$distinct_inputs$es6$circle");goog.require("module$distinct_inputs$node_modules$d3$d3");function circle$$module$distinct_inputs$es6$circle(sel,color){return sel.append("circle").attr("cx","100px").attr("cy","100px").attr("r","100px").attr("fill",color)}module$distinct_inputs$es6$circle.circle=circle$$module$distinct_inputs$es6$circle

Actual outcome.

cljs exits with exit code 1 and produces the following standard out.

stdout
Exception in thread "main" java.lang.IllegalArgumentException: Duplicate module path after resolving: /distinct_inputs/node_modules/d3/d3.js, compiling:(/distinct_inputs/build.clj:3:1)
	at clojure.lang.Compiler.load(Compiler.java:7391)
	at clojure.lang.Compiler.loadFile(Compiler.java:7317)
	at clojure.main$load_script.invokeStatic(main.clj:275)
	at clojure.main$script_opt.invokeStatic(main.clj:335)
	at clojure.main$script_opt.invoke(main.clj:330)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.lang.Var.invoke(Var.java:379)
	at clojure.lang.AFn.applyToHelper(AFn.java:154)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Duplicate module path after resolving: /distinct_inputs/node_modules/d3/d3.js
	at com.google.javascript.jscomp.deps.ModuleLoader.resolvePaths(ModuleLoader.java:276)
	at com.google.javascript.jscomp.deps.ModuleLoader.<init>(ModuleLoader.java:92)
	at com.google.javascript.jscomp.Compiler.parseInputs(Compiler.java:1731)
	at com.google.javascript.jscomp.Compiler.parse(Compiler.java:995)
	at cljs.closure$convert_js_modules.invokeStatic(closure.clj:1680)
	at cljs.closure$process_js_modules.invokeStatic(closure.clj:2371)
	at cljs.closure$handle_js_modules.invokeStatic(closure.clj:2495)
	at cljs.closure$build.invokeStatic(closure.clj:2592)
	at cljs.build.api$build.invokeStatic(api.clj:204)
	at cljs.build.api$build.invoke(api.clj:189)
	at cljs.build.api$build.invokeStatic(api.clj:192)
	at cljs.build.api$build.invoke(api.clj:189)
	at user$eval24.invokeStatic(build.clj:3)
	at user$eval24.invoke(build.clj:3)
	at clojure.lang.Compiler.eval(Compiler.java:6927)
	at clojure.lang.Compiler.load(Compiler.java:7379)
	... 11 more

None of the aforementioned expected files are produced.

Cause of the exception.

The exception emitted by ModuleLoader#resolvePaths is a result of the same input file (i.e. node_modules/d3/d3.js) having been specified more than once to Compiler#initModules. There happens to be this note in Compiler#getAllInputsFromModules:

src/com/google/javascript/jscomp/Compiler.java
// NOTE(nicksantos): If an input is in more than one module,
        // it will show up twice in the inputs list, and then we
        // will get an error down the line.

cljs.closure/process-js-modules is provided a :foreign-libs vector which contains a repeated entry for node_modules/d3/d3.js (and also it's package.json). That vector is a result of multiple invocations of cljs.closure/node-inputs; once for out/cljs$node_modules.js (which is presumably a result of the dependency in distinct_inputs/core) and again for es6/circle.js.

In short, the dependency on D3 is pulled in by both ClojureScript source files and JavaScript module source files.

Proposed solution.

In this scenario the :foreign-libs vector contains repeated entries dispite the use of distinct within cljs.closure/node-inputs. A possible solution would to remove the use of distinct within cljs.closure/node-inputs and require the caller of cljs.closure/node-inputs to use distinct.

Solution A
From 063e35080c14d35189ab7827f25f071e958ab5b4 Mon Sep 17 00:00:00 2001
From: Corin Lawson <corin@responsight.com>
Date: Tue, 21 Nov 2017 01:31:53 +1100
Subject: [PATCH] CLJS-2402: Ensure :foreign-libs vector contains distinct
 entries.

---
 src/main/clojure/cljs/closure.clj | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/src/main/clojure/cljs/closure.clj b/src/main/clojure/cljs/closure.clj
index a686f878..74a0cc86 100644
--- a/src/main/clojure/cljs/closure.clj
+++ b/src/main/clojure/cljs/closure.clj
@@ -2219,7 +2219,7 @@
      (when env/*compiler*
        (:options @env/*compiler*))))
   ([entries opts]
-   (into [] (distinct (mapcat #(node-module-deps % opts) entries)))))
+   (into [] (mapcat #(node-module-deps % opts) entries))))
 
 (defn index-node-modules
   ([modules]
@@ -2480,14 +2480,15 @@
         output-dir (util/output-directory opts)
         opts (update opts :foreign-libs
                (fn [libs]
-                 (into (if (= target :nodejs)
-                         []
-                         (index-node-modules node-required))
-                   (into expanded-libs
-                     (node-inputs (filter (fn [{:keys [module-type]}]
-                                            (and (some? module-type)
-                                              (not= module-type :amd)))
-                                    expanded-libs))))))
+                 (distinct
+                   (into (if (= target :nodejs)
+                           []
+                           (index-node-modules node-required))
+                         (into expanded-libs
+                               (node-inputs (filter (fn [{:keys [module-type]}]
+                                                      (and (some? module-type)
+                                                           (not= module-type :amd)))
+                                                    expanded-libs)))))))
         opts (if (some
                    (fn [ijs]
                      (let [dest (io/file output-dir (rel-output-path (assoc ijs :foreign true) opts))]
-- 
2.13.0

A more general solution is cljs.closure/process-js-modules must ensure the set of input files (i.e. js-modules) is distinct. This patch would be simpler (i.e. doesn't mess with code I don't understand) and closer to the call to Google Closure Compiler.

Solution B
From 6bf11a24b93642e118e6d29c5af8a137fa01ea94 Mon Sep 17 00:00:00 2001
From: Corin Lawson <corin@responsight.com>
Date: Sun, 19 Nov 2017 20:25:31 +1100
Subject: [PATCH] CLJS-2402: Ensure input source files are distinct.

---
 src/main/clojure/cljs/closure.clj | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/main/clojure/cljs/closure.clj b/src/main/clojure/cljs/closure.clj
index a686f878..24421bde 100644
--- a/src/main/clojure/cljs/closure.clj
+++ b/src/main/clojure/cljs/closure.clj
@@ -2364,7 +2364,8 @@
   (let [;; Modules from both :foreign-libs (compiler options) and :ups-foreign-libs (deps.cljs)
         ;; are processed together, so that files from both sources can depend on each other.
         ;; e.g. commonjs module in :foreign-libs can depend on commonjs module from :ups-foreign-libs.
-        js-modules (filter :module-type (concat (:foreign-libs opts) (:ups-foreign-libs opts)))]
+        js-modules (filter :module-type (concat (:foreign-libs opts) (:ups-foreign-libs opts)))
+        js-modules (distinct js-modules)]
     (if (seq js-modules)
       (util/measure (:compiler-stats opts)
         "Process JS modules"
-- 
2.13.0

FWIW: I prefer Solution B.

Comment by Corin Lawson [ 20/Nov/17 8:49 AM ]

Attached proposed Solution B

Comment by Corin Lawson [ 20/Nov/17 9:02 AM ]

Hi Mike,

I hope this is to your's (and BDFL's) satisfaction now; I ran lein test for both proposed solutions and I do not receive any failures. I do receive errors, however, that do not occur in assertions. I assume that the cause is something peculiar (or lack thereof) in my setup. Let me know if you require anything else from me.

Cheers,
Corin.

Comment by Mike Fikes [ 20/Nov/17 12:08 PM ]

Thanks Corin. The entire test suite passes for me with your latest patch.





[CLJS-2401] build with :optimizations is not working on Windows Created: 18/Nov/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Dmitry Gusarov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

I got cljs.jar from:
https://github.com/clojure/clojurescript/releases/tag/r1.9.946


Attachments: Zip Archive prj.zip    

 Description   

Just went through Quick Start and got an error at 'Production Builds' section.
Looks like it currently unable to compile with :optimizations

Exception in thread "main" java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/prj/out/cljs/core.js, compiling:(C:\prj\release.clj:3:1)
        at clojure.lang.Compiler.load(Compiler.java:7391)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$script_opt.invokeStatic(main.clj:335)
        at clojure.main$script_opt.invoke(main.clj:330)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:379)
        at clojure.lang.AFn.applyToHelper(AFn.java:154)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.nio.file.InvalidPathException: Illegal char <:> at index 2: /C:/prj/out/cljs/core.js
        at sun.nio.fs.WindowsPathParser.normalize(Unknown Source)
        at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
        at sun.nio.fs.WindowsPathParser.parse(Unknown Source)
        at sun.nio.fs.WindowsPath.parse(Unknown Source)
        at sun.nio.fs.WindowsFileSystem.getPath(Unknown Source)
        at com.google.javascript.jscomp.SourceMapResolver.getRelativePath(SourceMapResolver.java:73)
        at com.google.javascript.jscomp.SourceMapResolver.extractSourceMap(SourceMapResolver.java:53)
        at com.google.javascript.jscomp.JsAst.parse(JsAst.java:168)
        at com.google.javascript.jscomp.JsAst.getAstRoot(JsAst.java:55)
        at com.google.javascript.jscomp.CompilerInput.getAstRoot(CompilerInput.java:122)
        at com.google.javascript.jscomp.Compiler.hoistNoCompileFiles(Compiler.java:1992)
        at com.google.javascript.jscomp.Compiler.orderInputs(Compiler.java:1890)
        at com.google.javascript.jscomp.Compiler.parseInputs(Compiler.java:1793)
        at com.google.javascript.jscomp.Compiler.parseForCompilationInternal(Compiler.java:982)
        at com.google.javascript.jscomp.Compiler.access$300(Compiler.java:102)
        at com.google.javascript.jscomp.Compiler$6.call(Compiler.java:964)
        at com.google.javascript.jscomp.Compiler$6.call(Compiler.java:961)
        at com.google.javascript.jscomp.CompilerExecutor$2.call(CompilerExecutor.java:101)
        at java.util.concurrent.FutureTask.run(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
        at java.lang.Thread.run(Unknown Source)





[CLJS-2400] test-module-processing and test-global-exports fail under Java 9 Created: 17/Nov/17  Updated: 17/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

If you apply the fix in CLJS-2377, and then run script/test under Java 8, things pass, but under Java 9 test-module-processing and test-global-exports fail:

ERROR in (test-module-processing) (TypeError:NaN:NaN)
expected: (= (array/nth #js [1 2 3] 1) 2)
  actual: #object[TypeError TypeError: g4.ki is not a function]

ERROR in (test-global-exports) (TypeError:NaN:NaN)
expected: (= (plus 1 2) 3)
  actual: #object[TypeError TypeError: Cannot read property 'add' of undefined]


 Comments   
Comment by Mike Fikes [ 17/Nov/17 6:13 PM ]

For the first error, under Java 8, the :advanced JavaScript (in builds/out-adv/core-advanced-test.js) has this bit of code in it

var b=f4.nth([1,2,3],1);

while under Java 9, we evidently have a rename failure:

var b=g4.ki([1,2,3],1);




[CLJS-2398] cljs.core.Var shouldn't satisfy IWithMeta Created: 12/Nov/17  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File 0001-Remove-IWithMeta-implementation-from-Var.patch    
Patch: Code

 Description   

cljs.core.Var implements IWithMeta, which differs from vars in Clojure which do not have immutable metadata. with-meta shouldn't be supported on vars, only alter-meta! should.

Unlike Clojure, ClojureScript doesn't use dynamic dispatch for alter-meta!. The implementation of cljs.core/alter-meta! will happily mutate the metadata of any object with a "meta" field, even if it's not a ref/var/whatever identity. This means that if you want to have metadata on your custom type, you have to have a "meta" field, rather than implement methods for getting/changing metadata. That's not too ideal, but doesn't present an immediate problem for me.

The reason I care about IWithMeta is that I need to distinguish metadata handling between values and reference types. Ideally, there would be an IAlterMeta protocol as well, but (and (satisfies? IMeta x) (not (satisfies? IWithMeta x))) suffices. On the JVM, the same can be done with IMeta and IObj.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 8:14 PM ]

Unit test failure:

ERROR in (test-1248) (core-advanced-test.js:64:417)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[Error Error: No protocol method IWithMeta.-with-meta defined for type object: #'cljs.core/first]




[CLJS-2395] (str (goog.date.Date. 2017 11 8)) behaves differently with no optimizations than with :simple or :advanced Created: 08/Nov/17  Updated: 19/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Eero Helenius Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Zip Archive cljs-goog-date-str-bug.zip    

 Description   

With :optimizations :none, (str (goog.date.Date. 2017 11 8)) returns 20171208, but with :simple or :advanced, it returns 1512684000000.

I attached a simple test case with a README.



 Comments   
Comment by A. R [ 08/Nov/17 6:41 AM ]

Closure compiler changed at some point:

$cljs$core$str$$.$cljs$core$IFn$_invoke$arity$1$ = function $Qd($x$jscomp$279$$) {
  return null == $x$jscomp$279$$ ? "" : "" + $x$jscomp$279$$;
};

It used to keep our [x].join("") code.

Workaround could be to emit a [x, ""].join("")

See CLJS-890 for details

Comment by A. R [ 19/Jan/18 1:05 AM ]

Reported it a bit ago: https://github.com/google/closure-compiler/issues/2782

They're now rewriting x.join("") to String:

https://github.com/google/closure-compiler/commit/b6973ec7b37f3890c8b0e11456afa95d79aaffab

which would also fix CLJS-1631 .





[CLJS-2393] process.env preload is added after all existing preloads Created: 31/Oct/17  Updated: 17/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Eugene Pakhomov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: npm-deps


 Description   

When there're other preloads and the check for process-shim returns true, `process.env` is added to the end of the vector of all preloads.
That causes problems when one of the previous preloads requires something that checks `process.env` during loading.

E.g. I have a Reagent application with `:npm-deps` set to React and a few other libraries. And I have a preload exactly like this one: https://github.com/flexsurfer/re-frisk/blob/master/src/re_frisk/preload.cljs
The issue is that `re-frisk.core` requires Reagent, which requires React, which checks `process.env` - all before `process.env` was actually created.

I think that `process.env` should go before all existing preloads.



 Comments   
Comment by Hendrik Poernama [ 02/Nov/17 8:02 AM ]

Workaround is to manually add process.env to the first of :preloads vector





[CLJS-2392] WARNING: out/inferred_externs.js: WARNING - name goog is not defined in the externs. Created: 30/Oct/17  Updated: 01/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Arne Brasseur Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript 1.9.946 (standalone cljs.jar)



 Description   

See demo and explanation here: https://github.com/lambdaisland/npmdemo/tree/goog-undefined-minimal

  • :target :nodejs
  • :optimizations :advanced
  • :infer-externs true
  • :install-deps true

The inferred_externs.js contains goog.isArrayLike and goog.global, and the compilation complains on these lines that "goog" is not defined.

Oct 30, 2017 1:46:36 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: out/inferred_externs.js:3: WARNING - name goog is not defined in the externs.
goog.isArrayLike;
^^^^

Oct 30, 2017 1:46:36 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: out/inferred_externs.js:4: WARNING - name goog is not defined in the externs.
goog.global;


 Comments   
Comment by Mike Rodriguez [ 01/Dec/17 3:09 PM ]

I don't have a full demo project for this like the description does.

However, I figured I'd add that I see similar with a build like:

:closure-defines {goog.DEBUG false}
:infer-externs true
:optimizations :advanced
:pretty-print false

Notice that there is no :target :nodejs or :intall-deps true configured here.

I have warnings for externs, such as

WARNING: /path/to/file/inferred_externs.js:33: WARNING - name goog is not defined in the externs.
goog.DEBUG;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:34: WARNING - name goog is not defined in the externs.
goog.string;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:35: WARNING - name goog is not defined in the externs.
goog.string.StringBuffer;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:36: WARNING - name goog is not defined in the externs.
goog.string.StringBuffer.prototype.append;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:37: WARNING - name goog is not defined in the externs.
goog.isArrayLike;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:38: WARNING - name goog is not defined in the externs.
goog.net;
^^^^

Dec 01, 2017 3:54:23 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /path/to/file/inferred_externs.js:39: WARNING - name goog is not defined in the externs.
goog.net.XhrIo;
^^^^

The "inferred_externs.js" file has lines

goog.DEBUG;
goog.string;
goog.string.StringBuffer;
goog.string.StringBuffer.prototype.append;
goog.isArrayLike;
goog.net;
goog.net.XhrIo;

with goog never pre-defined. Of course it doesn't need to be of course.





[CLJS-2388] Creating :doc in attr-string? in defn throws java.lang.ClassCastException Created: 25/Oct/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Vitalie Spinu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Trying to build doc on the fly:

cljs.user=> (defn tt {:doc (str "A" "B")} [])
cljs.user=> java.lang.ClassCastException: clojure.lang.PersistentList cannot be cast to java.lang.CharSequence
	at clojure.string$split.invokeStatic(string.clj:219)
	at clojure.string$split_lines.invokeStatic(string.clj:228)
	at cljs.compiler$emit_comment$print_comment_lines__3266.invoke(compiler.cljc:625)
	at cljs.compiler$emit_comment.invokeStatic(compiler.cljc:639)
	at cljs.compiler$fn__3297.invokeStatic(compiler.cljc:665)
	at cljs.compiler$fn__3297.invoke(compiler.cljc:659)
	at clojure.lang.MultiFn.invoke(MultiFn.java:229)
	at cljs.compiler$emit.invokeStatic(compiler.cljc:198)
...

Setting other attributes doesn't evaluate the code:

cljs.user=> (defn tt {:ddd (str "A" "B")} [])
cljs.user=> #'cljs.user/tt
cljs.user=> (:ddd (meta #'tt))
cljs.user=> (str "A" "B")
cljs.user=>





[CLJS-2386] random-uuid should use a cryptographically strong PRNG if available Created: 19/Oct/17  Updated: 29/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Adrian Bendel Assignee: Adrian Bendel
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-2386-no-target-js-mod.patch     Text File 0002-CLJS-2386-remove-window.patch     Text File 0003-CLJS-2386-remove-node.patch     Text File 0004-CLJS-2386-join-array.patch     Text File 0005-CLJS-2386-join-aset.patch     Text File 0006-CLJS-2386-fix-join-aset.patch     Text File CLJS-2386.patch    
Patch: Code

 Description   

random-uuid currently uses Math/random via rand-int to generate random numbers for v4 UUIDs. This patch aims to use a cryptographically strong PRNG (pseudo random number generator) instead, if available.

The functions used are:

  • window.crypto.getRandomValues in most browsers
  • window.msCrypto.getRandomValues in IE11
  • Math/random in browsers not supporting the former or if the crypto module is not made available on Node.js

Currently not used:

  • crypto.randomBytes on Node.js

Google Closure doesn't seem to provide feature detection or wrappers for the crypto-APIs, so the attached patch proposal implements a shim based on feature detection.

One open question is how the Node.js crypto module can be made available, since ClojureScripts core.cljs doesn't seem to have conditional require of Node.js-modules and maybe it should be kept this way.



 Comments   
Comment by David Nolen [ 19/Oct/17 1:33 PM ]

Some initial feedback don't switch on target, just do feature detection please. Don't use `js/window`, use `goog.global`.

Comment by A. R [ 20/Oct/17 3:43 AM ]

Should we use `js-mod` since the `UInt8` array guarantees them to be positive numbers?

Possibly outside of scope but instead of using `str` we could get some speedup by manually using `(.join ... "")` and `(.join ... "-")`

Comment by Adrian Bendel [ 20/Oct/17 4:22 AM ]

Added 0001-CLJS-2386-no-target-js-mod.patch which removes the *target* switch and replaces js/window with goog.global as per Davids comment.

This patch also uses js-mod instead of mod like A.R. suggested and adds a docstring for random-uuid explaining the behavior.
I could implement the .join suggestion, too, just not right now.

The ticket description has been updated to reflect the current state.

Comment by A. R [ 21/Oct/17 4:42 AM ]

@Adrian : Don't use goog.global.window since goog.global is already the window object when in the browser. It happens to work since the window object also keeps a refernce to itself, so you could write js/window.window.window.window.crypto. Currently your code wouldn't work on nodejs.

So just use goog.global.crypto and use js/Uint8ClampedArray as you had it previously.

@David: We should probably add a bool-expr around the js-in macro, then we could use that for feature detection code.

Comment by Adrian Bendel [ 21/Oct/17 5:16 AM ]

Oh that search/replace went wrong and wasn't even intended, testing succeeded for the reasons you explained, will fix it.

Comment by Adrian Bendel [ 21/Oct/17 5:54 AM ]

0002-CLJS-2386-remove-window.patch fixes the problems pointed out by A. R. .join refactoring is still postponed.

@A. R. for (exists? crypto) on node i can't use goog.global because it is a module, right? Thanks for the help/reviews!

Comment by A. R [ 22/Oct/17 2:47 AM ]

1. There is no reason to check for goog.global, it'll exist
2. You're checking for goog.global.msCrypto.getRandomValues but then calling goog.global.crypto.getRandomValues
3. You can't check like this (exists? crypto) ;; nodejs for node modules. crypto has to resolve to something. Just omit this nodejs case for now. Unless David can give better advice on how to conditionally require a crypto libraray in node.

Comment by Adrian Bendel [ 22/Oct/17 10:25 AM ]

Sorry for the bad patches the last two days, always have been in a hurry.
Attached 0003-CLJS-2386-remove-node.patch fixes A. Rs latest finds.

Comment by Adrian Bendel [ 29/Oct/17 5:51 AM ]

Attached 0004-CLJS-2386-join-array.patch uses .join on an array instead of str

An alternative implementation could be to convert the Uint8ClampedArray to a regular array after generating the random numbers and then just aset the string conversions in place and .join the result. But I don't know if it would be more efficient and how to convert the typed array to a regular array.

Comment by Adrian Bendel [ 29/Oct/17 8:41 AM ]

0005-CLJS-2386-join-aset.patch should be more efficient because it doesn't use a closure to access the random values from the typed array. It just generates un/typed arrays with random values and then converts and transfers those into an untyped array to .join.

Comment by Adrian Bendel [ 29/Oct/17 9:08 AM ]

0006-CLJS-2386- fix-join-aset.patch fixes a bug in 0005-CLJS-2386-join-aset.patch and supersedes it.





[CLJS-2385] cljs.analyzer/infer-type pass infers tag with incorrect priority Created: 17/Oct/17  Updated: 17/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2385.patch    
Patch: Code

 Description   

I tracked down an issue related to externs inference and found the culprit to be in the infer-type analyzer pass.

(defn thing [this]
  (.componentDidUpdate ^js/Thing this))

This will be picked up by :infer-externs correctly.

(defn thing [{:as this}]
  (.componentDidUpdate ^js/Thing this))

This won't be. The tag will be ignored.

I tracked this down to cljs.analyzer/get-tag. Since the binding is destructured it is already tagged as clj, found in :info. This causes it to never look at the form itself and discard the explicit tag.

(defn get-tag [e]
  (if-some [tag (-> e :tag)]
      tag
      (if-some [tag (-> e :info :tag)]
          tag
          (-> e :form meta :tag))))

I believe that any tag on the form itself should always win.

(defn get-tag [e]
  (if-some [tag (-> e :form meta :tag)]
    tag
    (if-some [tag  (-> e :tag)]
      tag
      (-> e :info :tag)
      )))

Changing the lookup order accordingly fixes the missing externs issue.

I'm not 100% confident my solution is correct here. :tag is handled quite differently in several places, some set it explicitly some let the infer-type pass find it.

No clue how to write a test for this.






[CLJS-2383] Improve perf of select-keys by using keyword-identical? Created: 17/Oct/17  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File 0001-CLJS-2383-Speed-up-select-keys.patch     Text File 0002-CLJS-2383-Speed-up-select-keys-no-transient.patch     Text File CLJS-2383.patch    
Patch: Code and Test

 Description   

select-keys uses not= to compare keywords. Instead, using keyword-identical? results in decent speedups (an average of 1.34 across benchmarks and engines). Note that using identical? and lookup-sentinel doesn't appear to improve perf.

Speedup Summary:

V8: 1.15, 1.08, 1.08
  SpiderMonkey: 1.71, 1.48, 1.67
JavaScriptCore: 1.33, 1.35, 1.25
       Nashorn: 1.16, 1.04, 0.97
    ChakraCore: 1.59, 1.66, 1.72

Before:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 179 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 121 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 251 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 201 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 290 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 73 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 119 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1277 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 524 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 635 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 463 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 268 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 414 msecs

After

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 155 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 112 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 169 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 146 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 135 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 173 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 84 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 54 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 95 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1099 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 502 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 648 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 292 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 151 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 240 msecs


 Comments   
Comment by Erik Assum [ 17/Oct/17 2:37 PM ]

Just want to bring your attention to CLJ-1789, where reimplementing `select-keys` in terms of reduce gave a significant speedup.
Added a patch to show that way.

Comment by Mike Fikes [ 18/Oct/17 6:46 AM ]

Here are the perf numbers for Erik's patch:

V8: 0.81, 1.14, 1.30
  SpiderMonkey: 1.49, 1.31, 1.58
JavaScriptCore: 1.02, 0.99, 0.96
       Nashorn: 1.13, 1.17, 1.21
    ChakraCore: 1.27, 1.35, 1.28

After:

Benchmarking with V8
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 220 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 106 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 141 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 169 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 153 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 183 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 110 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 74 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 124 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 1128 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 447 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 524 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs, 366 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs, 199 msecs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs, 323 msecs
Comment by Erik Assum [ 18/Oct/17 6:56 AM ]

Uploaded a patch without transients as well.

Comment by Mike Fikes [ 18/Oct/17 7:40 AM ]

Since the CLJ-1789 patch works better with larger maps, here is an additional perf test covering that case using the data from that ticket, testing both the original patch I had attached and Erik's subsequent patch. You can see the CLJ-1789 approach pays off for ClojureScript as well.

Erik, I see you attached a 3rd patch. I'd recommend adding perf numbers with each such patch, so the effect of the patch under advanced optimizations can be more readily assessed.

Engine          keyword-identical?  CLJ-1789
            V8:               1.13      1.29
  SpiderMonkey:               1.89      2.39
JavaScriptCore:               1.02      0.96
       Nashorn:               1.12      1.42
    ChakraCore:               1.68      1.82

Before:

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 373 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 668 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 200 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 2236 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1074 msecs

After (keyword-identical?)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 330 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 353 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 197 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1991 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 640 msecs

After (CLJ-1789)

Benchmarking with V8
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 290 msecs

Benchmarking with SpiderMonkey
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 279 msecs

Benchmarking with JavaScriptCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 209 msecs

Benchmarking with Nashorn
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 1578 msecs

Benchmarking with ChakraCore
;;; select-keys
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs, 591 msecs
Comment by Erik Assum [ 18/Oct/17 7:54 AM ]

Both patches should have benchmarks now

Comment by Erik Assum [ 18/Oct/17 7:56 AM ]

oh, and as a comment to the comment about larger maps, I believe the performance `transient` bit is dependent on the size of the selected keys,
eg the more selected keys found in the map, the more we gain from `conj!`

Comment by Mike Fikes [ 23/Oct/17 12:04 PM ]

Running these 4 benchmarks

[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :x]), 200000 runs
[m {:a 1, :b 2, :c 3, :d 4}], (select-keys m [:a :b :c :x :y :z]), 200000 runs
[m {:a "b", :c "d", :b "b", :d "d", :e "e", :f "f", :g "g"}], (select-keys m [:a :c :b :d :e :f :g]), 200000 runs

against all 5 engines with the 3 attached patches yields the following speedups on my machine:

CLJS-2383.patch:

            V8: 1.11, 1.10, 1.64, 1.18  Avg: 1.26
  SpiderMonkey: 2.36, 1.46, 1.79, 2.02  Avg: 1.91
JavaScriptCore: 1.28, 1.34, 1.23, 1.49  Avg: 1.34
       Nashorn: 1.19, 1.17, 1.06, 1.29  Avg: 1.18
    ChakraCore: 1.61, 1.78, 1.75, 2.11  Avg: 1.81
                                Overall avg: 1.50
         Avg excluding Nashorn & ChakraCore: 1.50

0001-CLJS-2383-Speed-up-select-keys.patch:

            V8: 0.70, 0.95, 1.05, 1.23  Avg: 0.98
  SpiderMonkey: 1.20, 1.09, 1.05, 2.03  Avg: 1.34
JavaScriptCore: 0.78, 0.84, 0.83, 1.02  Avg: 0.87
       Nashorn: 1.18, 1.08, 1.02, 1.48  Avg: 1.19
    ChakraCore: 1.00, 1.12, 1.19, 1.75  Avg: 1.27
                                Overall avg: 1.13
         Avg excluding Nashorn & ChakraCore: 1.06	

0002-CLJS-2383-Speed-up-select-keys-no-transient.patch:
	
            V8: 1.28, 1.45, 1.37, 1.33  Avg: 1.36
  SpiderMonkey: 1.54, 1.44, 1.70, 2.17  Avg: 1.71
JavaScriptCore: 1.01, 0.99, 1.03, 1.13  Avg: 1.04
       Nashorn: 1.39, 1.21, 1.14, 1.26  Avg: 1.25
    ChakraCore: 1.20, 1.23, 1.19, 1.39  Avg: 1.25
                                Overall avg: 1.32
         Avg excluding Nashorn & ChakraCore: 1.37
Comment by Mike Fikes [ 19/Nov/17 7:08 PM ]

Summary: If applied, CLJS-2383.patch would be the one to apply as it provides the greatest speedup of all the patches.





[CLJS-2382] circular dependency in node_modules prevents compilation Created: 13/Oct/17  Updated: 13/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Hendrik Poernama Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Compiler complains of missing provides when there is circular dependency in node_modules.

Cause is the order of the 'goog.addDependency' statements in out/cljs_deps.js

goog.addDependency("../node_modules/lib1.js", ['lib1'], ['lib2']); // This line will fail since 'lib2' is not yet provided
goog.addDependency("../node_modules/lib2.js", ['lib2'], ['lib1']);

Example of affected node_modules: apollo-client 1.9.2

I'm not sure if this is a closure compiler limitation or explicitly unsupported, but it does reduce the number of node packages that can be included using node_modules.

Current workaround is to rewrite the library code to not have circular deps or to switch to cljsjs.






[CLJS-2381] Race condition in cljs.analyzer/load-core Created: 10/Oct/17  Updated: 10/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There is a race condition in cljs.analyzer/load-core.

#?(:clj
   (defn load-core []
     (when (not @-cljs-macros-loaded)
       (reset! -cljs-macros-loaded true)
       (if *cljs-macros-is-classpath*
         (locking load-mutex
           (load *cljs-macros-path*))
         (locking load-mutex
           (load-file *cljs-macros-path*))))
     (intern-macros 'cljs.core)))

When 2+ threads with independent cljs.env/*compiler* bindings call load-core at the same time only one will trigger the actual load and the others will proceed directly to intern-macros and begin interning the macros into their compiler env. Since the load of the first thread may not yet be finished with loading the actual cljs.core macros the second thread will have an incomplete set of macros in its compiler env.

Related: https://dev.clojure.org/jira/browse/CLJS-1963






[CLJS-2380] deftype/defrecord with Object resolves incorrectly Created: 08/Oct/17  Updated: 08/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Whenever any deftype/defrecord defines methods on Object the CLJS compiler will emit an invalid type annotation which Closure complains about when running with type checking enabled.

(ns demo.type)

(deftype Foo [a b]
  Object
  (foo [this x]
    x))

produces this JS

/**
* @constructor
 * @implements {demo.type.Object}
*/
demo.type.Foo = (function (a,b){
this.a = a;
this.b = b;
});

produces the warning

Bad type annotation. Unknown type demo.type.Object

Looking at the analyzer data suggests that it indeed resolves incorrectly.

...
 :defs
 {Foo
  {:num-fields 2,
   :protocols #{demo.type/Object},
   :name demo.type/Foo,
   :file "demo/type.cljs",
   :end-column 13,
   :type true,
   :column 10,
   :line 3,
   :record false,
   :end-line 3,
   :skip-protocol-flag nil},

Not entirely sure how this bypasses checks but it might be a sign that there is an overly general check for Object which does not check the namespace.

AFAICT there are no bad effects on the CLJS analyzer/compiler. Closure does complain when type checking is enabled but otherwise seems unaffected.

Not yet sure where the issue starts, but looks like cljs.analyzer/resolve-var not correctly resolving Object without an ns, via [1].

[1] https://github.com/clojure/clojurescript/blob/998933f5090254611b46a2b86626fb17cabc994a/src/main/clojure/cljs/core.cljc#L1669-L1673






[CLJS-2376] Add support for ES6 default imports/exports Created: 02/Oct/17  Updated: 10/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Text File CLJS-2376-1.patch    

 Description   

ES6 has special syntax for using "default" imports and there is currently no equivalent for CLJS when using imported ES6 code.

import * as name from "module-name";
(:require ["module-name" :as name])

import { export } from "module-name";
(:require ["module-name" :refer (export)])

import { export as alias } from "module-name";
(:require ["module-name" :refer (export) :rename {export alias}])

import defaultExport from "module-name";
import defaultExport, { export [ , [...] ] } from "module-name";
import defaultExport, * as name from "module-name";
;; no easy access to defaultExport

I'm proposing to add a :default to the ns :require.

(:require ["module-name" :as mod :default foo])

This makes it much more convenient to use rewritten ES6 code from CLJS. If "module-name" has a default export you currently have to write mod/default everywhere since they is no easy way to alias the default.

(:require ["material-ui/RaisedButton :as RaisedButton])
;; :as is incorrect and the user must now use
RaisedButton/default

(:require ["material-ui/RaisedButton :default RaisedButton])
;; would allow direct use of
RaisedButton

Internally the Closure compiler (and ES6 code rewritten by babel) will rewrite default exports to a .default property, so :default really is just a convenient way to access it.

The long version that already works is

(:require ["material-ui/RaisedButton :refer (default) :rename {default RaisedButton}])

When ES6 becomes more widespread we should have convenient way to correctly refer to "default" exports.

[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/import



 Comments   
Comment by Juho Teperi [ 10/Jan/18 2:25 PM ]

First take on the implementation. This implementation rewrites :default name to {{:refer [default] :rename {default name}}} at the lowest level (parse-require-spec), so I think other functions don't need to be changed.

I didn't update the require spec validation errors yet.





[CLJS-2369] Undefined nameToPath for bootstrap` when using Twitter's Bootstrap (twbs) Created: 24/Sep/17  Updated: 08/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Corin Lawson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: npm-deps


 Description   

How to reproduce problem

  1. {{git clone https://github.com/au-phiware/cljsbuild-bootstrap4.git}}
  2. cd cljsbuild-bootstrap4
  3. make CLJS=path/to/cljs.jar
  4. open index.html
  5. Observe console messages.

Expected result

  1. Console should show the following messages:
    Error: Bootstrap dropdown require Popper.js (https://popper.js.org)
    Hello world!
  2. The Bootstrap Alert component should fade from the page resulting in a blank page.
  3. Compiler should produce:
    out/cljs_deps.js
    goog.addDependency("base.js", ['goog'], []);
    goog.addDependency("../cljs/core.js", ['cljs.core'], ['goog.string', 'goog.object', 'goog.math.Integer', 'goog.string.StringBuffer', 'goog.array', 'goog.math.Long']);
    goog.addDependency("../process/env.js", ['process.env'], ['cljs.core']);
    goog.addDependency("../node_modules/bootstrap/dist/js/bootstrap.js", ['bootstrap'], []); 
    goog.addDependency("../cljsbuild_bootstrap4/core.js", ['cljsbuild_bootstrap4.core'], ['cljs.core', 'bootstrap']);

Actual result

  1. Console shows
    Error: Undefined nameToPath for bootstrap
  2. The Bootstrap Alert component fails to fade from the page and remains on the page.
  3. Compiler produces:
    out/cljs_deps.js
    goog.addDependency("base.js", ['goog'], []);
    goog.addDependency("../cljs/core.js", ['cljs.core'], ['goog.string', 'goog.object', 'goog.math.Integer', 'goog.string.StringBuffer', 'goog.array', 'goog.math.Long']);
    goog.addDependency("../process/env.js", ['process.env'], ['cljs.core']);
    goog.addDependency("../cljsbuild_bootstrap4/core.js", ['cljsbuild_bootstrap4.core'], ['cljs.core', 'bootstrap']);


 Comments   
Comment by Hendrik Poernama [ 28/Sep/17 7:44 PM ]

I came across the same problem and did some tracing. It looks like 'load-foreign-library' correctly populated the 'provides' key, but 'library-graph-node' failed to get the 'provides'. I also observed that the output of library-graph-node is the one that gets passed to cljs_deps.js

load-foreign-library: /Users/Hendrik/Projects/bugreports/cljsbuild-bootstrap4/node_modules/bootstrap/dist/js/bootstrap.js
provides: ["bootstrap" "bootstrap/dist/js/bootstrap.js" "bootstrap/dist/js/bootstrap"]

load-foreign-library: /Users/Hendrik/Projects/bugreports/cljsbuild-bootstrap4/node_modules/bootstrap/package.json
provides: []

Copying file:/Users/Hendrik/Projects/bugreports/cljsbuild-bootstrap4/node_modules/bootstrap/dist/js/bootstrap.js to out/node_modules/bootstrap/dist/js/bootstrap.js

load-library: out/node_modules/bootstrap/dist/js/bootstrap.js

library-graph-node: /Users/Hendrik/Projects/bugreports/cljsbuild-bootstrap4/out/node_modules/bootstrap/dist/js/bootstrap.js
{:requires [], :provides [],
:url #object[java.net.URL 0x6a0659ac "file:/<snip>/cljsbuild-bootstrap4/out/node_modules/bootstrap/dist/js/bootstrap.js"],
:closure-lib true, :lib-path "out/node_modules/bootstrap/dist/js/bootstrap.js"}

Comment by Hendrik Poernama [ 29/Sep/17 3:31 AM ]

I am also getting the same error when trying to use 'apollo-client'. 'cljs_deps.js' would be missing the 'whatwg-fetch' dependency.

I suspect this is because 'whatwg-fetch' is a polyfill that does not explicitly export anything. I was able to workaround the issue by appending the following lines to the bottom of '<projectdir>/node_modules/whatwg-fetch/fetch.js'

const whatwg_fetch = 1;
export { whatwg_fetch };

I don't know enough to say that this is the same problem, other than the error message being identical. I have not tried adding the same hack to bootstrap.js

Comment by Derek Chiang [ 08/Jan/18 2:03 AM ]

Getting the same error for `ethereumjs-abi` too.

Log from console:

```
base.js:1357 Uncaught Error: Undefined nameToPath for ethereumjs_abi
at visitNode (base.js:1357)
at visitNode (base.js:1355)
at visitNode (base.js:1355)
at Object.goog.writeScripts_ (base.js:1369)
at Object.goog.require (base.js:706)
at (index):46
```





[CLJS-2366] :arglists inconsistency in cljs Created: 18/Sep/17  Updated: 24/Sep/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: David Goldfarb Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

n/a



 Description   

ClojureScript does not seem to fully support setting :arglists meta-data to a function. In particular, it seems to fail when the real parameter list contains an '&'.

In Clojure,

(:arglists (meta (defn f {:arglists '([x])} [& a] a)))

returns ([x]). But, in ClojureScript, it returns ([& a])

Note that simpler forms do work correctly:

(:arglists (meta (defn f {:arglists '([x])} [a] a)))

returns ([x]) in both environments.

(Tested in in ClojureScript 1.9.908 and Clojure 1.9.0-alpha17)



 Comments   
Comment by David Goldfarb [ 24/Sep/17 3:57 AM ]

Looks like this is a duplicate of https://dev.clojure.org/jira/browse/CLJS-2351





[CLJS-2363] Warn user when a namespace requires itself Created: 16/Sep/17  Updated: 04/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Frozenlock Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When a namespace requires itself, the compilation hangs without an error message.
It would be nice to either throw an error, or at least print a warning message to the user.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 3:28 PM ]

Cannot repro with 1.9.908 nor 1.9.946:

src/itself/core.cljs:

(ns itself.core
  (:require itself.core))
$ java -cp cljs.jar:src clojure.main -m cljs.repl.node
ClojureScript Node.js REPL server listening on 54877
To quit, type: :cljs/quit
cljs.user=> (require 'itself.core)
clojure.lang.ExceptionInfo: Assert failed: Circular dependency detected, cljs.user -> itself.core -> itself.core
Comment by Frozenlock [ 04/Dec/17 11:21 AM ]

I still get the same behavior with 1.9.946.
Tried with figwheel and uberjar.
Both hang without an error message.

Could it be a problem with the toolchain, say Leiningen?

Comment by Mike Fikes [ 04/Dec/17 11:25 AM ]

Yes. With Figwheel, I'd try disabling :autoload (see https://github.com/bhauman/lein-figwheel/wiki/Configuration-Options#client)

If that allows you to isolate it to Figwheel, perhaps this JIRA can be closed.

Comment by Frozenlock [ 03/Jan/18 4:05 PM ]

This wouldn't change much in this case : it's hanging when running Figwheel or Uberjar because it can't complete the cljs compilation.

I've tested with lein-cljsbuild and I have the same issue.
I'll check with the cljsbuild team if the problem is on their side.





[CLJS-2360] ClojureScript ignores first two arguments passed to a macro when using vargs Created: 13/Sep/17  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Ethan McCue Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug


 Description   

The following code produces different results in clojure and in clojurescript

(defmacro beep [& args]
  (cons 'list args))
(print (beep 0 1 2 3))

In clojure that code outputs (0 1 2 3)
In clojurescript that code outputs (2 3)



 Comments   
Comment by Mike Fikes [ 19/Nov/17 8:32 PM ]

Ethan, I suspect this ticket is invalid. Are you defining the macro in the REPL? If you do that, you will see the consequence that macros are really just functions called by the compiler (with two extra special arguments &env and &form, which you are passing as 0 and 1 in your example code).

Macros in ClojureScript need to be defined in a separate namespace and consumed using :require-macros. See more at https://clojurescript.org/about/differences#_macros





[CLJS-2359] Improve cljs.test runtime error reporting Created: 13/Sep/17  Updated: 13/Sep/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andrea Richiardi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It would be an awesome addition to improve the reporter so that

ERROR in (test-hydrate-saladman) (TypeError:NaN:NaN)
Uncaught exception, not in assertion.
expected: nil
  actual: #object[TypeError TypeError: Cannot read property 'call' of null]

Ran 1 tests containing 1 assertions.
0 failures, 1 errors.

would display the full stack trace of the runtime error whenever it happens.
At the moment a user is left on its own with no location, no hints whatsoever.






[CLJS-2351] Setting :arglists metadata when vararg is present Created: 08/Sep/17  Updated: 12/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Hlöðver Sigurðsson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: vars
Environment:

I'm running cljs 1.9.909 with figwheel and lumo 1.7, bug is present in both environments.


Attachments: Text File CLJS-2351-1.patch     Text File CLJS-2351.patch    
Approval: Vetted

 Description   

consider function with no parameters

(defn aarg {:arglists '([fake])} [])
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([fake]),
 :doc nil,
 :test nil}

All works as expected, but with the introduction of a vararg

(defn aarg {:arglists '([fake])} [& env])
(meta #'aarg)
=> {:ns cljs.user,
 :name aarg,
 :file nil,
 :end-column 11,
 :top-fn {:variadic true,
          :max-fixed-arity 0,
          :method-params [(env)],
          :arglists ([& env]),
          :arglists-meta (nil)},
 :column 1,
 :line 1,
 :end-line 1,
 :arglists ([& env]),
 :doc nil,
 :test nil}

:arglists does not get affected.



 Comments   
Comment by Hlöðver Sigurðsson [ 09/Dec/17 8:54 PM ]

I just submitted a patch, never used jira patch system before, that aside, the patch will make the compiler respect :arglists metadata when the user provdes one. Hope to get feedback and merge on this

Comment by David Nolen [ 11/Dec/17 8:53 AM ]

Thanks have you submitted your Clojure CA?

Comment by David Nolen [ 11/Dec/17 8:56 AM ]

Patch review, I don't really understand the purpose the changing arity of variadic-fn and adding this new flag. Just put the logic directly into variadic-fn no?

Comment by Hlöðver Sigurðsson [ 11/Dec/17 9:35 AM ]

Yes, I'll find a better solution to my patch, just that the `m` symbol in the let binding on line 3176

m (conj {:arglists (core/list 'quote (sigs fdecl))} m)

Merges the arglists and there's after this point no way to know if the :arglists came from a provided metadata or not. But I'll rename the let symbols and it's easy to avoid adding arity, I'll do another patch later today.

No haven't submitted my Clojure Contibutor Agreement, I'm filling it out now...

Comment by Hlöðver Sigurðsson [ 12/Dec/17 1:48 PM ]

Yesterday I signed the Clojure CA
"Clojure CA between Rich Hickey and Hlöðver Sigurðsson is Signed and Filed!"

The code:
The symbol name m (for metadata) is already not very good, but I just added a comma to differentiate between vararg-fn case and the other cases.

I found as I was testing this that multi-arity-fn arglist metadata does not work with or without specifically adding arglists. If so, I can open another jira ticket for that.





[CLJS-2347] NPE on :infer-externs true Created: 03/Sep/17  Updated: 22/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Kanwei Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

When :infer-externs is true in 1.9.908, an NPE will result from the Closure compiler. Also reported here with a stacktrace:

https://github.com/google/closure-compiler/issues/2629



 Comments   
Comment by Juho Teperi [ 08/Sep/17 9:00 AM ]

Well, this is interesting. I haven't been able to reproduce this without Boot-cljs completely, but I can reproduce this with Cljs by manually providing extern with single line: `var COMPILED;`. For some reason, inferred extern with Boot-cljs includes this line, but not when calling compiler directly.
`





[CLJS-2346] Make (:require foo/bar) work for JS modules Created: 01/Sep/17  Updated: 05/Sep/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: js-modules

Attachments: Text File CLJS-2346.patch    
Patch: Code

 Description   

the string require only really becomes necessary when there are multiple slashes. We can support this style as a symbol for dependencies that only have 1 slash because it's a valid symbol.



 Comments   
Comment by Thomas Heller [ 02/Sep/17 2:43 AM ]

Me again ... what is so bad about using string requires? Qualified symbols are illegal for normal CLJS code and using a string already solves all potential issues. Adding a case where you can use a symbol if there is one slash but not if there are two is just going to confuse new users with no additional benefit.

Comment by Juho Teperi [ 05/Sep/17 3:40 AM ]

I can see the point of only allowing slashes with strings.

However, if we do this, it might be good to check the warnings we give when trying to do this:

(ns example.core
  (:require [react-dom/server :as sdf]))

  No such namespace: react-dom/server, could not locate react_dom_SLASH_server.cljs, react_dom_SLASH_server.cljc, or JavaScript source providing "server"

Notice how it says "server" instead of react-dom/server. This is because everything allows slashes, but foreign lib code only uses name part instead of namespace. (This patch doesn't change the warning.)

Comment by Thomas Heller [ 05/Sep/17 4:50 AM ]

There are other ambiguities when it comes to JS requires which is why I'm still advocating for just using strings in all circumstances.

Technically we could make {{(:require [@scoped/thing :as thing])}} work as well but shouldn't.

Also I'm pretty sure I saw several JS packages that either used "-" or "_" in their names. For CLJS we always convert to an underscore which would add more confusion if we don't do this for JS.

Yes, the warning should be fixed but wouldn't even be an issue if a string was used.





[CLJS-2344] de-dupe externs Created: 31/Aug/17  Updated: 16/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: Next

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Sameer Rahmani
Resolution: Unresolved Votes: 1
Labels: closure

Attachments: Text File CLJS-2344.patch    
Patch: Code
Approval: Vetted

 Description   

We could possibly call distinct on externs to remove dupes in the common case where externs may appear multiple times accidentally on the classpath.



 Comments   
Comment by David Nolen [ 16/Jan/18 8:09 AM ]

I don't think we want to silently dedupe. We probably want to also warn so users can fix the issue. The only reason to even do this ticket is because the Closure warning is so unfriendly and fails the build.

Comment by Sameer Rahmani [ 16/Jan/18 8:33 AM ]

got it, I'll improve the patch





[CLJS-2342] Speed up printing of js object by using forEach and regex optimizations Created: 30/Aug/17  Updated: 29/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-2342-2.patch     Text File CLJS-2342-3.patch     Text File CLJS-2342.patch    
Patch: Code

 Description   

By using goog.object/forEach, along with direct interop for regex checking it is possible to speed up the printing of JavaScript objects anywhere between 1.14 and 1.77



 Comments   
Comment by Mike Fikes [ 30/Aug/17 10:50 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.42, 1.31, 1.19
  SpiderMonkey: 1.14, 1.48, 1.41
JavaScriptCore: 1.48, 1.58, 1.62
       Nashorn: 1.27, 1.36, 1.20
    ChakraCore: 1.49, 1.77, 1.77


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 74 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 84 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 75 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 95 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 92 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 134 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 46 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 41 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 55 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 1228 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 1048 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 620 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 76 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 129 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 52 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 64 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 63 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 83 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 62 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 95 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 31 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 26 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 34 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 970 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 769 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 518 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (pr-str obj), 1000 runs, 37 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (pr-str obj), 1000 runs, 43 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (pr-str obj), 1000 runs, 73 msecs
Comment by Mike Fikes [ 01/Oct/17 9:27 AM ]

Added CLJS-2342-2.patch which rebases against current master.

Comment by Mike Fikes [ 29/Oct/17 9:03 AM ]

CLJS-2342-3.patch rebaselines against current master.





[CLJS-2341] Speed up js->clj on objs using forEach and transient volatile Created: 30/Aug/17  Updated: 31/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.908
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 3
Labels: performance

Attachments: Text File CLJS-2341.patch    
Patch: Code

 Description   

It is possible to speed up js->clj on JavaScript objects by revising the implementation to employ goog.object/forEach, accumulating by bashing on a transient volatile map.

The speedups range from 1.5 to 2.1 over the current implementation.

Note: The current implementation uses js-keys. The optimization in CLJS-2340 could help js->clj, but it doesn't appear to provide much speedup in itself (perhaps 1.1?) compared to the change in implementation described above.



 Comments   
Comment by Mike Fikes [ 30/Aug/17 9:19 AM ]
Speedup Summary:

        Engine  small, medium, with sub-object
            V8: 1.62, 1.50, 1.13
  SpiderMonkey: 1.91, 1.74, 1.59
JavaScriptCore: 1.67, 1.74, 2.10
       Nashorn: 1.54, 2.13, 1.51
    ChakraCore: 1.71, 2.10, 1.95


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 55 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 63 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 93 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 155 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 87 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 94 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 45 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 33 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 42 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 1123 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 1195 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 773 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 65 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 44 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 78 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 34 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 42 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 82 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 81 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 50 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 59 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 27 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 19 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 20 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 728 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 561 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 513 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2)], (js->clj obj), 1000 runs, 38 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6)], (js->clj obj), 1000 runs, 21 msecs
[sub-obj (js-obj "g" 7 "h" 8 "i" 9 "j" 10) obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6 "s" sub-obj)], (js->clj obj), 1000 runs, 40 msecs




[CLJS-2329] REPL should not error out if quote is missing on string require Created: 18/Aug/17  Updated: 18/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Andrea Richiardi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

At the REPL, the following should not happen I think, cause it feels unidiomatic:

(require "something") ;; there is no quote before the apices

----  Could not Analyze  <cljs form>   line:1  column:1  ----

  Library name must be specified as a symbol in :require / :require-macros; offending spec: ["something"] at line 1 <cljs repl>

  1  (require '"something")
     ^--- 

----  Analysis Error  ----

Minor, solvable with:

(require '"something") ;; quote before the apices





[CLJS-2319] cljs.core/mod handling of floats inconsistent with Clojure & JavaScript Created: 13/Aug/17  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: André Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2319-Fix-cljs.core-mod-handling-of-floats_INLINED.patch     Text File CLJS-2319-Fix-cljs.core-mod-handling-of-floats.patch    
Patch: Code and Test

 Description   

The workaround for negative numbers (https://dev.clojure.org/jira/browse/CLJS-417) results in annoying behavior for floats:

(mod 2.1 3) ; => 2.0999999999999996

Both Clojure and the standard JavaScript modulo return the expected 2.1 here.

Two possible solutions:

  • only do the double-mod workaround when the dividend is actually negative
  • check if the dividend is smaller than the divisor and just return it in that case


 Comments   
Comment by David Nolen [ 13/Aug/17 5:00 PM ]

Patch welcome.

Comment by André Wagner [ 14/Aug/17 8:00 AM ]

The patch renames cljs.core/mod to double-mod and redefines mod to invoke js-mod directly when both args have the same sign.
It includes test cases for the previously failing cases, but I've also tested more exhaustively against the Clojure impl: https://gist.github.com/aw7/a32bd69923c65bddc23fd63ee062833c

Comment by David Nolen [ 14/Aug/17 8:46 AM ]

Great thanks, have you submitted your Clojure CA?

Comment by André Wagner [ 14/Aug/17 9:06 AM ]

Yeah, 2h ago.

Comment by António Nuno Monteiro [ 14/Aug/17 5:42 PM ]

Nit: shouldn't the `double-mod` function be marked as private then?

Comment by André Wagner [ 15/Aug/17 4:06 AM ]

I guess there's no need for `double-mod` to exist as a separate function at all, I've added a patch where it's inlined into `mod`.

Comment by Mike Fikes [ 19/Nov/17 8:37 PM ]

André, ClojureScript requires squashed patches (CLJS-2319-Fix-cljs.core-mod-handling-of-floats_INLINED.patch has two patches in it). See https://clojurescript.org/community/patches

Comment by Mike Fikes [ 19/Nov/17 9:13 PM ]

Arg... something in the patch is killing the older JavaScriptCore we run tests with in Travis. My only guess is the additional mod tests may be triggering CLJS-910, but that seems unlikely.





[CLJS-2301] Avoid use of deprecated goog.string/isEmptySafe in clojure.string/blank? Created: 05/Aug/17  Updated: 05/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.854
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2301.patch    
Patch: Code

 Description   

clojure.string/blank? calls goog.string/isEmptySafe, which is marked as deprecated. Instead this can be inlined with the code that this internally calls, which is non-deprecated code. Also, it can be seen that such a change has no effect on perf, with these benchmarking tests tried out:

Before:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 265 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 268 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 331 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 336 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 59 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 56 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 57 msecs


After:

Benchmarking with V8
[s nil f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 26 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 27 msecs
Benchmarking with SpiderMonkey
[s nil f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 262 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 261 msecs
Benchmarking with JavaScriptCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 4 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s nil f clojure.string/blank?], (f s), 1000000 runs, 328 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 324 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 60 msecs
Benchmarking with ChakraCore
[s nil f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
[s " \n " f clojure.string/blank?], (f s), 1000000 runs, 63 msecs
[s "aBcDeF" f clojure.string/blank?], (f s), 1000000 runs, 62 msecs
Lindas-iMac-2:clojurescript mfikes$ # Using or nil?


 Comments   
Comment by Mike Fikes [ 05/Aug/17 11:37 AM ]

The change in the attached patch was used to produce the benchmarks in the ticket description.

I also tried an alternative which uses a Clojure or and a nil? check in lieu of the goog.string/makeSafe call, and this resulted in the same benchmark numbers.

So, the patch goes with the recommended deprecation change in the documentation for goog.string/isEmptyOrWhitespaceSafe, which indicates "Use goog.string.isEmptyOrWhitespace(goog.string.makeSafe(str)) instead.", which exactly matches the code we are indirectly calling today.





[CLJS-2268] clojure.string/escape in ClojureScript (unlike in Clojure) assumes cmap is a map Created: 23/Jul/17  Updated: 29/Oct/17

Status: In Progress
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Max Kreminski Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs

Attachments: Text File 0001-CLJS-2268-Make-clojure.string-escape-constent-with-C.patch    
Patch: Code and Test

 Description   

The ClojureScript implementation of the clojure.string/escape function assumes that the cmap parameter will always be a map. This makes it different from (and specifically less general than) the Clojure implementation of the same function, which permits cmap to be anything callable.

Here's the relevant lines of the clojure.string/escape implementations in Clojure and ClojureScript. The ClojureScript implementation calls get on cmap, while the Clojure implementation invokes cmap directly.

Here's an example that works on Clojure, but doesn't work on ClojureScript, because it passes a function to clojure.string/escape instead of a map:

(defn regex-escape
  "Escapes regex special chars in the string `s`."
  [s]
  (let [special? #{\- \[ \] \{ \} \( \) \* \+ \? \. \\ \^ \$ \|}]
    (clojure.string/escape s #(when (special? %) (str \\ %)))))

Ideally, this discrepancy would be fixed by changing the ClojureScript implementation of clojure.string/escape to follow the Clojure one. This would also match the behavior described in the function's docstring, which is the same on both platforms.






[CLJS-2257] Expand dotted symbols into field accesses in the analyzer Created: 17/Jul/17  Updated: 17/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently, there is a lot of implicit information in a :var node in the analyzer around dotted symbols.

The :name of a :var node can contain implicit field accesses, and this information must be manually disambiguated with every new tool that consumes the CLJS AST (like core.typed).

A solution might be to expand specific dotted :var nodes into :dot nodes containing a :var node. Locals in particular might benefit from this transformation, would others? (Global variables, js/* variables)






[CLJS-2252] Self-host: :redef-in-file doesn't trigger for bootstrapped Created: 16/Jul/17  Updated: 16/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bootstrap


 Description   

If you redefine a symbol in a file and require that file using self-hosted ClojureScript, the :redef-in-file diagnostic doesn't trigger.

It is difficult to create a minimal repro for this, as it requires a setup that loads files. (Perhaps one can be made with the script/test-self-parity infrastructure).

It appears that this can be resolved by setting ana/file-defs at the right places and unsetting it at the right async completion points.






[CLJS-2237] Defrecord does not escape field names matching JS keywords (with/in/...) Created: 14/Jul/17  Updated: 14/Jul/17

Status: Reopened
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.660, 1.9.671
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Defrecord produces code that's incompatible with ECMASCRIPT3 language:

(defrecord Query [with in])
Compiling "target/main.js" from ["src"]...
Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                            ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                            ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                                                                                                  ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                                                                                                               ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 0 error(s), 4 warning(s)
Successfully compiled "target/main.js" in 6.224 seconds.

Probably there should be some name escaping happening? I see that (defn with []) is compiled as cljs-test.core.with$. Should we do the same for records?



 Comments   
Comment by David Nolen [ 14/Jul/17 12:19 PM ]

The problem has less to do with records than how to handle reserved names. As far as I'm concerned the Closure warnings are sufficient, but if somebody wants to devise a warning patch that warns on using reserved fields names on deftpye/record when the output language is ES3, then be my guest.

Comment by David Nolen [ 14/Jul/17 12:20 PM ]

Related CLJS-871

Comment by Nikita Prokopov [ 14/Jul/17 12:37 PM ]

Is there any reason why CLJS defaults to language_in=ES3? Shouldn’t CLJS lock in the version of JS it outputs? As I understand, programmers have no control over how JS looks, but CLJS compiler has knowledge and control over what version of JS it outputs (and feeds into Closure)? In other words, why not set language_in to ES5 by default?





[CLJS-2236] Defrecord does not escape field names matching JS keywords (with/in/...) Created: 14/Jul/17  Updated: 02/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.660, 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Defrecord produces code that's incompatible with ECMASCRIPT3 language:

(defrecord Query [with in])
Compiling "target/main.js" from ["src"]...
Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                            ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                            ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                                                                                                  ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager println
WARNING: /Users/prokopov/work/cljs-test/target/cljsbuild-compiler-0/cljs_test/core.js:117: WARNING - Keywords and reserved words are not allowed as unquoted property names in older versions of JavaScript. If you are targeting newer versions of JavaScript, set the appropriate language_in option.
return (!((other13203 == null))) && ((this13202__$1.constructor === other13203.constructor)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.with,other13203.with)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.in,other13203.in)) && (cljs.core._EQ_.cljs$core$IFn$_invoke$arity$2(this13202__$1.__extmap,other13203.__extmap));
                                                                                                                                                                                                                                                               ^

Jul 14, 2017 6:58:14 PM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 0 error(s), 4 warning(s)
Successfully compiled "target/main.js" in 6.224 seconds.

Probably there should be some name escaping happening? I see that (defn with []) is compiled as cljs-test.core.with$. Should we do the same for records?



 Comments   
Comment by David Nolen [ 01/Aug/17 5:43 PM ]

Now that CLJS-1620 is done, we should think about this more deeply.

Comment by Stephen Sugden [ 01/Oct/17 11:19 AM ]

Ran into an interesting bug that is likely a consequence of this:

cljs.user=> (defrecord T [arguments])
cljs.user/T
cljs.user=> (= (T. [1]) (T. [0]))
true




[CLJS-2209] case docstring should explain constants may be evaluated (cljs only) Created: 10/Jul/17  Updated: 10/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.660
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Vetted

 Description   

The ClojureScript docstring for case says, "The test-constants are not evaluated." But that statement is not complete. The ClojureScript "Differences from Clojure" page <https://www.clojurescript.org/about/differences> says ":const metadata: ... causes case test constants which are symbols resolving to ^:const Vars to be inlined with their values". The case docstring should reflect that. Related discussion at <https://groups.google.com/d/msg/clojure/u1RZsmjbQ64/p7B9eRwuAQAJ>.






[CLJS-2196] SpiderMonkey path needs quoting in test scripts Created: 08/Jul/17  Updated: 08/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-2196.patch    
Patch: Code

 Description   

The paths to the other engines (V8, Nashorn, etc.), are quoted, allowing paths with spaces. This needs to also be done for SpiderMonkey.






[CLJS-2168] Properly document browser-env options Created: 04/Jul/17  Updated: 08/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Timothy Pote Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: browser-repl

Attachments: Text File 001-CLJS-2168.patch    
Patch: Code
Approval: Screened

 Description   

There are a number of browser-env options that work only partially or not at all:

  • :optimizations - Only :whitespace and :simple appear to work for me
  • :host - Is never read. Instead, we always bind to 0.0.0.0.
  • :serve-static - Is never read.
  • :preloaded-libs - Is never read.

These should either be properly documented, removed, or made to work.



 Comments   
Comment by Timothy Pote [ 04/Jul/17 4:55 PM ]

I think we should:

  • Properly document :optimizations and throw exception on an unsupported option
  • Make :host function properly
  • Remove :serve-static as an option
  • Remove :preloaded-libs as an option
Comment by Timothy Pote [ 04/Jul/17 4:57 PM ]

Note that, as it stands, this also addresses CLJS-1502.

Comment by Timothy Pote [ 04/Jul/17 5:51 PM ]

This does what I said in my initial comment.

Note that this commit is rather small if you exclude whitespace.

Comment by Timothy Pote [ 05/Jul/17 8:43 AM ]

After thinking about it some more, I'm not sure what the gain is for being able to specify :optimizations in the browser-env, and the cost is confusion on the part of the users. I do not think it's apparent that this is a compiler option for the child iframe JS and evaluated repl forms. This may be a case where we can just "do the right thing" and remove some burden from the user.

I'm thinking we either remove :optimizations entirely or we only use it for evaluated repl forms and use :simple for the initial payload. Considering the user can already override this in the arguments to cljs.repl/repl, I lean toward removing it altogether.

Comment by David Nolen [ 08/Jul/17 10:48 AM ]

Related CLJS-1502





[CLJS-2167] Browser REPL leaves a socket open when it fails to connect to the browser Created: 04/Jul/17  Updated: 05/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Timothy Pote Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: browser-repl, repl

Attachments: Text File 001-CLJS-2167.patch    
Patch: Code
Approval: Screened

 Description   

Repro steps:
0. Via an nrepl session
1. Start a browser REPL but do not connect the browser
2. Interrupt the evaluation
3. Start another browser REPL

This will result in the following exception:

java.net.BindException: Address already in use (Bind failed)

Note that, though this is easiest to trigger via an nrepl session, the underlying problem is that the socket server is not being closed in the event of an exception during initialization. You can re-create this in a plain old clojure repl by setting up monospaced}}repl/set-break-handler!{{monospaced prior to starting the browser REPL.



 Comments   
Comment by Timothy Pote [ 04/Jul/17 3:57 PM ]

There are two parts to this patch:
1. Try/Catch the repl to make sure repl-env always gets a chance to clean-up
2. Make BrowserEnv interrupt its threads

Note that this patch is mostly whitespace from re-indenting after wrapping a from in try/catch.





[CLJS-2156] Add postamble, or some other generic way to append code to a file Created: 03/Jul/17  Updated: 18/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: Next

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: closure

Approval: Vetted

 Description   

Generally useful, but specifically for appending clj.loader/set-loaded! calls to user files.



 Comments   
Comment by Thomas Heller [ 03/Jul/17 7:08 AM ]

In shadow-cljs I have 4 attributes per :module for this.

  • :prepend treated as text, eg. licence headers. Can contain JS but won't go through optimizations.
  • :prepend-js treated as JS code that will also go through closure optimizations
  • :append-js
  • :append

I think it is valuable to have all 4 and making the distinction between "text" and "JS". shadow.loader is implemented entirely through these attributes.

Comment by David Nolen [ 03/Jul/17 7:21 AM ]

I'm having second thoughts about this one, since I don't think we need this for CLJS-2157, which is why I opened it originally.

Comment by Thomas Heller [ 03/Jul/17 10:09 AM ]

set-loaded! should only be called once per module, so it must be appended to the module itself not to individual files in the module.

By appending to individual files you will eventually call it more than once for modules that have multiple entries. AFAICT the call is not idempotent and may cause events to be emitted multiple times. At the very least it may call set-loaded! before the full module has actually been loaded. Since it immediately triggers the callbacks that may lead to bad results.

The config options are generally useful not just for the loader.

Comment by David Nolen [ 03/Jul/17 10:44 AM ]

Thomas as far as I can tell ModuleManager.setLoaded is idempotent. I haven't run into any issues locally with testing. Feel free provide a failing case if you can but I couldn't find one myself.





[CLJS-2147] apply test suit Created: 01/Jul/17  Updated: 01/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.671
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: test

Attachments: Text File CLJS-2147.patch    
Patch: Code and Test

 Description   

Created an apply test suit.

Few tests are commented out which currently fail (nothing new and there are tickets for them).



 Comments   
Comment by David Nolen [ 01/Jul/17 1:56 PM ]

Please remove #_ following each test.

Comment by A. R [ 01/Jul/17 2:14 PM ]

Forgot about those. Done.





[CLJS-2138] Remove redundant checks in ChunkedSeq.-rest and ChunkedSeq.-next Created: 29/Jun/17  Updated: 29/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.660
Fix Version/s: 1.9.671

Type: Defect Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: refactoring

Attachments: Text File CLJS-2138.patch    
Patch: Code
Approval: Screened

 Description   

chunked-seq always returns a ChunkedSeq object and hence the nil? checks do nothing.






[CLJS-2132] Optimize transient vector creation Created: 27/Jun/17  Updated: 27/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.655
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File CLJS-2132.patch    
Patch: Code

 Description   

This is a very simple optimization around transient []. It avoids copying the empty array.

Performance improvements, for mapv on smallish vectors (5-32) elements anywhere from 20% up to 100% across FF & Chrome.

(defn faster-editable-root
  [node]
  (if (identical? (.-EMPTY_NODE PersistentVector) node)
    (VectorNode. (js-obj) (make-array 32))
    (VectorNode. (js-obj) (aclone (.-arr node)))))
(def orig-editabe-root tv-editable-root)
(enable-console-print!)
(dotimes [_ 2]
  (doseq [size [5 10 40]]
    (let [xs (range size)
          sims 500000]
      (set! tv-editable-root orig-editabe-root)
      (prn "Size: " size)
      (simple-benchmark [] (mapv inc xs) sims)
      (set! tv-editable-root faster-editable-root)
      (prn "NEW:")
      (simple-benchmark [] (mapv inc xs) sims))))





[CLJS-2127] Add invoke* helper macro Created: 26/Jun/17  Updated: 03/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.562
Fix Version/s: 1.9.671

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: refactoring

Attachments: Text File CLJS-2127.patch    
Patch: Code
Approval: Screened

 Description   

This is a simple refactor around {IFn} protocol around core.cljc and core.cljs. We would like to hide the details of the invocation naming convention to avoid simple errors as well as to support changes more simply.



 Comments   
Comment by David Nolen [ 29/Jun/17 7:05 AM ]

The scope of this ticket needs to be narrowed to make it simpler for me to review. For the time being the only thing I would like to see is `invoke*` which hides the naming convention for direct invokes. No other higher level macro helpers should be provided in the resolution of this sissue.

Comment by A. R [ 29/Jun/17 12:29 PM ]

Patch updated. Much fewer changes to keep it simple for now.

Comment by David Nolen [ 29/Jun/17 2:08 PM ]

Looking better but lets have one helper for constructing the name, should just take number or :variadic.





[CLJS-2120] Optimize keyword function Created: 24/Jun/17  Updated: 25/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.562
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJS-2120.patch    
Patch: Code
Approval: Accepted

 Description   

keyword function can be sped up. This matters when keywords are created dynamically when doing XHR.

The patch now matches Clojure more closely (using substring). It's also optimized for passing strings.

Results:

(enable-console-print!)
(let [sims 1000000]
  (dotimes [_ 2]
    (doseq [x ["foo" "foo/bar" [nil "foo"] ["foo" "bar"] [:foo :bar] [nil :foo]]]
      (prn "Testing keyword with: " x)
      (if (vector? x)
        (do (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword a0 a1)) sims)
            (simple-benchmark [[a0 a1] x] (set! js/FOOO (keyword2 a0 a1)) sims))
        (do (simple-benchmark [] (set! js/FOOO (keyword x)) sims)
            (simple-benchmark [] (set! js/FOOO (keyword2 x)) sims))))))


"Testing keyword with: " "foo"
[], (set! js/FOOO (keyword x)), 1000000 runs, 194 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 71 msecs
"Testing keyword with: " "foo/bar"
[], (set! js/FOOO (keyword x)), 1000000 runs, 260 msecs
[], (set! js/FOOO (keyword2 x)), 1000000 runs, 104 msecs
"Testing keyword with: " [nil "foo"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 278 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 188 msecs
"Testing keyword with: " ["foo" "bar"]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 379 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 215 msecs
"Testing keyword with: " [:foo :bar]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 351 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 207 msecs
"Testing keyword with: " [nil :foo]
[[a0 a1] x], (set! js/FOOO (keyword a0 a1)), 1000000 runs, 376 msecs
[[a0 a1] x], (set! js/FOOO (keyword2 a0 a1)), 1000000 runs, 37 msecs


 Comments   
Comment by A. R [ 24/Jun/17 10:56 AM ]

Changes the behavior of:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> [nil "foo"]
(.-fqn (keyword "foo/bar/hmm"))
=> "foo/bar/hmm"
((juxt namespace name) (keyword2 "foo/bar/hmm"))
=> ["foo" "bar/hmm"]
(.-fqn (keyword2 "foo/bar/hmm"))
=> "foo/bar/hmm"

Clojure 1.9:

((juxt namespace name) (keyword "foo/bar/hmm"))
=> ["foo" "bar/hmm"]

So: yay





[CLJS-2103]  clarify arg type and order constraints of keys and vals Created: 19/Jun/17  Updated: 19/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.562
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File CLJS-2103.patch    
Patch: Code

 Description   

Backport CLJ-1302 to ClojureScript, while also making the argument name be map instead of hash-map.






[CLJS-2102] Case fallback (cond) doesn't match consts Created: 19/Jun/17  Updated: 20/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.562
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Reproduce:

(def ^:const ccc 1)
(case 1
  ccc :yes
  :no)
(case 1
  ccc :yes
  :hmm :hmm
  :no)

Second example yields :no because it falls back to cond which doesn't handle the consts properly.



 Comments   
Comment by Kevin Downey [ 20/Jun/17 12:54 PM ]

a related thread https://groups.google.com/d/msg/clojure/u1RZsmjbQ64/p7B9eRwuAQAJ

clojurescript's handling of symbols in case is broken(it diverges from clojure's), but the cond fallback is correct(it matches clojure)

Comment by David Nolen [ 20/Jun/17 2:10 PM ]

As discussed in that thread we're not re-breaking a thing we broke 2 years ago. It's simply not that important and far too late.





[CLJS-2087] Duplicate set/map keys when using characters or quoting Created: 15/Jun/17  Updated: 25/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Related: CLJS-1587

This ticket deals with the following cases:

{'0 "a", 0 "b", \a "a", "a" "b"}
#{\a "a"}
(hash-set \a "a")
(array-map '0 "a", 0 "b", \a "a", "a" "b")

Potential idea: Use emit-str in cljs.compiler on the keys/set-members and only then check for uniqueness. I'm not sure if this is a good idea though. Anybody have thoughts on this?



 Comments   
Comment by Mike Fikes [ 25/Jun/17 4:01 PM ]

FWIW, tools.reader, used in self-hosted ClojureScript, rejects the first two examples with a diagnostic:

Set literal contains duplicate key: a




[CLJS-2074] Compiler option to ignore JS files provided by :foreign-libs Created: 06/Jun/17  Updated: 07/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It is increasingly common that people want to use JS packages not provided by :foreign-libs. Many CLJS libs however are built on the assumption that you are using :foreign-libs.

Om, Reagent and others expect cljsjs.react to provide a global js/React. Currently users need to work around [1] those :foreign-libs dependencies by using :exclusions in lein or boot and by creating empty stub files to ensure that the cljsjs.react namespace still exist so the libraries can be compiled

This CLJSJS pull request [2] wants to provide those empty stub files in a surrogate package since work around this has become to common. As detailed in the PR comments I think this is a dangerous precedent and that we should come up with a better solution.

The simplest way forward would be to add a compiler option to just keep all (or some) :foreign-libs from being included in the build. The externs are still useful but the JS could just be ignored.

The better solution probably involves using the JS packages by their actual name (via https://dev.clojure.org/jira/browse/CLJS-2061) so the need for cljsjs alias packages goes away.

[1] http://blob.tomerweller.com/reagent-import-react-components-from-npm
[2] https://github.com/cljsjs/packages/pull/1192






[CLJS-2070] Dotted interop forms can leak invalid JS into output Created: 04/Jun/17  Updated: 20/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.8.51, 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Peter Jaros Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

(println .aJSMethod) produces cljs.core.println.call(null,.aJSMethod);, which is not valid JavaScript.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 2:49 PM ]

For clarity, perhaps this should not be considered dotted interop. Dotted interop looks like (. "a" toUpperCase), for which we can alternatively rely on macroexpansion to write idiomatically as (.toUpperCase "a")

My preference would be to view (println .aJSMethod) as a call where .aJSMethod is a symbol. If ClojureScript follows the definition of symbols at https://clojure.org/reference/reader#_symbols, then an argument can be made that .aJSMethod is a reserved symbol.

Clojure doesn't appear to have a mechanism that prohibits the use of reserved symbols. For example, you can do the following in Clojure.

(def .aJSMethod "hello")
(println .aJSMethod)

Perhaps ClojureScript could be revised to also work like Clojure for such cases. Or, alternatively, perhaps an analysis error could be triggered for reserved symbols not in operator position.

Comment by Mike Fikes [ 20/Nov/17 2:58 PM ]

Ahh, even Clojure won't let you go down the path of "allow symbols starting with dot" if you do:

(def .quux inc)
(.quux 1)

So, I'd argue for treating things like (println .aJSMethod) and (def .aJSMethod "hello") as invalid code, with the underlying reason being the attempted use of a reserved symbol.





[CLJS-2063] Auto-optimize equality check of goog-defines in advanced mode Created: 31/May/17  Updated: 16/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Shaun LeBron Assignee: Shaun LeBron
Resolution: Unresolved Votes: 0
Labels: None


 Description   

per discussion: https://clojurians-log.clojureverse.org/cljs-dev/2017-05-30.html






[CLJS-2062] js->clj throws "No protocol method IEmptyableCollection.-empty..." Created: 29/May/17  Updated: 31/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

js->clj says

(cond

    [...]
                
    (coll? x)
    (into (empty x) (map thisfn x))

The gap is that (coll? x) checks for ICollection, while (empty x) requires IEmptyableCollection. When x passes the first test but fails the second, js->clj throws "Error: No protocol method IEmptyableCollection.-empty defined for type : [object Object]."

Inasmuch as js->clj is recursive through Javascript objects that tend to contain everything but the kitchen sink, it is inevitably a matter of best effort rather than high principle. In a word, this is no place to split hairs. If the collection is not emptyable, I expect js->clj to make some effort to preserve order but not type: use a vector!



 Comments   
Comment by Rohit Aggarwal [ 30/May/17 12:02 PM ]

Phill Wolf can you give a concrete failing case?

Comment by John Szakmeister [ 30/Oct/17 2:57 PM ]

It's hard to give a concrete case here--it seems to only really come out with advanced compilation. I had a routine that was returning something like this from a server: {"Br": 0, "B": 100, "P": 1000} as JSON. js->clj would trip generate the "no protocol method" error mentioned in the description on this. I found that if I moved this snippet:

(identical? (type x) js/Object)
(into {} (for [k (js-keys x)]
           [(keyfn k) (thisfn (unchecked-get x k))]))

ahead of this snippet:

(coll? x)
(into (empty x) (map thisfn x))

that the error went away--I'm not sure if it's the right thing to do though. I suspect that the keys are somehow colliding again with known things, and something is being treated like a collection when it is not. Then, when (empty x) is called, we fail because we the object doesn't implement the required protocol.

I thought this kind of situation was fixed in the past by CLJS-1658 and 7e15b1f2b894d93ef94ff86d75226f1fd3919580, but maybe something else is going on that we're hitting false positives again.

Comment by Thomas Heller [ 30/Oct/17 4:20 PM ]

This is probably not fixable without breaking the semantics of js->clj for someone else. At least I can't think of something easy.

Here is a short reproduction of whats happening:

  • coll? checks ICollection which is a fast-path protocol
  • fast-path protocols do a bit check, instead of the previously fixed sentinel check
  • bit check is done on a property named cljs$lang$protocol_mask$partition0$ which gets shortened by Closure to something unknown. Given the "short" keys in your example Object that is very likely to conflict given that Closure starts with a,b,c,...

This can be reproduced in the Node REPL:

[6:1]~cljs.user=> (def obj #js {"cljs$lang$protocol_mask$partition0$" 8})
#'cljs.user/obj
[6:1]~cljs.user=> (coll? obj)
true
[6:1]~cljs.user=> (empty obj)
eval error Error: No protocol method IEmptyableCollection.-empty defined for type object: [object Object]

One easy fix is to just forget about js->clj and use something that only supports JSON values and doesn't use protocols.

I wrote one you can use here:
https://github.com/thheller/shadow-cljs/blob/master/src/main/shadow/json.cljs#L4-L37

Maybe we should add a json->clj to core and properly document the problematic behaviour of js->clj?

Comment by John Szakmeister [ 31/Oct/17 7:36 AM ]

Maybe we should add a json->clj to core and properly document the problematic behaviour of js->clj?

I think that would be advisable. I implemented my own json->clj in my project that worked around this issue a while back, but I think something in the core would be very beneficial here.

BTW, nice reproduction recipe. It's unfortunate that advanced compilation shortens that to something that potentially conflicts.





[CLJS-2051] Add end-line and end-column to analyzer AST Created: 25/May/17  Updated: 26/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Julien Fantin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: analyzer

Attachments: Text File CLJS-2051.patch    

 Description   

Some existing Clojure tooling [1] currently built on top of tools.analyzer.jvm, depend on having end-line and end-column on an AST node.

This data is currently missing from the ClojureScript analyzer, which prevents these tools from being ported to ClojureScript [2]

[1] https://github.com/clojure-emacs/refactor-nrepl
[2] https://github.com/clojure-emacs/refactor-nrepl/issues/195#issuecomment-303910871



 Comments   
Comment by Julien Fantin [ 25/May/17 11:24 PM ]

Here is a patch that adds end-line and end-column and tries to standardize how that data is obtained from the env.

Comment by António Nuno Monteiro [ 26/May/17 12:04 AM ]

I think this is already being tackled in CLJS-1461, which goal is to achieve full compatibility with the tools.analyzer AST.

Comment by David Nolen [ 26/May/17 7:57 AM ]

CLJS-1461 is a big project and we're not sure how long it will take. In the meantime I don't see a problem with incremental steps in that direction if we get the patches.

Comment by David Nolen [ 26/May/17 1:14 PM ]

This patch looks fine but it would be nice to get some feedback that in fact source mapping is not affected.

Comment by Julien Fantin [ 26/May/17 4:16 PM ]

Unfortunately our main project depends on an older ClojureScript version so I couldn't test this on our main codebase. Are there specific things you'd watch out for?

Comment by David Nolen [ 26/May/17 4:25 PM ]

Julien, no need for you to test this, trying to get some outside help here





[CLJS-2050] js->clj breaks on Objects with the key "v" Created: 24/May/17  Updated: 25/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Ian Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

With advanced compilation enabled, a JS Object with the key "v" causes the `(seq? x)` clause of `js->clj` to return true, but since js/Object does not satisfy `ISeqable` the `map` on the following line causes an error. I've identified this issue in the circleci frontend codebase. Working on creating a minimal project test case now.



 Comments   
Comment by Ian Davis [ 24/May/17 8:27 PM ]

Was not able to generate a minimal case. It definitely fails in our repository, but cannot make it fail on a simpler case. Leaving this issue open in case anyone else encounters it, but don't expect any follow up unless there are other reports.

Comment by Thomas Heller [ 25/May/17 1:57 AM ]

The (seq? x) does a protocol check which checks if the given object has a marker property for the protocol. In your case that property was most likely renamed to x.v which then causes a hit.

This was fixed for normal protocols a while back: https://dev.clojure.org/jira/browse/CLJS-1658

But fast-path protocols (ie. ISeq) use a bit check (x.cljs$lang$protocol_mask$partition0$ & (64)) and I assume your value in v satisfies that check?

Comment by David Nolen [ 25/May/17 9:20 AM ]

I believe Thomas's analysis is correct here. I think having the `seq?` and `coll?` checks in `js->clj` was probably ill-considered but changing that will probably introduce a different kind of breakage for a different group of users. You can either provide an externs file for your JS objects and their properties (difficult to do I know if dynamic) or write a simpler custom `js->clj` which only expects JS values (no protocol checks) and use that instead.

Comment by Ian Davis [ 25/May/17 12:15 PM ]

Ok, I figured it was something like that. I considered removing the first three cases, or just reversing the order of the js and protocol checks, but I wasn't entirely sure what kind of breakage that might introduce. If we are just using the modified version in our json parser, it should be fine, right? It seems like those protocol checks are only useful if you have clojure intermixed with the json.





[CLJS-2049] Implicitly require macros from CLJC files - towards Clojure parity Created: 24/May/17  Updated: 24/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Dennis Heihoff Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: macro, namespace
Environment:

All



 Description   

When a .cljc file that contains macros is required and aliased in another namespace, macros are not automatically included. In that case the :include-macros flag must be set to true. Since the .cljc file is experienced as a single file one would expect macros to be included just like in Clojure.

David Nolen:

The reason we don't do it automatically already is because it wasn't safe to do so when macro files and runtime files were separate

in the past just because a .clj file and .cljs file had the same name didn't mean anything all - implicitly loading the .clj file was not a good idea.






[CLJS-2045] sort-by: Avoid re-creating comparator Created: 20/May/17  Updated: 25/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-2045.patch    
Patch: Code

 Description   

The fn->comparator call should be lifted:

(sort (fn [x y] ((fn->comparator comp) (keyfn x) (keyfn y))) coll)
(let [comparator (fn->comparator comp)]
  (sort (fn [x y] (comparator (keyfn x) (keyfn y))) coll))

Also, fn->comparator is again called on the function in sort, not sure how to avoid that unless we copy the sort code into sort-by.






[CLJS-2038] self calls do not optimize - regression Created: 15/May/17  Updated: 25/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: A. R Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This is a regression of:

https://dev.clojure.org/jira/browse/CLJS-275



 Comments   
Comment by A. R [ 18/May/17 2:19 AM ]

This issue can be solve by changing the defn macro for the "simple" case and carrying the function name over to the function:

(core/list 'def (with-meta name m)
  (cons `fn (cons name fdecl)))

This isn't done in clojure because of: https://dev.clojure.org/jira/browse/CLJ-809

Though I don't think that's and issue in CLJS since we don't have real vars anyways and can't redefine {{def}}s.

Comment by A. R [ 17/Jun/17 10:07 AM ]

So the issue that the CLJ ticket has is emulated/shown below in CLJS:

(enable-console-print!)

(defn self-call-test
  [n]
  (prn "inner")
  (when (pos? n)
    (self-call-test (dec n))))

(let [orig self-call-test]
  (set! self-call-test
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test 2)

(def self-call-test2
  (fn self-call-test2
       [n]
       (prn "inner")
       (when (pos? n)
         (self-call-test2 (dec n)))))

(let [orig self-call-test2]
  (set! self-call-test2
        (fn outer [n]
          (pr "outer")
          (orig n))))
(self-call-test2 2)

Output in with no optimizations:

"outer"
"inner"
"outer"
"inner"
"outer"
"inner"


"outer"
"inner"
"inner"
"inner"

So: It does seem this would also break the current behaviour, HOWEVER, the above with advance optimizations gives this:

"outer"
"inner"
"inner"
"inner"

*for both*. Given this, it seem better to not change behavior during advanced builds to avoid hard to track down production bugs for the users. Even if this is a slight deviation from CLJ behavior. Thoughts?

Comment by A. R [ 25/Jun/17 2:27 PM ]

Any thoughts on this? I can create a patch if that this change is ok. It could matter a bit (performance wise) since a few of the very low level data structure functions are recursive.





[CLJS-2018] User supplied externs not loaded with user specified compiler state Created: 25/Apr/17  Updated: 16/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Jonathan Henry Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

User supplied externs for use with warn-on-infer are only loaded in the 2-arity versions of cljs.closure/build, cljs.build.api/build and cljs.build.api/watch when compiler is nil.

Note: This only affects the warnings that are generated by the analyzer with warn-on-infer; the externs are correctly passed to gclosure.



 Comments   
Comment by Jonathan Henry [ 25/Apr/17 7:34 PM ]

This patch moves the loading of externs from cljs.env/default-compiler-env to cljs.closure/build.

Comment by Jonathan Henry [ 26/Apr/17 12:20 PM ]

Ignore this patch, I just realized this makes it so the built-in externs are no longer loaded for the compiler and analyzer API.

Comment by David Nolen [ 16/Jun/17 9:18 AM ]

I deleted the patch to avoid confusion.





[CLJS-2016] Support inheritance annotations in externs Created: 25/Apr/17  Updated: 16/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Jonathan Henry Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Closure externs may contain @extends annotations to specify base classes. The base classes' attributes should be propagated to subclasses in `:cljs.analyzer/externs`.






[CLJS-2004] Minor fix for test-simple script Created: 10/Apr/17  Updated: 12/Dec/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Dejan Josifovic Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: clojurescript, script, test
Environment:

Lubuntu 16.10


Attachments: Text File CLJS-2004.patch     Text File CLJS-2004-rebase.patch    
Patch: Code

 Description   

On Ubuntu based Linux distributions $[] doesn't work.
Output is: Tested with $[ran+1] out of 4 possible js targets
and should be: Tested with 4 out of 4 possible js targets

As in CLJS-929 (for test script), $(()) will work for ash, dash, bash, and zsh.



 Comments   
Comment by Dejan Josifovic [ 10/Apr/17 2:42 PM ]

Add patch.

Comment by Mike Fikes [ 19/Nov/17 7:32 PM ]

Patch no longer applies; needs re-baseline.

Comment by Dejan Josifovic [ 12/Dec/17 5:33 PM ]

Hi,
Sorry for the late replay.
Rebased patch is uploaded.
Regards.





[CLJS-2000] Don't log deprecation warnings on recursive calls to the same function with a different arity Created: 05/Apr/17  Updated: 20/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: warning


 Description   

If a function has two arity's where one calls the other, then if that function is marked with `^:deprecated`, then compile warnings about deprecations will always be emitted while that function exists. E.g.

(defn ^:deprecated test-deprecated
  ([]
    (test-deprecated nil))
  ([a]
    nil))

produces these logs:

WARNING: my.test/test-deprecated is deprecated. at line 3 src/my/test/error.cljs

I think that only outside references to a deprecated function should warn here. Otherwise, it's impossible to deprecate a multi-arity function and still get clean compiles.



 Comments   
Comment by Mike Fikes [ 20/Nov/17 4:03 PM ]

A workaround: You can mark call forms with ^:deprecation-nowarn to suppress deprecation warnings.

Applied to the example in this ticket:

(defn ^:deprecated test-deprecated
  ([]
    ^:deprecation-nowarn (test-deprecated nil))
  ([a]
    nil))




[CLJS-1997] Outward function type hint propagation Created: 03/Apr/17  Updated: 20/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJS-1997.patch    
Patch: Code

 Description   

New feature: Detect the type of a function and arrange so that it is available for outward type hinting.

For reference, a small amount of discussion from Jan 2016 in the #cljs-dev Slack:

mfikes: Curious if "outward" ^boolean propagation has been debated / rejected. Example: If simple analysis detects that ^boolean can be applied as in for functions like (defn f [x] (not x)). Perhaps automatically propagating type hints in this direction would be unwelcome by users?

dnolen: @mfikes: we have already have outward propagation for numerics which broke because of changes for code motion

dnolen: absolutely nothing against that



 Comments   
Comment by Mike Fikes [ 03/Apr/17 7:38 PM ]

The attached patch will handle cases like

(defn foo? [x] (or (string? x) (number? x)))

and

(def baz (fn ([x] 1) ([x y] 2)))

but it doesn't attempt to address the case of multi-arity defn s, as these are implemented by dispatching to multiple single-arity functions that could have different return types.

It adds a :inferred-ret-tag to the AST only because the arguments to or on line 18 in the patch are not reversed (only to allow explicit hints to override inferred hints, but perhaps that is overcomplicating things and the inferred tag could be dropped directly into :ret-tag.)

Bronsa helpfully pointed out in Slack that outward type hint propagation is avoided in Clojure because it breaks things like:

(defn foo [] 1)
(defn bar [] (String/valueOf (foo)))
(defn foo [] "foo")
(bar)

for which an analogy in ClojureScript would be:

(defn foo? [] true)
(defn bar [] (if (foo?) :t :f))
(defn foo? [] "")
(bar)

where the patch causes it to yield :f.

This could be viewed as a REPL-only concern (perhaps addressed by another compiler flag along the lines of :static-fns).

But, arguably the problem already exists today, with type hinting in code like the following affecting the compilation of bar:

(def foo? true)
(defn bar [] (if foo? :t :f))
(def foo? "")
(bar)
Comment by David Nolen [ 07/Apr/17 11:15 AM ]

Thanks will think about it

Comment by Mike Fikes [ 16/Jun/17 10:30 AM ]

I've been soaking this for a couple of months (with local builds of Planck), and haven't seen anything break, FWIW. So, I think it is at least a safe change. Assigning to David for further consideration.





[CLJS-1995] Possible conflict with automatic aliases for JS modules Created: 02/Apr/17  Updated: 02/Apr/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

https://clojurescript.org/guides/javascript-modules

The hello-es6 example uses a directory to apply :module-type to every file in that directory.

{:file "src" :module-type :es6}

This leads to src/js/hello.js being aliased to the js.hello ns which .cljs files can then :require.

Given a directory structure like this:

{:file "lib-a" :module-type :es6}
{:file "lib-b" :module-type :es6}

.
├── lib-a
│   └── js
│       └── hello.js
├── lib-b
│   └── js
│       └── hello.js

Leads to lib-b silently replacing lib-a as they both claim the js.hello name.

The same issue is present in closure-compliant libs but they typically follow some kind of manual namespacing (ie. goog.string, cljs.core, ...) which ES6/JS libs do not do (and do not even support given their use of relative imports vs absolute imports)

Not sure how to handle this but at the very least there should be some kind of warning that there is a conflicting alias.

Demo here: https://github.com/thheller/hello-es6-conflict






[CLJS-1990] Clojurescript programs targeting nodejs should support global installation Created: 28/Mar/17  Updated: 24/Apr/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Greg Haskins Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJS-1990-Use-module-relative-__dirname-for-bootstra.patch    

 Description   

The top-level entry point in a :target :nodejs application uses $CWD relative paths to load the bootstrapping. See "path.resolve('.')" here: https://github.com/clojure/clojurescript/blob/296d0a69340e832b92ed742b3cd0304a06bea27f/src/main/clojure/cljs/closure.clj#L1460 for example.

This works fine for a local build, but is problematic when we try to globally install clojurescript (such as via 'npm install -g') because it requires the caller $CWD to be something that is likely to be unnatural (e.g. /usr/lib/node_modules/$pkg). Suggested fix is to replace "path.resolve('.')" with "__dirname".



 Comments   
Comment by Matt Lee [ 24/Apr/17 2:34 AM ]

Also see: CLJS-1444





[CLJS-1975] Perf: Compare f with nil in Delay impl Created: 11/Mar/17  Updated: 21/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-1975.patch    
Patch: Code

 Description   

When a Delay has been realized, f is set to nil. We can avoid truth_ and not calls and directly compare with nil for a minor perf boost.

In script/noderepljs, this leads to these

(simple-benchmark [x (delay 1)] @x 1e9)
(simple-benchmark [x (delay 1)] (realized? x) 1e9)
(simple-benchmark [x (doto (delay 1) deref)] (realized? x) 1e9)

speeding up by 6%, 11% and 9%, respectively.



 Comments   
Comment by Mike Fikes [ 21/Nov/17 2:35 PM ]

Under :advanced across all engines, the 3 benchmarks in the ticket description produce these speedup numbers:

            V8: 1.18, 1.15, 1.09
  SpiderMonkey: 1.08, 1.04, 1.19
JavaScriptCore: 1.70, 0.86, 0.93
       Nashorn: 1.05, 1.15, 1.14
    ChakraCore: 1.21, 0.52, 0.81




[CLJS-1970] Cannot infer target type for list/vector expressions Created: 08/Mar/17  Updated: 10/Mar/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel Janus Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Ubuntu 16.04.2, Oracle JRE 8



 Description   

With

(set! *warn-on-infer* true)
enabled, attempting to compile functions like:

(defn foo [] (list))
(defn bar [] (vector))

results in a warning:

WARNING: Cannot infer target type in expression (. cljs.core/List -EMPTY) at line 2427 src/cljs/discann/core.cljs

WARNING: Cannot infer target type in expression (. cljs.core/PersistentVector -EMPTY) at line 2435 src/cljs/discann/core.cljs

The line number reported is totally unrelated to the line of code where the problematic fn appears.

Affects 1.9.456 and 1.9.494.






[CLJS-1965] letfn collisions across namespaces Created: 02/Mar/17  Updated: 05/Jan/18

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Jeremy R Sellars Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Affects 1.9.495 and earlier. Probably only affects browsers (not nodejs).


Attachments: Text File CLJS-1965-failing-tests.patch     Text File cljs-1965_wrap_letfn_statements.patch    
Patch: Code

 Description   

If you `letfn` a fn with the same name in two namespaces, the wrong fn is used.

one.cljs
(ns hello-world.one)

(letfn [(answer [] "1")]
  (defn get-answer []
    (answer)))
two.cljs
(ns hello-world.two)

(letfn [(answer [] "2")]
  (defn get-answer []
    (answer)))
core.cljs
(ns hello-world.core
  (:require [hello-world.one]
            [hello-world.two]))

(println "one =>" (hello-world.one/get-answer))  ; one => 1
(println "two =>" (hello-world.two/get-answer))  ; two => 1      WHAT?!?

This issue seems to exist both on top-level `letfn`s and non-top-level `(let [] (letfn [...]))`.



 Comments   
Comment by Jeremy R Sellars [ 02/Mar/17 4:21 PM ]

This patch wraps `letfn` :expr and :statement forms in a function declaration (formerly, only :expr forms were wrapped).

I only did minimal testing. It fixes the code from the description.

Note: This is my first crack at the compiler and it is entirely possible I have not understood the entire problem.

Comment by Mike Fikes [ 16/Jun/17 10:54 AM ]

Confirmed that this fixes things downstream in self-hosted ClojureScript (Planck):

Without the patch:

cljs.user=> (require 'hello-world.core)
one => 2
two => 2
nil

With the patch:

cljs.user=> (require 'hello-world.core)
one => 1
two => 2
nil
Comment by daniel sutton [ 29/Dec/17 3:18 PM ]

This doesn't require different namespaces. The bug is that `let-fn` is putting its binding as a global variable.

And easy reproduction is
1. `lein new mies letfn-bug`,
2. update cljs version to `[org.clojure/clojurescript "1.9.946"]`
3. and then

(ns letfn-bug.core
  (:require [clojure.browser.repl :as repl]))

(enable-console-print!)

(letfn [(non-unique-name [] 4)]
  (defn f1 [] (non-unique-name)))

(letfn [(non-unique-name [] 5)]
  (defn f2 [] (non-unique-name)))

(println "should be 4-> " (f1))
(println "should be 5-> " (f2))

then `scripts/repl`.

results in:

cljs.user=> (load-file "letfn_bug/core.cljs")
should be 4->  5
should be 5->  5
nil
cljs.user=>

With the generated js:

// Compiled by ClojureScript 1.9.946 {}
goog.provide('letfn_bug.core');
goog.require('cljs.core');
goog.require('clojure.browser.repl');
cljs.core.enable_console_print_BANG_.call(null);
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (4);
});
letfn_bug.core.f1 = (function letfn_bug$core$f1(){
return non_unique_name.call(null);
});
var non_unique_name = (function letfn_bug$core$non_unique_name(){
return (5);
});
letfn_bug.core.f2 = (function letfn_bug$core$f2(){
return non_unique_name.call(null);
});
cljs.core.println.call(null,"should be 4-> ",letfn_bug.core.f1.call(null));
cljs.core.println.call(null,"should be 5-> ",letfn_bug.core.f2.call(null));
Comment by David Nolen [ 05/Jan/18 10:42 AM ]

Quick review of the patch, instead of always wrapping in a statement context it might be better if we only did it at the top-level where this is actually a problem.





[CLJS-1963] cljs.analyzer/load-core is called for every analyzed form Created: 01/Mar/17  Updated: 25/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

In cljs.analyzer/analyze-form the load-core fn is called. load-core guards against doing its work multiple times. It then always calls (intern-macros 'cljs.core), which also checks whether it was called before. This ends up doing the checks very often. load-core should probably be called in a less frequent manner.

Performance impact is very minimal but I did a quick test in my work project and load-core is called 416671 times there (without cache) when 1 would be enough.



 Comments   
Comment by Mike Fikes [ 25/Jun/17 4:22 PM ]

Previously, it used to not always call (intern-macros 'cljs.core), with this changing with this commit: https://github.com/clojure/clojurescript/commit/7025bd212fb925cb90db680aa7a5eb3f4c0de4bb





[CLJS-1933] Support CLJS browserless remote REPL from nodejs Created: 09/Feb/17  Updated: 09/Feb/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Mike Longworth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, remote, repl
Environment:

Dev machine OSX - build and Cursive editing and REPL
Remote: RaspberryPI - nodejs
compiler out-files shared between devices with OSXFUSE.



 Description   

I would like to develop clojurescript for a remote nodejs target compiling cljs and running a REPL on my development machine.
https://github.com/clojure/clojurescript/wiki/Remote-REPL suggests a way of doing this however:

!) I haven't managed to get this working.
2) I don't like that the solution relies identical absolute file paths for the compiler output, better to have matching relative paths.

I made a post to request help with this but haven't managed to resolve all the issues:
https://groups.google.com/forum/#!topic/clojurescript/Y4ajOcej8Qo

I have made some progress since the post:
1) I dug into the cljs.repl.node source and found I can stop the node hang I reported by specifying repl-env :debug-port, and get:

Clojure 1.8.0
Debugger listening on port 5002
ClojureScript Node.js REPL server listening on 5001
TypeError: Cannot read property 'nameToPath' of undefined
at Object.goog.require (repl:2:49)
at repl:1:-56
at Object.exports.runInThisContext (vm.js:54:17)
at Domain.<anonymous> ([stdin]:50:34)
at Domain.run (domain.js:221:14)
at Socket.<anonymous> ([stdin]:49:25)
at emitOne (events.js:77:13)
at Socket.emit (events.js:169:7)
at readableAddChunk (_stream_readable.js:146:16)
at Socket.Readable.push (_stream_readable.js:110:10)
TypeError: goog.provide is not a function
at repl:1:-56
at Object.exports.runInThisContext (vm.js:54:17)
at Domain.<anonymous> ([stdin]:50:34)
at Domain.run (domain.js:221:14)
at Socket.<anonymous> ([stdin]:49:25)
at emitOne (events.js:77:13)
at Socket.emit (events.js:169:7)
at readableAddChunk (_stream_readable.js:146:16)
at Socket.Readable.push (_stream_readable.js:110:10)
at TCP.onread (net.js:523:20)
To quit, type: :cljs/quit

This looks like some kind of path problem but I haven't managed to resolve it.

I did some investigations with my original relative-path setup to try and identify the issues:
1) I eliminated absolute paths from the compile output by disabling analysis caching.
2) I ran wireshark on the REPL port and found that absolute paths were being sent by the REPL, this currently makes the relative path option unworkable.

I have many gaps in my knowledge of the REPL operation at the moment and I don't know what the best approach is to getting a good solution for a browserless remote repl setup.



 Comments   
Comment by David Nolen [ 09/Feb/17 12:33 PM ]

It's probably going to be easier to discuss this issue in IRC or Slack first. There's just too many different issues piled into this one. Thanks.





[CLJS-1926] Changes to namespace metadata are not properly transferred to *ns* dynamic var Created: 02/Feb/17  Updated: 02/Feb/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A CLJS macro may want to access the metadata of the current ns via (meta *ns*).

Changes to the ns metadata are not reflected back the *ns* var when re-compiling a namespace, the metadata of the first compile will remain. This is due to the analyzer always calling create-ns but never updating the meta. It should probably be updated inside parse 'ns [1]. Clojure always resets the metadata via the ns macro.

One potential conflict is when a .clj and a .cljs file exist for the same namespace and both provide different metadata. Both platforms reseting the meta is probably not ideal, maybe we should vary-meta merge instead?

[1] https://github.com/clojure/clojurescript/blob/94b4e9cdc845c1345d28f8e1a339189bd3de6971/src/main/clojure/cljs/analyzer.cljc#L2312






[CLJS-1924] The compiler cannot infer the target type of "(. RecordName -prototype)" expressions Created: 01/Feb/17  Updated: 01/Feb/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Viktor Magyari Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: compiler, extern


 Description   

Repro:

Place

(set! *warn-on-infer* true)

(defrecord Foo [])

anywhere in your source files, compile with :infern-externs true.

Expected:

Multiple warnings like:

  • WARNING: Cannot infer target type in expression (. Foo -prototype)
  • WARNING: Cannot infer target type in expression (. other__8838__auto__ -constructor)
  • WARNING: Cannot infer target type in expression (. user/Foo -getBasis)

There are also warnings for (. cljs.core/List -EMPTY), but this might be unrelated.






[CLJS-1917] `case` doesn't handle matching against lists Created: 28/Jan/17  Updated: 28/Jan/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The following works in Clojure but not ClojureScript

(let [foo '(:a :b)]
  (case foo
    '(:a :b) :works))





[CLJS-1913] Investigate slow reading / compilation of CLJC files Created: 27/Jan/17  Updated: 27/Jan/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: António Nuno Monteiro Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None





[CLJS-1908] Improve error messages by using pr-str instead of str when printing objects Created: 26/Jan/17  Updated: 05/Oct/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Daniel Compton
Resolution: Unresolved Votes: 6
Labels: errormsgs

Attachments: Text File 0001-CLJS-1908-Wrap-vars-in-throw-message-with-pr-str.patch    
Patch: Code

 Description   

Many error messages from ClojureScript include the invalid argument like this:

(throw (js/Error. (str "Doesn't support name: " x)))

If x is nil, then the error message produces is "Doesn't support name: " which is a bit mystifying to debug. If x was wrapped with pr-str then the error message would be the much more understandable: "Doesn't support name: nil".

If there's interest in this, then I can prepare a patch which wraps these kinds of errors with pr-str.



 Comments   
Comment by David Nolen [ 16/Jun/17 10:30 AM ]

Go for it

Comment by Mike Fikes [ 16/Jun/17 10:33 AM ]

I also pondered this for a bit with CLJS-2089. Seems like the right thing to do in general.





[CLJS-1902] Add support for compiler option :inline-source-maps Created: 24/Jan/17  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Antonin Hildebrand Assignee: Antonin Hildebrand
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File CLJS-1902.patch    
Patch: Code and Test

 Description   

1. refactor `emit-source-map` and break it into multiple functions
2. fix logic for relative path computation (see `strip-prefix-path`)
3. add support for `:inline-source-maps` option
4. add tests

Related: CLJS-1402, CLJS-1901



 Comments   
Comment by Antonin Hildebrand [ 24/Jan/17 8:09 PM ]

Patches:
https://github.com/darwin/clojurescript/commit/de1da63072c8049b9812d41cce750e5a972c0b0b.patch
https://github.com/darwin/clojurescript/commit/2389fb7839a18299d842d381b135a669a7091869.patch
https://github.com/darwin/clojurescript/commit/3ba594d8c7215ef8c8276737b7e5d8008c6b3a98.patch

Comment by Antonin Hildebrand [ 24/Jan/17 8:27 PM ]

Full review: https://github.com/clojure/clojurescript/compare/darwin:inline-source-maps~3...darwin:inline-source-maps

Also please note that the first patch testing original functionality fails in one test because there was a bug in timestamp formatting in :source-map-url case:
https://github.com/clojure/clojurescript/compare/master...darwin:inline-source-maps#diff-55b85385d2d0bfb6dc20d59ed982d5c8L1239

Comment by Antonin Hildebrand [ 25/Jan/17 10:25 AM ]

Today when testing Dirac I realised we need to embed sources contents as well.

Additional patch:
https://github.com/darwin/clojurescript/commit/c1df38f14a33d02fe2d421f80db0b421b17286bb.patch

New review URL: https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps

Tested in DevTools and works like a charm.

Comment by Dusan Maliarik [ 06/Feb/17 10:00 AM ]

This would be helpful for us as well.

Comment by Andrea Richiardi [ 01/May/17 3:49 PM ]

I have run across this one as well by following this tutorial.

Without either this patch or Dirac's complicated setup it is not currently possible to use node --inspect and debug correctly. The symptom I see on our side is that source maps are detected but for some reason Chrome DevTools does not show them in the Tree View.

The content of one of it is:

{"version":3,"file":"\/Users\/user\/cqrs-engine-cljs\/out\/cqrs\/event_store.js","sources":["event_store.cljs"], ...
Comment by David Nolen [ 16/Jun/17 10:29 AM ]

Linking to patches outside of JIRA is not proper for tickets. Please add a single squashed patch to this ticket directly.

Comment by Antonin Hildebrand [ 16/Jun/17 2:25 PM ]

Attached it as a patch file.

Took https://github.com/darwin/clojurescript/compare/inline-source-maps~4...darwin:inline-source-maps.diff and applied it to current master. It applied cleanly without conflicts. Tests are still passing on my machine.

Comment by Mike Fikes [ 19/Nov/17 7:36 PM ]

Patch no longer applies; needs re-baseline.





[CLJS-1901] Investigate new Google Closure source mapping support Created: 24/Jan/17  Updated: 05/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.655
Fix Version/s: None

Type: Task Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Google Closure now contains comprehensive support for (at least from the command line) for source map merging and inline source map generation. We should investigate how reusable this functionality actually is.



 Comments   
Comment by Antonin Hildebrand [ 24/Jan/17 3:30 PM ]

Investigated it a bit, just sharing what I learned so far:

1. historically there used to be a hidden flag `--source_map_input` which could be used to produce source-map-aware error reporting, not source map composition as name would suggest[1]
2. mid 2016, a patch landed[2], which enhanced this for full source map composition
3. by the end of 2016, the feature seems to be public and enabled in command-line tool by default[3][5]
4. as of today, the official source-maps wiki page[4] has not been updated to reflect this latest development

[1] https://github.com/google/closure-compiler/issues/1360#issuecomment-170716968
[2] https://github.com/google/closure-compiler/pull/1971
[3] https://github.com/google/closure-compiler/pull/2008
[4] https://github.com/google/closure-compiler/wiki/Source-Maps
[5] https://github.com/google/closure-compiler/pull/2129

Comment by Antonin Hildebrand [ 24/Jan/17 3:53 PM ]

Closure compiler also newly understands inlined source maps using data URLs in input Javascript files[1].

1. parsing of inline source maps is enabled by default unless `--parse_inline_source_maps=false` is passed, it is independent on `--source_map_input` flag
2. information from `--source_map_input` and inlined source-maps is merged, inlined maps override `--source_map_input`, the last inlined map wins in case of multiple //# sourceMappingURL=<data URL> present [2]

[1] https://github.com/google/closure-compiler/pull/1982
[2] https://github.com/google/closure-compiler/pull/1982#issuecomment-243249065

Comment by Thomas Heller [ 02/May/17 5:23 AM ]

FWIW I added support for this in shadow-build a while ago. It does not need inline source maps to work.

The code can be found here: https://github.com/thheller/shadow-build/blob/master/src/main/shadow/cljs/closure.clj

The relevant bits are .addInputSourceMap on Compiler and .setApplyInputSourceMaps on CompilerOptions.

If everything is properly configured the warnings displayed by Closure will contain an "Originally at:" location which points to the CLJS file.

Closure will also use the input source maps when generating source maps for :advanced builds, so the manual merge done by CLJS at the moment becomes unnecessary. The source maps also appear to be more accurate. Before input source maps I had a few issues where source maps were off by a few lines, but that may have been due to my incorrect source map handling in shadow-build.





[CLJS-1899] Local bindings conflict with global JS namespace Created: 24/Jan/17  Updated: 24/Jan/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None


 Description   

Not sure if it's a bug or expected behaviour, but this:

(defn test-fn []
  (let [href     js/location.href
        location "123"]
    href))

gets compiled to this (not in advanced mode):

cognician.chat.ui.pages.insights.test_fn = (function cognician$chat$ui$pages$insights$test_fn(){
var href = location.href;
var location = "123";
return href;
});

and local location var shadows global I'm trying to access in location.href.

That sort of thing is expected and one should pay attention and work around stuff like this in JS, but in CLJS it's very confusing because nothing hints what am I doing wrong and why that code fails. I remember one of ClojureScript goals was to fix JS semantics, so maybe there's a way this might be addressed? At least throw a warning, maybe?



 Comments   
Comment by Thomas Heller [ 24/Jan/17 5:04 AM ]

This came up recently on the #cljs-dev slack channel. There is definitely a bug somewhere.

(let [href     js/location.href
      location "123"]
  href)

produces

var href_51444 = location.href;
var location_51445 = "123"; // << correct

So it works at the top level, but when inside a defn (and others) we get

(ns test)
(defn test-fn []
  (let [href     js/location.href
        location "123"]
    href))
test.test_fn = (function test$test_fn(){
var href = location.href;
var location = "123"; // << incorrect
return href;
});
Comment by David Nolen [ 24/Jan/17 7:27 AM ]

Taking a quick look it seems that maybe we aren't checking `:js-globals` consistently and often only looking at locals? Also now that externs inference is a thing we should probably compute `:js-globals` from all known externs instead of the obviously incomplete list we currently have in place.





[CLJS-1888] Seqs of PHMs and PAMs do not handle metadata correctly Created: 13/Jan/17  Updated: 16/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Mulvaney Assignee: Thomas Mulvaney
Resolution: Unresolved Votes: 0
Labels: metadata

Attachments: Text File CLJS-1888.patch    

 Description   

Metadata on parent seq ends up being passed to the next seq. Calling `empty` on a seq also ends up carrying metadata.

Examples:

(def s (with-meta (seq {:a 1 :b 2}) {:some :meta}))

(meta s) => {:some :meta} ;; Good
(meta (rest s))  => {:some :meta} ;; Bad, expected nil
(meta (next s))  => {:some :meta} ;; Bad, expected nil
(meta (empty s)) => {:some :meta} ;; Bad, expected nil


 Comments   
Comment by David Nolen [ 16/Jun/17 10:16 AM ]

Patch no longer applies to master.





[CLJS-1881] Improve cljs.core/distinct perf by using transient map Created: 25/Dec/16  Updated: 29/Dec/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File cljs-1881-transient-in-distinct.patch    
Patch: Code

 Description   

Current implementation of cljs.core/distinct uses persistent set. This patch improves performance by ~10%-33% by using transient map instead. Mirror Clojure task CLJ-2090

10 elements
(reduce + 0 (distinct coll))     12.360220502805724 µs => 9.504153281757874 µs (-23%)
(transduce (distinct) + 0 coll)  7.689213711227641 µs => 5.3549045227207 µs (-30%)
100 elements
(reduce + 0 (distinct coll))     136.43424283765356 µs => 106.66990187713321 µs (-21%)
(transduce (distinct) + 0 coll)  73.05427319211107 µs => 48.737280701754386 µs (-33%)
1000 elements
(reduce + 0 (distinct coll))     1.1207102908277415 ms => 919.8952205882359 µs (-17%)
(transduce (distinct) + 0 coll)  677.2834912043312 µs => 482.79681467181547 µs (-28%)
10000 elements
(reduce + 0 (distinct coll))     4.777295238095228 ms => 4.3203448275862115 ms (-9%)
(transduce (distinct) + 0 coll)  2.889020114942531 ms => 2.44890487804879 ms (-15%)

Benchmarking code: https://gist.github.com/tonsky/258c3d715e6a485522f7ba5e663624fd






[CLJS-1871] A declare with :arglists should generate static function calls Created: 14/Dec/16  Updated: 15/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: performance


 Description   

This is performance enhancement.

  1. Problem
    After a declare the compiler doesn't know which arities the function will be defined with and hence generates code that checks if that arity is defined and then either calls it or uses a slower `xy-fn.call(null, ...)` construct. This is not optimal since it can be slower and also generates slightly more code.

Especially functions which only have one arity are problematic since they will end up being called with `xy-fn.call`.

  1. Affects
    Code that uses a function that was only declared and not def'ed yet. Such as `cons` in `IndexedSeq` or `conj!` in `TransientHashMap`.
    1. Performance
      A preliminary benchmark showed neglible improvements in Chrome v54 but a significant (factor of 2.2) performance benefit in Firefox.
  1. Solution
    Most of the declares should use `(def ^{:declare true, :arglists '([x xs])} cons)` and the compiler should take the `:arglists` into consideration and emit direct function calls instead.


 Comments   
Comment by A. R [ 12/May/17 8:26 AM ]

Similarly, functions that call themselves recursively don't get invoked optimally. Such as:

  • push-tail
  • do-assoc
  • pop-tail
  • tv-push-tail
  • tv-pop-tail

Matters quite a bit for TreeMap kv-reduce + dissoc.

EDIT: Separately addressed: https://dev.clojure.org/jira/browse/CLJS-2038





[CLJS-1866] RangedIterator performance tweaks Created: 08/Dec/16  Updated: 19/Dec/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Thomas Mulvaney
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS-1866.patch     Text File CLJS-1866-updated.patch     Text File CLJS-1866-updated.patch    
Patch: Code

 Description   

The attached patch simplifies and speeds up the RangedIterator.

The benchmarks were run using the following function to test vector iteration:

(defn consume-iterator
  [v]
  (let [iter (-iterator v)]
    (loop []
      (when (.hasNext iter)
        (.next iter)
        (recur)))))

A series of "simple-benchmarks" were setup as follows:

(simple-benchmark [v (into [] (range N))] (consume-iterator v) I)

Where 'N' and 'I' were values from the 'Vector Size' and 'Iterations' columns of the table below .

Vector Size Iterations V8 Speed [msec] (master) V8 Speed [msec] (patch) JSC Speed [msec] (master) JSC Speed [msec] (patch)
1 100,000 15 11 13 7
2 100,000 14 10 7 4
4 100,000 18 10 9 5
8 100,000 27 12 14 6
16 100,000 43 17 19 9
32 100,000 74 24 37 15
100 100,000 217 59 105 45
1000 100,000 2008 524 1032 392
10,000 100,000 20390 5856 10249 4178
100,000 10,000 20334 5324 10324 4387

Javascript engine versions used:

  • V8 version 5.1.281.47
  • JSC version Unknown

The RangedIterator constructor function `ranged-iterator` was also made private.



 Comments   
Comment by David Nolen [ 16/Dec/16 2:04 PM ]

Let's get a patch with the performance change without altering the constructor first. Thanks.

Comment by Thomas Mulvaney [ 17/Dec/16 7:15 AM ]

Rebased and constructor no longer private.

Comment by David Nolen [ 17/Dec/16 9:59 AM ]

Sorry for not being clear. Leave the fields of the deftype alone even if we aren't using them for now. We want the performance enhancements without changing the API at all.

Comment by Thomas Mulvaney [ 19/Dec/16 5:58 AM ]

Thanks that makes sense. Fixed in this patch.





[CLJS-1864] timestamped source maps broken with Node Created: 29/Nov/16  Updated: 13/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Dmitr Sotnikov Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None


 Description   

I'm using Figwheel with Node, and noticed a bug with timestamped source maps. When the :source-map-timestamp compiler flag is set, the sourceMappingURL is set to source.js.map?timestamp.

This works fine in the browser, but breaks in Node where files are loaded from the filesystem. It looks like a simple workaround would be to check if :target is :node and output something like source.js.timestamp.map instead, and use it directly as the value of sourceMappingURL.

Here's a change I made locally in cljs.compiler/emit-source-map that allows source maps to be resolved on Node when using timestamps:

emit-source-map
(defn emit-source-map [src dest sm-data opts]
     (let [timestamp (System/currentTimeMillis)
           filename (str (.getPath ^File dest)
                         (when (and
                                 (true? (:source-map-timestamp opts))
                                 (= (:target opts) :nodejs))
                           (str "." timestamp))
                         ".map")
           sm-file  (io/file filename)]
       (if-let [smap (:source-map-asset-path opts)]
         (emits "\n//# sourceMappingURL=" smap
                (string/replace (util/path sm-file)
                                (str (util/path (io/file (:output-dir opts))))
                                "")
                (if (and (true? (:source-map-timestamp opts))
                         (not= (:target opts) :nodejs))
                  (str
                    (if-not (string/index-of smap "?") "?" "&")
                    "rel=" timestamp)
                  ""))
         (emits "\n//# sourceMappingURL="
                (or (:source-map-url opts) (.getName sm-file))
                (if (and (true? (:source-map-timestamp opts))
                         (not= (:target opts) :nodejs))
                  (str "?rel=" timestamp)
                  "")))
       (spit sm-file
             (sm/encode {(url-path src) (:source-map sm-data)}
                        {:lines                   (+ (:gen-line sm-data) 2)
                         :file                    (url-path dest)
                         :source-map-path         (:source-map-path opts)
                         :source-map-timestamp    (:source-map-timestamp opts)
                         :source-map-pretty-print (:source-map-pretty-print opts)
                         :relpaths                {(util/path src)
                                                   (util/ns->relpath (first (:provides opts)) (:ext opts))}}))))


 Comments   
Comment by David Nolen [ 30/Nov/16 8:32 AM ]

Does Node.js have source map caching issues? The timestamp feature was created for caching issues present in web browsers.

Comment by Dmitr Sotnikov [ 30/Nov/16 8:39 AM ]

I tried it with :source-map-timestamp set to false, and source maps got out of sync when Cljs sources were reloaded.

Comment by David Nolen [ 30/Nov/16 10:01 AM ]

Okay. This issue will require more investigation into Node.js source mapping support before pursuing anything. As the behavior is understood, information should be added here.

Comment by Dmitr Sotnikov [ 30/Nov/16 2:56 PM ]

Sounds like a plan.

Comment by David Nolen [ 30/Nov/16 7:25 PM ]

OK I took a look at the implementation of source-map-support, it does indeed cache the source map. However the proposed idea here isn't comprehensive enough. We need to change all the places where :source-map-timestamp is used in the source code. Patch is welcome.

Comment by Dmitr Sotnikov [ 30/Nov/16 7:28 PM ]

Yeah, I noticed the key is used in a few places. I can definitely take a look at making a patch in the near future if the approach looks good to you.

Comment by Dmitr Sotnikov [ 28/Jan/17 12:39 PM ]

It looks like the approach of adding a timestamp introduces some problems. Generating unique file names would mean that old files have to be cleaned up somehow, since the new files will no longer overwrite them. Having to keep track of that isn't ideal. Perhaps it would be better to see if there's a way to prevent Node from caching the source maps.

Comment by Andrea Richiardi [ 13/Nov/17 12:57 AM ]

Hello folks I was exploring options for this and I went ahead and read the Source Map Options: it looks like a solution could be similar to what Meteor does, which is to load source maps in memory so that there would be no need to write files. We are in the REPL already so this should really be straightforward to implement, if I am not missing anything of course.





[CLJS-1863] :reload/:reload-all issue with .cljc runtime/macro nses Created: 29/Nov/16  Updated: 29/Nov/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Brandon Bloom discovered an issue where a ns that is used both from runtime and for macros (where the macros are self-required) won't respect `(require ... :reload/:reload-all)`.



 Comments   
Comment by David Nolen [ 29/Nov/16 3:59 PM ]

Whatever we do needs to be copied over into a bootstrapped, but that's a separate issue.





[CLJS-1843] EDN analysis cache may write unusable data Created: 08/Nov/16  Updated: 08/Nov/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

EDN has a built-in default writer for all objects, this may cause the cache to write something like

#object[Thing "thing-str"]
that cannot be read to construct an actual Thing instance.

This leads to an issue when trying to use the analysis data since it will contain different things when coming from cache or not.

This issue was highlighted by transit since it has no default writer and didn't know how to encode JSValue. [1] Instead of writing something unusable it failed early.

The cache write should rather gracefully fail (and warn) instead of writing unusable data or exploding.

[1] http://dev.clojure.org/jira/browse/CLJS-1666






[CLJS-1832] destructuring with #js at :or breaks the compilation when transit is part of the project Created: 23/Oct/16  Updated: 23/Oct/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.293
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Wilker Lúcio da Silva Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Since Om `1.9.293` I was having some compilation issues, I was able to narrow it down to this code:

(defn f [{:keys [a] :or {a #js {}}}])

The code above fails to compile when [com.cognitect/transit-clj "0.8.290"] is part of the project dependencies. The problem seems to happen when we try to destructure data at function arguments, using `:or` and having `#js` at part of the `:or`.

I put up a repository with a minimal case here: https://github.com/wilkerlucio/cljs-compilation-fail

Error stack when compiling:

Wilkers-MacBook-Pro:cljs-compile-bug wilkerlucio$ lein clean && lein cljsbuild once site
Compiling ClojureScript...
Compiling "resources/public/site/site.js" from ["src"]...
Compiling "resources/public/site/site.js" failed.
clojure.lang.ExceptionInfo: failed compiling file:src/cljs_compile_bug/core.cljs {:file #object[java.io.File 0x21399e53 "src/cljs_compile_bug/core.cljs"]}
        at clojure.core$ex_info.invokeStatic(core.clj:4725)
        at clojure.core$ex_info.invoke(core.clj:4725)
        at cljs.compiler$compile_file$fn__4104.invoke(compiler.cljc:1410)
        at cljs.compiler$compile_file.invokeStatic(compiler.cljc:1376)
        at cljs.compiler$compile_file.invoke(compiler.cljc:1356)
        at cljs.closure$compile_file.invokeStatic(closure.clj:432)
        at cljs.closure$compile_file.invoke(closure.clj:423)
        at cljs.closure$eval6005$fn__6006.invoke(closure.clj:499)
        at cljs.closure$eval5941$fn__5942$G__5930__5949.invoke(closure.clj:389)
        at cljs.closure$compile_task$fn__6096.invoke(closure.clj:779)
        at cljs.closure$compile_task.invokeStatic(closure.clj:777)
        at cljs.closure$compile_task.invoke(closure.clj:770)
        at cljs.closure$parallel_compile_sources$fn__6102.invoke(closure.clj:806)
        at clojure.lang.AFn.applyToHelper(AFn.java:152)
        at clojure.lang.AFn.applyTo(AFn.java:144)
        at clojure.core$apply.invokeStatic(core.clj:657)
        at clojure.core$with_bindings_STAR_.invokeStatic(core.clj:1963)
        at clojure.core$with_bindings_STAR_.doInvoke(core.clj:1963)
        at clojure.lang.RestFn.invoke(RestFn.java:425)
        at clojure.lang.AFn.applyToHelper(AFn.java:156)
        at clojure.lang.RestFn.applyTo(RestFn.java:132)
        at clojure.core$apply.invokeStatic(core.clj:661)
        at clojure.core$bound_fn_STAR_$fn__6761.doInvoke(core.clj:1993)
        at clojure.lang.RestFn.invoke(RestFn.java:397)
        at clojure.lang.AFn.run(AFn.java:22)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
        at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.RuntimeException: clojure.lang.ArityException: Wrong number of args (1) passed to: analyzer/fn--1412/fn--1413
        at com.cognitect.transit.impl.WriterFactory$1.write(WriterFactory.java:64)
        at cognitect.transit$write.invokeStatic(transit.clj:149)
        at cognitect.transit$write.invoke(transit.clj:146)
        at cljs.analyzer$write_analysis_cache.invokeStatic(analyzer.cljc:3320)
        at cljs.analyzer$write_analysis_cache.invoke(analyzer.cljc:3307)
        at cljs.compiler$emit_source.invokeStatic(compiler.cljc:1307)
        at cljs.compiler$emit_source.invoke(compiler.cljc:1237)
        at cljs.compiler$compile_file_STAR_$fn__4081.invoke(compiler.cljc:1328)
        at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1159)
        at cljs.compiler$with_core_cljs.invoke(compiler.cljc:1150)
        at cljs.compiler$compile_file_STAR_.invokeStatic(compiler.cljc:1317)
        at cljs.compiler$compile_file_STAR_.invoke(compiler.cljc:1313)
        at cljs.compiler$compile_file$fn__4104.invoke(compiler.cljc:1398)
        ... 25 more
Caused by: clojure.lang.ArityException: Wrong number of args (1) passed to: analyzer/fn--1412/fn--1413
        at clojure.lang.AFn.throwArity(AFn.java:429)
        at clojure.lang.AFn.invoke(AFn.java:32)
        at cognitect.transit$write_handler$reify__1328.tag(transit.clj:79)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:147)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.AbstractEmitter.emitArray(AbstractEmitter.java:82)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:164)
        at com.cognitect.transit.impl.AbstractEmitter.emitArray(AbstractEmitter.java:87)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:164)
        at com.cognitect.transit.impl.AbstractEmitter.emitTagged(AbstractEmitter.java:34)
        at com.cognitect.transit.impl.AbstractEmitter.emitEncoded(AbstractEmitter.java:59)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:169)
        at com.cognitect.transit.impl.AbstractEmitter.emitArray(AbstractEmitter.java:87)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:164)
        at com.cognitect.transit.impl.AbstractEmitter.emitTagged(AbstractEmitter.java:34)
        at com.cognitect.transit.impl.AbstractEmitter.emitEncoded(AbstractEmitter.java:59)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:169)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.JsonEmitter.emitMap(JsonEmitter.java:158)
        at com.cognitect.transit.impl.AbstractEmitter.emitMap(AbstractEmitter.java:70)
        at com.cognitect.transit.impl.AbstractEmitter.marshal(AbstractEmitter.java:166)
        at com.cognitect.transit.impl.AbstractEmitter.marshalTop(AbstractEmitter.java:193)
        at com.cognitect.transit.impl.JsonEmitter.emit(JsonEmitter.java:28)
        at com.cognitect.transit.impl.WriterFactory$1.write(WriterFactory.java:61)
        ... 37 more
Subprocess failed





[CLJS-1827] Improve reader performance on Firefox/Windows Created: 20/Oct/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.76
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Michael Sperber Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance, reader
Environment:

Firefox on Windows


Attachments: Text File cljs-reader.patch    
Patch: Code

 Description   

cljs.reader/read-string speeds up by a factor of 2 on Firefox/Windows through this change without complicating the code.

(Other JS engines, including Firefox on Linux/Mac do not seem to be affected as significantly.)



 Comments   
Comment by David Nolen [ 20/Oct/16 10:33 AM ]

It would be nice to have a bit more information on this ticket as to what Google Closure does that's unnecessary or whether this path is actually a faithful port of Clojure behavior (copies the implementation of the EDN reader in these hot spots??). Finally the patch names David Frese, have they submitted a CA?

Thanks!

Comment by Michael Sperber [ 21/Oct/16 5:49 AM ]

I believe the Google functions are too general, work on strings in addition to characters etc.

It's not clear to us though why only Firefox on Windows benefits.

(David Frese is a co-worker - yes, has submitted a CA.)

Comment by Mike Fikes [ 19/Nov/17 7:41 PM ]

Patch no longer applies; needs re-baseline.
Also, doesn't work with git am; recommend creating patch using https://clojurescript.org/community/patches





[CLJS-1810] Refactoring of find-and-cache-best-method Created: 05/Oct/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Andrey Zaytsev Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-refactor-find-and-cache-best-method.patch    
Patch: Code

 Description   

find-and-cache-best-method was pretty messy and confusing. cache reset is done in -get-method fn itself and it was basically a dead code. find-best-method is the replacement of it and operates with immutable data instead of internal multimethod's mutable state.
prefers* function didn't mutate the atom too, so now it takes an immutable value.
dominates is now an internal helper of find-best-method since it is private and not used by anything else.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 7:43 PM ]

Patch no longer applies; needs re-baseline.





[CLJS-1806] build api fails to generate inline code for :target :nodejs Created: 01/Oct/16  Updated: 09/Dec/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.76
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None


 Description   

when inline code is provided as vector to the composition of `cljs.build.api/build` and `cljs.build.api/inputs` methods under `:target :nodejs` the provided inline code is not output.

;; this outputs code
(cljs.build.api/build (cljs.build.api/inputs '[(ns hello.core) (+ 1 2)]) {}) 

;; this does not output inline code
(cljs.build.api/build (cljs.build.api/inputs '[(ns hello.core) (+ 1 2)]) {:target :nodejs}) 


;; When you don't use cljs.build.api/inputs everything works correctly
(cljs.build.api/build '[(ns hello.core) (+ 1 2)] {}) ;; this outputs code

(cljs.build.api/build '[(ns hello.core) (+ 1 2)] {:target :nodejs}) ;; this outputs code


 Comments   
Comment by Bruce Hauman [ 30/Oct/16 11:31 AM ]

From @ykomatsu on Github:

add-preloads seems to remove cljs/nodejs.cljs.

https://github.com/clojure/clojurescript/blob/ab7a4911f1fd3a81210b1a9f2d84857748f8268b/src/main/clojure/cljs/closure.clj#L897

This patch will fix this problem but I am not sure if this is correct solution.

https://github.com/ykomatsu/clojurescript/commit/fc986467e66e6a628dc8f0e8a2ef2b30f715fd23

Comment by Dusan Maliarik [ 09/Dec/16 2:15 PM ]

Would anyone from the team please look at the patch? Thank you

Comment by David Nolen [ 09/Dec/16 6:22 PM ]

Please attach a patch to the ticket for review. Linking out of JIRA is not desirable. Thanks.





[CLJS-1784] Cleanup set creation functions Created: 20/Sep/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1784.patch    
Patch: Code

 Description   

Use .fromArray for consistency/speed when handling zeroed IndexedSeqs.

Use reduce as the default construction path to take advantage of reducible collections.



 Comments   
Comment by Rohit Aggarwal [ 26/Sep/16 4:13 PM ]

Thomas Mulvaney, could you provide some benchmarks for the speed assertion? It would be nice to run it on Chrome/Firefox/Safari.

Comment by Thomas Mulvaney [ 28/Sep/16 1:30 AM ]

Sure thing, I'll do some more benchmarks.

Comment by Mike Fikes [ 19/Nov/17 7:45 PM ]

Patch no longer applies; needs re-baseline.





[CLJS-1783] Unify List creation code Created: 20/Sep/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1783.patch    
Patch: Code

 Description   

There is some duplication and redundant functions around List creation.

In this patch a fromArray method was added to List, consistent with other persistent data structures in the code base.



 Comments   
Comment by Mike Fikes [ 19/Nov/17 7:45 PM ]

Patch no longer applies; needs re-baseline.





[CLJS-1776] Add fixed arities for mapcat Created: 13/Sep/16  Updated: 13/Sep/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.76
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Robert C Faber Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: Text File CLJS_1776__Add_fixed_arities_for_mapcat.patch     Text File CLJS-1776.patch    
Patch: Code

 Description   

Following the pattern of map, this patch adds three fixed arities for mapcat.



 Comments   
Comment by Alex Miller [ 13/Sep/16 10:25 AM ]

Presumably this is to improve performance. Please include a benchmark showing the difference.





[CLJS-1766] Set literals in REPL end up reified as ArrayMap backed PersistentHashSets. Created: 28/Aug/16  Updated: 28/Aug/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl


 Description   

Entering a set literal in the REPL with more than 8 elements should create a PHM backed set but instead it is array backed.

Example (in REPL):
cljs.user=> (type (.-hash-map #{1 2 3 4 5 6 7 8 9}))
cljs.core/PersistentArrayMap

This means operations such as `get` and `contains?` end up doing long scans and are slower than a user would expect.






[CLJS-1755] Support sourcesContent in source maps Created: 16/Aug/16  Updated: 16/Aug/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: sourcemap


 Description   

This issue adds sourcesContent support for source maps: https://github.com/google/closure-compiler/issues/1890. This means that your source maps can include your source as well in one bundled file. This makes handling sourcemaps much easier for things like error tracking services. It could also simplify config for source mapping as everything is included in the source map and you don't need to specify relative paths, e.t.c.

This will need to wait for the next release of the Closure Compiler.






[CLJS-1734] :import in ns silently discards imported classes with the same name Created: 11/Aug/16  Updated: 08/May/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Thomas Heller Assignee: Robin Hermansson
Resolution: Unresolved Votes: 0
Labels: newbie


 Description   
(ns some.ns
  (:import goog.fx.Transition.EventType
           goog.history.EventType))

Both classes are named EventType and the second will effectively remove the first one without warning or error.



 Comments   
Comment by Robin Hermansson [ 07/May/17 8:03 AM ]

I would like to work on this issue.

Comment by David Nolen [ 08/May/17 7:14 PM ]

Robin, go for it





[CLJS-1726] demunge is too agreesive and incorrect in some cases Created: 04/Aug/16  Updated: 04/Aug/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I have implemented some "demunging" logic in cljs-devtools (and dirac) to present original user-friendly names in UI.

During my testing I spotted some wrong edge-cases and incorrect behaviours of demunge:

1) it is too aggressive in replacing dollars - some dollars can be "real" dollars as part of original name
2) it does not revert js-reserved? transformation applied during munging
3) it is oblivious to underscores/dashes - some underscores were "real" underscores before munging
(this may be not complete)

I have worked around those issues on my side and implemented some heuristics[1] based on context, but it is far from perfect.

I'm not sure how to properly fix those, so I wanted to open a ticket with discussion. Maybe people will have some clever ideas.

Currently munging is lossy and we probably don't want to touch it for compatibility reasons.
Maybe we could mark original underscores and dollars in some way, so demunge could properly skip them.

1) One strategy could be to use some (rare) unicode characters, but that would be problematic for people to type.
2) Another strategy could be to escape original dollars and underscores somehow (using more dollars and underscores .
3) Better ideas?

[1] https://github.com/binaryage/cljs-devtools/blob/52899e61e33373df36be8dcb23c69377936821b2/src/lib/devtools/munging.cljs#L154-L185






[CLJS-1722] Upgrade ExceptionInfo to proper deftype Created: 03/Aug/16  Updated: 18/Jun/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS-1722.patch    
Patch: Code

 Description   

Currently ExceptionInfo is implemented as a raw constructor function which inherits from js/Error with some ad-hoc javascript-level patches to satisfy a tiny subset of deftype functionality (mainly for printing).

Unfortunately this does not play well with cljs-devtools[1]. This problem surfaced when I started playing with ExceptionInfo and cljs-devtools v0.8 which newly supports printing deftypes[2]. ExceptionInfo does not contain getBasis, cljs$lang$type, cljs$lang$ctorStr and similar machinery.

My proposed patch implements ExceptionInfo as a proper deftype and does some patch-work to provide backward compatibility. I'm pretty sure we must not break current contract of ExceptionInfo constructor accepting 3 args and synthesizing other fields on the fly in constructor.

Implementation details:
1) first we define ExceptionInfo as normal deftype (to get a template)
2) then we remember reference to ExceptionInfo in ExceptionInfoTypeTemplate
3) then we redefine ExceptionInfo with raw constructor function which should mimic original behaviour (by scraping newly created js/Error instance, but calling ExceptionInfoTypeTemplate to do proper deftype initialization)
4) then we copy keys from ExceptionInfoTypeTemplate over ExceptionInfo
5) then we set ExceptionInfo's prototype to be ExceptionInfoTypeTemplate's prototype
6) then we fix ExceptionInfo's prototype's constructor to point to our re-defined constructor function
7) then we patch ExceptionInfo's prototype to inherit from js/Error (note this clobbers ExceptionInfoTypeTemplate as well - but we don't care about it)

This effectively gives us properly working ExceptionInfo deftype with redefined constructor function wrapping deftype's constructor for backwards compatibility.
We also patch ExceptionInfo's prototype to inherit from js/Error the same was as original code did.

Note: With working deftype, we can move IPrintWithWriter and toString implementation to the deftype itself.

[1] https://github.com/binaryage/cljs-devtools/issues/23
[2] https://github.com/binaryage/cljs-devtools/releases/tag/v0.8.0



 Comments   
Comment by Thomas Heller [ 04/Aug/16 4:25 AM ]

Why not just add the missing getBasis, cljs$lang$type, cljs$lang$ctorStr bits per set!?

The patch looks like it would mess up advanced compilation although that is just an instinct not something I verified, did you?

Comment by Antonin Hildebrand [ 04/Aug/16 4:44 AM ]

I ran clojurescript tests and I assumed they run also against advanced-mode build. During development when my tests were failing I saw error messages about minified names.

This may seem as a hacky solution, but IMO the original code was also a hack. My hack will stay up-to-date with future changes to deftype implementation. I can imagine people would forget to update this part when touching deftype.

btw. there is another patch coming related to discrepancies between deftype and defrecord. That could have been avoided if defrecord shared common implementation with deftype.

Comment by Thomas Heller [ 04/Aug/16 6:27 AM ]

Closure is usually very strict about re-defining stuff but I guess my instincts were wrong, the tests should cover advanced.

My issue with this is that deftype is for defining Clojure-specific types. ExceptionInfo is not since it inherits from Error, just like you can't have a superclass in Clojure you can't in CLJS. So IF we were to change deftype in the future we might break things in unexpected ways that just re-use deftype but aren't actually deftype.

Yes, you have to do some house-keeping but you can't enforce the rules of deftype when dealing with inheritance.

Just my 2 cents, it has advantages to re-use deftype too (as you suggested).

Comment by Antonin Hildebrand [ 04/Aug/16 6:36 AM ]

Unfortunately I was unable to look up any comments or docs explaining the reasoning why we do that js/Error inheritance there.

My first attempt to "fix" ExceptionInfo was to simply implement it as an ordinary deftype. And that worked just fine (for my tests). Then I tried to re-implement original behaviours on top just to make it 100% compatible.

Comment by Antonin Hildebrand [ 04/Aug/16 12:47 PM ]

Just adding a motivational screenshot:

https://box.binaryage.com/CLJS-1722-example.png

Those yellow warnings are listing which properties are getting copied by gobject/extend call.
The expanded log item is new implementation logged via cljs-devtools v0.8.0.
The last log item is the old implementation logged via cljs-devtools v0.8.0 (cljs-devtools does not recognise ExceptionInfo as CLJS type, but detects IPrintWithWriter and uses it to present the value)





[CLJS-1712] Make PersistentHashSet implement IReduce Created: 21/Jul/16  Updated: 21/Jul/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: performance

Attachments: Text File difference-benchmark.txt     Text File into-benchmark.txt     Text File phs-reduce.patch     Text File union-benchmark.txt    
Patch: Code

 Description   

This improves speed of many reduce based operations on set which were falling back to seq-reduce, including code in `clojure.set` namespace such as `clojure.set/union` and `(into [] some-set)`.

I've included a few benchmarks I performed using `simple-benchmark` in a JavascriptCore environment (Planck REPL)



 Comments   
Comment by Rohit Aggarwal [ 21/Jul/16 3:35 PM ]

I think the code currently is faithful to Clojure's implementation of PersistentHashSet. So any change from that would probably require more thought and/or history.

Also someone else also raised a similar issue on ClojureScript mailing list.





[CLJS-1677] Requiring [goog] breaks an :advanced build, but the compiler exits successfully Created: 09/Jun/16  Updated: 10/Jun/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.9.36
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None


 Description   

A single file with the following in it is enough to break a build:

(ns goog-error.core
  (:require [goog]))

with this error

Jun 10, 2016 11:18:03 AM com.google.javascript.jscomp.LoggerErrorManager println
SEVERE: ERROR - Duplicate input: file:/Users/danielcompton/.m2/repository/org/clojure/google-closure-library/0.0-20151016-61277aea/google-closure-library-0.0-20151016-61277aea.jar!/goog/base.js

Jun 10, 2016 11:18:03 AM com.google.javascript.jscomp.LoggerErrorManager printSummary
WARNING: 1 error(s), 0 warning(s)
ERROR: JSC_DUPLICATE_INPUT. Duplicate input: file:/Users/danielcompton/.m2/repository/org/clojure/google-closure-library/0.0-20151016-61277aea/google-closure-library-0.0-20151016-61277aea.jar!/goog/base.js at (unknown source) line (unknown line) : (unknown column)

however the ClojureScript compiler exits successfully without throwing an error. The build looks successful, but the file produced doesn't work. Should the ClojureScript compiler throw on these kinds of errors, or otherwise indicate failure?



 Comments   
Comment by David Nolen [ 10/Jun/16 8:27 AM ]

We should look into why the namespace validation that checks where a ns exists or not isn't already catching this case.





[CLJS-1644] Recursive protocol method for a record never terminates Created: 23/May/16  Updated: 08/Jul/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.228
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Brian Stiles Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Any



 Description   

When extending a protocol for a record, a recursive method can never terminate in some cases.

(defrecord R [a])

(defprotocol P
  (f [x]))

(extend-protocol P
  R
  (f [x]
    (if x
      (recur nil)
      x))
  default
  (f [x]
    (if x
      (recur nil)
      x)))

(prn (f 1))        ; #1
(prn (f (R. 1)))   ; #2

prn call #1 prints nil as expected, but prn call #2 never terminates.

It looks like the compiler creates a variable assigned to this within the while loop such that the test of "x" is always really testing this when it should be testing the value of x passed in by the call to recur.

Note, I'm testing ClojureScript 1.8.51. The "Affects Version/s" field above only gives 1.7.228 as the most recent version.



 Comments   
Comment by Brian Stiles [ 23/May/16 4:14 AM ]

Actually, "always really testing 'this' when it should be testing the value of x passed in by the call to recur" is only true if the type of the value represented by x remains the same. If, as in the example, the type changes, the call should be dispatched to the properly matching method implementation.

This seems to behave as expected in Clojure.

Comment by David Nolen [ 08/Jul/17 10:58 AM ]

Confirmed with Mike Fikes this is still an issue on master even with the recur enhancements.





[CLJS-1631] The str function should handle JavaScript symbols Created: 21/Apr/16  Updated: 20/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJS-1631-Make-str-handle-js-symbols.patch    
Patch: Code and Test

 Description   

The str function should handle primitive JavaScript symbols produced by Symbol.for. At the moment the str function raises an exception, because it runs into some JavaScript safety checks by using implicit string coersion via (.join #js [x] "").
More info on the safety check here: http://www.2ality.com/2014/12/es6-symbols.html
This ticket is also related to:

(def x (.for js/Symbol "x"))
(str x)
TypeError: Cannot convert a Symbol value to a string
    at Array.join (native)
    at Function.cljs.core.str.cljs$core$IFn$_invoke$arity$1 (/home/roman/workspace/clojurescript/.cljs_node_repl/cljs/core.js:9561:12)
    at Object.cljs$core$str [as str] (/home/roman/workspace/clojurescript/.cljs_node_repl/cljs/core.js:9543:22)
    at repl:1:100
    at repl:9:3
    at repl:14:4
    at Object.exports.runInThisContext (vm.js:54:17)
    at Domain.<anonymous> ([stdin]:41:34)
    at Domain.run (domain.js:228:14)
    at Socket.<anonymous> ([stdin]:40:25)

Calling the toString method on a symbol directly works

(.toString x)
;;=> "Symbol(x)"


 Comments   
Comment by Mike Fikes [ 19/Nov/17 9:09 PM ]

Unit tests fail in (presumably older environments) in Travis:

JavaScriptCore:

ERROR in (test-cljs-1631) (core-advanced-test.js:3724:60)
expected: (= "Symbol(x)" (str (.for js/Symbol "x")))
  actual: #object[ReferenceError ReferenceError: Can't find variable: Symbol]

Nashorn:

ERROR in (test-cljs-1631) (ReferenceError:NaN:NaN)
expected: (= "Symbol(x)" (str (.for js/Symbol "x")))
  actual: #object[ReferenceError ReferenceError: "Symbol" is not defined]

Since this appears to only affect the tests, and not the production code, perhaps the tests could be conditional on (exists? js/Symbol).

Comment by Thomas Heller [ 20/Nov/17 6:43 AM ]

There is a lot of history behind the str fn/macro and why they do what they to.

See: https://dev.clojure.org/jira/browse/CLJS-890

The patch just uses toString which is not an option until we sort out CLJS-890.





[CLJS-1628] Make instances of js/Symbol printable Created: 20/Apr/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Roman Scherer Assignee: Roman Scherer
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File CLJS-1628.patch    
Patch: Code and Test

 Description   

At the moment instances of js/Symbol are not printable. Trying to
print them or enter them in the REPL causes an exception.

cljs.user> (.for js/Symbol "react.element")
#object[TypeError TypeError: Cannot convert a Symbol value to a string]

Symbols are supported in all major browsers, except Internet Explorer and Nashorn.
https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol



 Comments   
Comment by Roman Scherer [ 20/Apr/16 10:23 AM ]

The attached patch adds an additional clause to `pr-writer-impl` and
implements the printing of Symbol instances, in the same way as it is
done for other JavaScript objects. Here's an example of a printed
js/Symbol:

(.for js/Symbol "react.element")
;;=> #object[Symbol "react.element"]

@david: Regarding your point about shims, do you think the
implementation of `js-symbol-defined?`, which was used for the ES6
iterator support, is enough for this patch? I'm not too familiar with
JavaScript and not sure if this already addressed the "shim" issue.

Another thing I stumbled upon is, that my test currently generates a
compiler warning when using the default compiler options. The warning
is generated when compiling the following ClojureScript form:

(.for js/Symbol "react.element")

The following snippet shows the warning and the generated code from my
test:

WARNING - Keywords and reserved words are not allowed as unquoted
property names in older versions of JavaScript. If you are targeting
newer versions of JavaScript, set the appropriate language_in option.

try{var values__13328__auto__ = (function (){var x__6628__auto__ = cljs.core.pr_str.cljs$core$IFn$_invoke$arity$variadic(cljs.core.array_seq([Symbol.for("react.element")], 0));

I think this has nothing to do with this patch, but with the emitted
code not being legal Ecmascript 3, since "for" is a reserved word.

The warning goes away when changing the :language-in option to
something newer than Ecmascript 3, or doing something like this:

((gobj/get js/Symbol "for") "react.element")

So, the questions is: Should the ClojureScript compiler handle those
reserved words when they appear in a function call or property lookup?

If that's the case I would leave the warning in that patch, and open
another issue for this new problem.

What do you think?

Roman

Comment by Roman Scherer [ 20/Apr/16 11:33 AM ]

Ok, given advanced mode and renaming of symbols I think there's not much the ClojureScript compiler can do about this.
I think I'll change the test to use ((gobj/get js/Symbol "for") "react.element") to remove the warning.
https://github.com/google/closure-compiler/wiki/FAQ#i-get-invalid-property-id-errors-but-it-works-on-firefox

Comment by David Nolen [ 20/Apr/16 2:21 PM ]

ECMAScript 3 is not an issue for people who are ok with outputting ECMAScript 5. The goog.typeOf of check on "symbol" does not seem like it would work with Symbol shims, have you confirmed?

Comment by Roman Scherer [ 21/Apr/16 10:57 AM ]

David, yes you are right about goog.typeOf, it returns "object" when
js/Symbol is implemented via shims. I updated the patch and tested it
against the following shim implementation in Nashorn so far.

Start a Nashorn REPL.

./script/nashornrepljs

js/Symbol does not exist in Nashorn yet.

(exists? js/Symbol)
;;=> false

Load core-js shim into Nashorn.

(js/load "https://raw.githubusercontent.com/zloirock/core-js/master/client/shim.min.js")
(print (.for js/Symbol "x"))
;;=> #object[Symbol "x"]

I gave up on this shim implementation because I got an error when
trying to load the minified js into Nashorn.

https://github.com/medikoo/es6-symbol

This is another shim implementation, but it only enhances js/Symbol
when it already exists, so not useful in the context of Nashorn.

https://github.com/paulmillr/es6-shim

Comment by Roman Scherer [ 21/Apr/16 2:07 PM ]

Which implementations do support js/Symbol?

JavaScriptCore, NodeJS, SpiderMonkey, Chrome and Firefox support
js/Symbol. Evaluating (instance? js/Symbol (.for js/Symbol "x")) on
those engines returns false. On all those implementations symbols
generated via (.for js/Symbol "x") are primitive values and can be
identified via typeof. See the "Using the typeof operator with
symbols" section in [1].

Nashorn and Rhino do not support js/Symbol.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Comment by Roman Scherer [ 21/Apr/16 2:27 PM ]

Looking around I found that the exception caused by printing symbols
happens because the str [1] function uses implicit string conversion
to convert it's argument into a string. This is explained in the
"Coercing a symbol to string" section of [2]. One way to solve the
problem is to use (.toString x) instead.

(.join #js [(.for js/Symbol "x")] "")
;;=> TypeError: Symbol is not a constructor

(.toString (.for js/Symbol "x"))
;;=> "Symbol(x)"

[2] https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/core.cljs#L2695
[2] http://www.2ality.com/2014/12/es6-symbols.html

Comment by Roman Scherer [ 21/Apr/16 2:38 PM ]

(= (goog/typeOf x) "symbol") vs (instance? js/Symbol x)

Symbols are primitive types and must be constructed via
Symbol("x"). They can't be constructed via new Symbol("x"), trying
to do so raises a TypeError. This is explained in more detail in the
"Safety checks" section of [1]. The "Using the typeof operator with
symbols" section of [2] explains how to identify symbols. It has to be
done via typeof, because symbols are primitive types and not
instances of Symbol.

[1] http://www.2ality.com/2014/12/es6-symbols.html
[2] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Comment by Roman Scherer [ 21/Apr/16 3:02 PM ]

I think we have 2 options now to fix exceptions when printing symbols:

  • Change str to handle symbols as well.
  • Implement custom printing for symbols.

I think I would go for the first option. I think calling str on a
symbol can't be seen as implicitly coercing a symbol to a string, I
would say this is as explicit as it can get and we don't have to raise
an exception in this case. I think str is basically the Clojure
equivalent to toString in JavaScript, at least this is what the out
of date doc string of str says.

The file CLJS-1628-fix-str has an implementation of the first option.

Comment by Francis Avila [ 21/Apr/16 3:35 PM ]

Changing str to use .toString is not possible because of CLJS-847: there are unconfirmable reports that .toString() causes TypeError on Safari 6.0.5.

This problem has spilled over into the unresolved CLJS-890. This is a long thread, so see my recap comment, my failed efforts to reproduce the bug and my proposed final patch, which can be easily extended for the symbol case with a new "symbol" clause in the case statement.

Comment by Roman Scherer [ 21/Apr/16 4:20 PM ]

Thanks Francis, I'll take a look at this discussion.

Comment by Roman Scherer [ 21/Apr/16 5:12 PM ]

Since one can still make so called Symbol wrapper objects via the
Object() function [1], I added one more test case for this:

(js/Object (.for js/Symbol "x"))

Also the updated patch defines the test case only if js/Symbol exists?
in the JavaScript engine.

[1] https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Symbol

Comment by Mike Fikes [ 19/Nov/17 7:54 PM ]

Patch no longer applies.





[CLJS-1627] jsdoc parsing fails to recognize union types, breaking resolution Created: 18/Apr/16  Updated: 19/Nov/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.228
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Patrick Killean Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJS-1627-4.patch     Text File CLJS-1627-5.patch    
Patch: Code and Test

 Description   

The Closure Spec For Union Types states that parentheses are necessary for union type expressions. Trying this ...

(defn foo
  "@param {(IBar|IMap)} x"
  [x] 
  ...)

Raises a Closure Error :

...ERROR - Bad type annotation. expected closing }
* @param {user.(IBar|user.IMap)}

This is because comp/resolve-types treats the parentheses as a part of the type tokens and incorrect var resolution occurs as a result. In addition, the compiler emits multiple resolved types separated by "|" characters but does not enclose them in parentheses to create a valid union type.



 Comments   
Comment by Patrick Killean [ 18/Apr/16 4:36 PM ]

This patch includes:

  • comp/resolve-types now removes parentheses when present and emits them when >1 type is detected. This makes parenthesis use optional and existing code remains unbroken (with the added benefit that it may work now)
  • changes to comp/resolve-type
    1. checks for js globals like document or window which are recognized by closure
    2. allows dot.delimited.forms to pass through so we can use types defined in externs and avoid unnecessary resolution
    3. uses ana/resolve-existing-var with a "unresolved jsdoc type" warning
    4. checks if a resolved var is a protocol and warns otherwise. This is more informative than Closure's standard unrecognized type error
  • a test for comp/resolve-types
Comment by David Nolen [ 21/Apr/16 12:45 PM ]

Thanks will try to look more closely at this tomorrow.

Comment by David Nolen [ 23/Apr/16 2:03 PM ]

The patch is getting there, please remove the `js-doc-type` meta stuff. Just extend the signature of resolve-existing-var to take an additional parameter - the confirm-var-exists handler.

Comment by Patrick Killean [ 09/May/16 4:58 PM ]

CLJS-1627-1.patch:
resolve-existing-var now has an additional arity that accepts a missing-var handler passed to confirm-existing-var

Comment by Patrick Killean [ 10/May/16 6:16 AM ]

This has revealed a problem where deftype + defrecord using Object protocols emit resolved names when really they shouldn't. For example : "@implements {cljs.core.async.impl.timers.Object}" --> Bad Type Annotation

Since Object is a special case simply excluding it from the comments should fix it. Another patch incoming

Comment by Patrick Killean [ 10/May/16 7:42 AM ]

CLJS-1627-2.patch:
The emit* methods for deftype and defrecord now filter out Object protocols.

This produced an interesting result! With no more bad type annotations, static analysis can now proceed... and it has alot to say. Theres all kinds of info now about arity discrepencies (particularly cljs.core.IndexedSeq), type mismatches, and more. It even includes a type coverage percentage. Lots to parse here but very cool.

Comment by Patrick Killean [ 18/May/16 4:26 PM ]

CLJS-1627-3.patch:

  • fix require extern
  • add type application support for Array & Object
  • GC likes uppercase for Object & Array, lowercase for string, number.
  • support for explicit nullable types, variable typed arg
  • function type context modifiers this + new

Missing is the GC 'record type' . It also may be useful to fill out the node externs for common types

Comment by Patrick Killean [ 20/May/16 11:42 AM ]

CLJS-1627-4.patch:

  • fix a few problems in last patch
  • add record type support. Everything here should be covered
Comment by Patrick Killean [ 02/Sep/16 8:21 AM ]

update patch

Comment by Mike Fikes [ 19/Nov/17 7:55 PM ]

CLJS-1627-5.patch no longer applies





[CLJS-1610] Refs api Created: 26/Mar/16  Updated: 28/Mar/16

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It would be nice to support the refs API in clojurescript to ease porting of libraries which require it. Obviously usage of it would not bring concurrency benefits, but it would at least allow some existing clojure code to run.

I've satisfied myself it can be done reasonably easily (see https://github.com/jjl/clojurescript/tree/ref-support ), and I can't really see any downsides.

For the minute, this has been put into the cljs.stm namespace. I don't know whether it should be a feature you are automatically opted into, so I didn't put it into core. Thoughts?