<< Back to previous view

[CLJS-2386] random-uuid should use a cryptographically strong PRNG if available Created: 19/Oct/17  Updated: 20/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 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 to use are:

  • window.crypto.getRandomValues in most browsers
  • window.msCrypto.getRandomValues in IE11
  • crypto.randomBytes on Node.js
  • Math/random in browsers not supporting the former or if the crypto module is not made available 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. Maybe a docstring hint for how to make the crypto module available on Node.js would be sufficient.



 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.





[CLJS-2387] CLJS Analyzer does not correctly detect cache hits for analyzed spec files Created: 19/Oct/17  Updated: 19/Oct/17

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

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

OS X



 Description   

When Analyzing a cljs file which contains only (cljs.spec.alpha/def ...) forms and no clojure core (def ...) special forms, the Analyzer does not determine the source file to have been analyzed on https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/analyzer.cljc#L4021. When this is combined with the :cache-analysis true compiler option, it results in the analysis cache continually being repopulated from disk, and in certain degenerate cases, poor compilation performance.



 Comments   
Comment by Alexander Redington [ 19/Oct/17 1:30 PM ]

Steps to reproduce: have multiple cljs files require one namespace that contains only (s/def ...) forms, such that analysis is examined multiple times for the spec namespace. Run with compiler-options :verbose true and :cache-analysis true, and observe the file is analyzed multiple times and the cache is re-read multiple times.

Workarounds: Add a meaningless (def ...) form to the files which are repeatedly analyzed, and they will suddenly stop being analyzed more than once.

On my local machine doing this with the worst offendeding namespaces dropped compile times from 104 seconds to 34 seconds.





[CLJS-2383] Improve perf of select-keys by using keyword-identical? Created: 17/Oct/17  Updated: 18/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 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!`





[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: 0
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-2384] Old Infinity and NaN literals are still used in tests Created: 17/Oct/17  Updated: 17/Oct/17

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

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

Attachments: Text File CLJS-2384.patch    
Patch: Code and Test

 Description   

You can see when running tests:

WARNING: Use of undeclared Var cljs.core-test/Infinity at line 1178 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/-Infinity at line 1179 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/NaN at line 1180 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/Infinity at line 1181 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/-Infinity at line 1181 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/Infinity at line 1182 src/test/cljs/cljs/core_test.cljs
WARNING: Use of undeclared Var cljs.core-test/-Infinity at line 1182 src/test/cljs/cljs/core_test.cljs





[CLJS-2378] Keep the value of ':npm-deps-installed?' in the repeated builds such as "lein cljsbuild auto". Created: 04/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: Jinseop Kim Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Node.js


Attachments: Text File CLJS-2378.patch    

 Description   

While using 'lein-cljsbuild',
I found a bug that toggle the ':npm-deps-installed?' whenever compiling.

In the 'build' function, src/main/clojure/cljs/closure.clj:2524:
(swap! compiler-env update-in [:npm-deps-installed?]
(fn [installed?]
(when-not installed?
(maybe-install-node-deps! opts))))

In above, the lambda function will return nil if the ':npm-deps-installed?' is true.
I think that should keep the ':npm-deps-installed?' if it is true.

(swap! compiler-env update-in [:npm-deps-installed?]
(fn [installed?]
(if-not installed? ;; <= !!!
(maybe-install-node-deps! opts)
true))) ;; <= !!!



 Comments   
Comment by Jinseop Kim [ 04/Oct/17 9:36 AM ]

I add a patch.

Comment by António Nuno Monteiro [ 11/Oct/17 11:53 AM ]

The attached patch looks good to me.

Comment by David Nolen [ 13/Oct/17 2:38 PM ]

Jinseop have you submitted a Clojure contributor agreement? Thanks.

Comment by Jinseop Kim [ 13/Oct/17 7:05 PM ]

Yes.





[CLJS-1444] Node.js shim requires `node` invocation to be in the same directory as shim script Created: 10/Sep/15  Updated: 13/Oct/17

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

Type: Defect Priority: Minor
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-In-bootstrapping-code-use-__dirname-to-calculate-pat.patch    

 Description   

Don't have time to provide a proper repro now but the basic issue can be illustrated by this:

~/c/boot-cljs-example (master=) node target/main.js
module.js:338
    throw err;
          ^
Error: Cannot find module '/Users/martin/code/boot-cljs-example/out/goog/bootstrap/nodejs.js'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/Users/martin/code/boot-cljs-example/target/main.js:6:1)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:501:10)
~/c/boot-cljs-example (master=) cd target/
~/c/b/target (master=) node main.js
Starting...

This is compiled with boot because that was what I had at hand right now. The compiled shim looks like this:

var path = require("path");
try {
    require("source-map-support").install();
} catch(err) {
}
require(path.join(path.resolve("."),"out","goog","bootstrap","nodejs.js"));
require(path.join(path.resolve("."),"out","cljs_deps.js"));
goog.global.CLOSURE_UNCOMPILED_DEFINES = {"cljs.core._STAR_target_STAR_":"nodejs"};
goog.require("boot.cljs.main");
goog.require("cljs.nodejscli");

The problem here is that path.resolve(".") will return the directory the node command was invoked in and not the directory of the shim. (See the "Cannot find module..." error above)

A solution could be to use __dirname which always resolves to the directory of the current file. This might result in some breakage for existing setups.



 Comments   
Comment by Sebastian Bensusan [ 14/Oct/15 11:31 AM ]

I have a proposed solution but I fear that supporting "run from anywhere" adds essential complexity to the resulting code. My thought process:

1. Relative paths are desirable to produce "context free code." If the user chooses absolute paths, then that behavior is respected and there's nothing to handle (no "path algebra") .

2. When dealing with relative paths, the whole system needs to establish a "frame of reference", a root path. The ClojureScript compiler assumes the path from which it is compiling to be that frame of reference, which usually coincides with the top root of the project. Though arbitrary, it is the only choice that makes sense.

3. The frame of reference is not explicit anywhere in the code, since it is defined as ".". If it were explicit, it would reveal context, as in "/home/some-user/their-folder/this-project/".

4. When we approach the code from another reference point (executing the script from another directory), we first need to find the original compiler path (reference point,) and then resolve all paths from there. The compiler uses `cljs.closure/path-relative-to` for this purpose.

Path algebra:
compiler-path = __dirname - output-to

Node.js

var compiler-path = __dirname.replace(output-to, "")
path.resolve (compiler-path, output-dir, "goog", "base.js")
path.resolve (compiler-path, output-dir, "cljs_deps.js")

which assumes that if output-to was given relatively, then output-dir is also relative. If they are not in sync, more work needs to be done to keep them that way.

It's not up to me to decide if the extra complexity is worth the use-case. I actually hope there is a simpler solution to solve this that I'm not seeing.

Comment by Karol Majta [ 14/Mar/16 10:43 AM ]

I find this behavior really weird and would opt for switching to __dirname. I am also not sure i fully understand consequences of such switch (I have little cljs experience, speaking more from the perspective of nodejs user). My point is: current behavior renders clojurescript hard to use for commandline and desktop applications (namely electron).

For command line and desktop applications assumptions about CWD cannot be made. For now i run my applications through a bootstrap script JS script:

process.chdir(__dirname);
require('./target/out');

I am lucky that my code does not have to use the real CWD, but it's a hack more than a real solution.

Speaking from nodejs perspective:

1. Using absolute paths is considered a bad practice anyway.
2. Nodejs programs that don't use external packages (don't depend on node_modules) can be run from any CWD
3. Nodejs programs that do depend on node_modules will break if run from a different directory than the one containing node_modules, but this is expected behavior.

Comment by J. Pablo Fernández [ 26/Jan/17 5:42 AM ]

I just run into this problem when trying to develop an Electron application. The way it's working right now is essentially unpackageable. I think it would be nice to have this behavior even as an option and I'm happy to work on a patch.

Comment by J. Pablo Fernández [ 26/Jan/17 5:57 AM ]

As far as I can see, this is the relevant code:

https://github.com/clojure/clojurescript/blob/cdaeff298e0f1d410aa5a7b6860232270d287084/src/main/clojure/cljs/closure.clj#L1410-L1411

Comment by J. Pablo Fernández [ 26/Jan/17 6:15 AM ]

A potential workaround seems to use :simple optimization.

Comment by Matt Lee [ 21/Apr/17 12:02 PM ]

I have also just hit this issue in an electron app that I'm building. @pupeno did you get anywhere with your offer to work on a patch? I too would be happy to look in to a patch for this. Although I think I'll need some pointers to get started.

Comment by Matt Lee [ 22/Apr/17 5:36 AM ]

From a quick experiment this morning it looks like replacing path.resolve(".") with __dirname at https://github.com/clojure/clojurescript/blob/aa5f001300e9aebd976cb180f5b7ccb37fcb6898/src/main/clojure/cljs/closure.clj#L1460-L1461 works for a couple of simple electron and node apps. By work I specifically mean that the error doesn't occur even when the app is started from a current working directory that is different to the compilation directory, i.e. the code is path independent.

I'll attach a patch for this once I've done some more complete testing. Before then any feedback on this approach would be appreciated.

Comment by Matt Lee [ 23/Apr/17 1:51 AM ]

Attaching patch for the suggested fix of using __dirname instead of "." in the generated script. I have tested this with nodejs 6.10.0 and electron 1.6.5.

Comment by Matt Lee [ 24/Apr/17 2:34 AM ]

Also see: CLJS-1990

Comment by Matthias Nehlsen [ 13/Oct/17 5:19 PM ]

Having the same problem where I can't use optimization :none when packaging with electron-builder. Any chance to accept the suggested patch? Thanks!





[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-2377] The CLJS compiled uses deprecated modules on Java 9 Created: 03/Oct/17  Updated: 06/Oct/17

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

Type: Defect Priority: Major
Reporter: Mike Kaplinskiy Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

OSX Java 9


Attachments: Text File 0001-CLJS-2377-Remove-usage-of-java.xml.bind.DatatypeConv.patch    
Patch: Code

 Description   

Using a simple Java9 REPL to require cljs.build.api will currently result in:

Exception in thread "main" java.lang.ClassNotFoundException: javax.xml.bind.DatatypeConverter, compiling:(util.clj:9:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3657)
	at clojure.lang.Compiler.compile1(Compiler.java:7474)
	at clojure.lang.Compiler.compile1(Compiler.java:7464)
	at clojure.lang.Compiler.compile(Compiler.java:7541)
	at clojure.lang.RT.compile(RT.java:406)
	at clojure.lang.RT.load(RT.java:451)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$load_one.invoke(core.clj:5692)
	at clojure.core$load_lib$fn__5626.invoke(core.clj:5737)
	at clojure.core$load_lib.invokeStatic(core.clj:5736)
	at clojure.core$load_lib.doInvoke(core.clj:5717)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$load_libs.invokeStatic(core.clj:5774)
	at clojure.core$load_libs.doInvoke(core.clj:5758)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invokeStatic(core.clj:648)
	at clojure.core$require.invokeStatic(core.clj:5796)
	at cljs.analyzer$loading__5569__auto____44.invoke(analyzer.cljc:9)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3652)
	at clojure.lang.Compiler.compile1(Compiler.java:7474)
	at clojure.lang.Compiler.compile1(Compiler.java:7464)
	at clojure.lang.Compiler.compile(Compiler.java:7541)
	at clojure.lang.RT.compile(RT.java:406)
	at clojure.lang.RT.load(RT.java:451)
	at clojure.lang.RT.load(RT.java:419)
	at clojure.core$load$fn__5677.invoke(core.clj:5893)
	at clojure.core$load.invokeStatic(core.clj:5892)
	at clojure.core$load.doInvoke(core.clj:5876)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invokeStatic(core.clj:5697)
	at clojure.core$compile$fn__5682.invoke(core.clj:5903)
	at clojure.core$compile.invokeStatic(core.clj:5903)
	at clojure.core$compile.invoke(core.clj:5895)
	at user$eval26$fn__33.invoke(compile_clj.clj:48)
	at user$eval26.invokeStatic(compile_clj.clj:45)
	at user$eval26.invoke(compile_clj.clj:36)
	at clojure.lang.Compiler.eval(Compiler.java:6927)
	at clojure.lang.Compiler.load(Compiler.java:7379)
	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:436)
	at clojure.lang.Var.invoke(Var.java:388)
	at clojure.lang.AFn.applyToHelper(AFn.java:160)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.ClassNotFoundException: javax.xml.bind.DatatypeConverter
	at java.base/java.net.URLClassLoader.findClass(URLClassLoader.java:466)
	at clojure.lang.DynamicClassLoader.findClass(DynamicClassLoader.java:69)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:563)
	at clojure.lang.DynamicClassLoader.loadClass(DynamicClassLoader.java:77)
	at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:496)
	at java.base/java.lang.Class.forName0(Native Method)
	at java.base/java.lang.Class.forName(Class.java:375)
	at clojure.lang.RT.classForName(RT.java:2168)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2181)
	at cljs.util$loading__5569__auto____46.invoke(util.clj:9)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3652)
	... 56 more

The javax.xml.bind package was deprecated in Java 9 and moved to a module, which requires adding --add-modules java.xml.bind to your REPL command line. It would be great if clojurescript didn't depend on the deprecated javax.xml.bind package in the first place.



 Comments   
Comment by Thomas Heller [ 04/Oct/17 2:36 AM ]

Since Closure now depends on JDK 8 anyways it should probably use java.util.Base64 instead.





[CLJS-2379] Print negative zero as -0.0 Created: 06/Oct/17  Updated: 06/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: enhancement, print

Attachments: Text File CLJS-2379.patch    
Patch: Code and Test

 Description   

In Clojure, negative zero is printed as -0.0 (for example when evaluating -0.0).

This ticket asks for the same for ClojureScript.

Note: The question may arise: Why not print as -0? The ClojureScript reader behaves similarly to Clojure's in that -0 is read in as an integer value, which doesn't have signed zeros, and is thus read in as 0. To properly round trip, this ticket asks that negative zero be printed as -0.0 (presuming that there are to be no changes to the reader behavior.)



 Comments   
Comment by Mike Fikes [ 06/Oct/17 8:24 AM ]

Oh hey Erik, I had assigned this ticket to myself and have attached a patch. I see you attached an alternative. But, I think your alternative prints 0 as -0. Is that correct?

Comment by Erik Assum [ 06/Oct/17 8:27 AM ]

Yeah, I didn't see you assigned to yourself, so I made a patch solving the description as it was (when it specified -0.

Comment by Mike Fikes [ 06/Oct/17 9:05 AM ]

The attached patch doesn't attempt to make (str -0.0) match Clojure (presuming that changing str could pose a perf challenge). But, it does give us round tripping for -0.0.

Here are perf benchmarks:

Speedup summary:

                 true     10  ##NaN  ##Inf ##-Inf (js-obj)  -0.0      0
            V8:  1.02   0.97   1.00   1.02   1.04    1.01   1.00   1.00
  SpiderMonkey:  0.98   1.04   1.00   1.01   0.98    1.02   1.05   1.03
JavaScriptCore:  1.00   1.01   1.02   1.01   1.01    1.04   1.04   1.00
       Nashorn:  0.98   1.13   0.96   0.95   0.99    0.92   0.99   0.98 
    ChakraCore:  1.02   1.03   1.03   1.02   1.01    0.99   1.07   1.07

Before:

Before:

Benchmarking with V8
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 386 msecs
[x 10], (pr-str x), 1000000 runs, 561 msecs
[x js/NaN], (pr-str x), 1000000 runs, 641 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 634 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 593 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 898 msecs
[x -0.0], (pr-str x), 1000000 runs, 449 msecs
[x 0], (pr-str x), 1000000 runs, 530 msecs

Benchmarking with SpiderMonkey
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1321 msecs
[x 10], (pr-str x), 1000000 runs, 1330 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1293 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1307 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1297 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2143 msecs
[x -0.0], (pr-str x), 1000000 runs, 1340 msecs
[x 0], (pr-str x), 1000000 runs, 1312 msecs

Benchmarking with JavaScriptCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 502 msecs
[x 10], (pr-str x), 1000000 runs, 510 msecs
[x js/NaN], (pr-str x), 1000000 runs, 652 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 698 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 714 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 1266 msecs
[x -0.0], (pr-str x), 1000000 runs, 788 msecs
[x 0], (pr-str x), 1000000 runs, 562 msecs

Benchmarking with Nashorn
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1746 msecs
[x 10], (pr-str x), 1000000 runs, 1498 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1256 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1251 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1302 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2905 msecs
[x -0.0], (pr-str x), 1000000 runs, 1342 msecs
[x 0], (pr-str x), 1000000 runs, 1383 msecs

Benchmarking with ChakraCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1254 msecs
[x 10], (pr-str x), 1000000 runs, 1190 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1118 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1119 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1124 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2420 msecs
[x -0.0], (pr-str x), 1000000 runs, 1183 msecs
[x 0], (pr-str x), 1000000 runs, 1192 msecs

After:

After:

Benchmarking with V8
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 377 msecs
[x 10], (pr-str x), 1000000 runs, 577 msecs
[x js/NaN], (pr-str x), 1000000 runs, 642 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 623 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 570 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 887 msecs
[x -0.0], (pr-str x), 1000000 runs, 448 msecs
[x 0], (pr-str x), 1000000 runs, 530 msecs

Benchmarking with SpiderMonkey
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1354 msecs
[x 10], (pr-str x), 1000000 runs, 1282 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1320 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1298 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1322 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2093 msecs
[x -0.0], (pr-str x), 1000000 runs, 1280 msecs
[x 0], (pr-str x), 1000000 runs, 1277 msecs

Benchmarking with JavaScriptCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 504 msecs
[x 10], (pr-str x), 1000000 runs, 507 msecs
[x js/NaN], (pr-str x), 1000000 runs, 639 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 690 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 708 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 1216 msecs
[x -0.0], (pr-str x), 1000000 runs, 759 msecs
[x 0], (pr-str x), 1000000 runs, 563 msecs

Benchmarking with Nashorn
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1778 msecs
[x 10], (pr-str x), 1000000 runs, 1323 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1303 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1311 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1309 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 3168 msecs
[x -0.0], (pr-str x), 1000000 runs, 1360 msecs
[x 0], (pr-str x), 1000000 runs, 1405 msecs

Benchmarking with ChakraCore
;; printing of numbers and handling of ##Nan, ##Inf, ##-Inf, -0.0
[x true], (pr-str x), 1000000 runs, 1233 msecs
[x 10], (pr-str x), 1000000 runs, 1150 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1084 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1094 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1117 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2445 msecs
[x -0.0], (pr-str x), 1000000 runs, 1106 msecs
[x 0], (pr-str x), 1000000 runs, 1119 msecs




[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: 4
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-1793] Few misplaced docstrings Created: 24/Sep/16  Updated: 05/Oct/17

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

Type: Defect Priority: Trivial
Reporter: Thomas Mulvaney Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

A few docsctrings were placed after the binding form in the src/main/clojure/cljs/closure.clj namespace.






[CLJS-1676] Unused local in ObjMap / IKVReduce Created: 09/Jun/16  Updated: 05/Oct/17

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

Type: Defect Priority: Trivial
Reporter: Stuart Hinson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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

 Description   

Local len isn't used in ObjMap / IKVReduce

https://github.com/clojure/clojurescript/blob/463de005b81d4a00951188e8b8d38a61d684c18e/src/main/cljs/cljs/core.cljs#L5792



 Comments   
Comment by Stuart Hinson [ 10/Aug/17 2:56 PM ]

BTW, recognize this is a trivial issue, but it's still around

https://github.com/clojure/clojurescript/blob/fe77a1349aba903856cbaa0a0f10e03b6fb30d92/src/main/cljs/cljs/core.cljs#L6150





[CLJS-1631] The str function should handle JavaScript symbols Created: 21/Apr/16  Updated: 05/Oct/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)"





[CLJS-1601] Optimize cljs.core dump Created: 12/Mar/16  Updated: 05/Oct/17

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

Type: Enhancement Priority: Minor
Reporter: Nikita Beloglazov Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: bootstrap

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

 Description   

When building cljs source that uses cljs.js namespace the final js file is quite huge: 6.4M. As described in wiki: https://github.com/clojure/clojurescript/wiki/Optional-Self-hosting it mostly consists of analysis cache of the cljs.core namespace. As a workaround, the wiki article suggests dumping cache to a separate file and load it at runtime instead of bundling in js binary. I think it is possible to have something in between that doesn't require additional efforts from a user and also optimizes the size of the js file. The idea that instead of dumping cache as raw clojure data-structure it is serialized to string. This way compiler won't compile cache into js (which adds a lot of code) and leave it a string. At runtime, this string will be parsed back to clojure using tools.reader.

Here is the proposal: https://gist.github.com/nbeloglazov/0bf163fb62fa4b61d446

Checking locally it reduces the size of js file from 6.4M to 2.7M which I think quite good. The downside is that now js has to do more work on runtime (parse huge string) when today it simply read js code and evaluates it. But I don't think if it's a big concern. If it is desired to keep all behavior a new option can be added for :dump-core compiler setting, something like :dump-core :string that enables string serialization of the cache.

Does it sound reasonable?



 Comments   
Comment by Nikita Beloglazov [ 27/Mar/16 8:54 PM ]

Attaching suggested fix. Analysis cache is serialized to string and read back to clojure datastructure when cljs.js is initialized.

Comment by David Nolen [ 28/Mar/16 6:39 AM ]

Please change the patch so this optional as you've suggested.

Comment by David Nolen [ 28/Mar/16 6:40 AM ]

Also have you submitted your Clojure CA yet?

Comment by Nikita Beloglazov [ 28/Mar/16 1:35 PM ]

Will do. Yes, I've submitted CA. I used my official name, Mikita Belahlazau there.

Comment by Nikita Beloglazov [ 29/Mar/16 12:16 AM ]

Updated patch that adds option to serialize core analysis cache as string. Possible values of :dump-core are :raw, :string, :none. Old true/false values supported for backward compatibility.

As for default, current patch uses :raw, but I think it makes more sense to use :string. Saving extra few mb of final js is quite good. I think most devs won't go deep into figuring out why js is big and just leave it as it is. Additional one-time parsing performance hit :string introduces acceptable: when :string is used, page loads in 1s while with :raw the time is ~800ms.

Comment by David Nolen [ 16/Jun/17 12:41 PM ]

I'm questioning whether this actual valuable? It seems to me if you're serious about code size you would just use Transit and them load the analysis asynchronously?

Comment by Nikita Beloglazov [ 17/Jun/17 2:39 AM ]

Yes, if size is critical then there are better ways to hand-tune the way of loading analysis. At the same time having 3m vs 6m file for local/simple development is a nice win. The only downside is speed, but I feel like big reduction in size is better than small speed penalty.





[CLJS-1297] defrecord does not emit IKVReduce protocol Created: 03/Jun/15  Updated: 05/Oct/17

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

Type: Defect Priority: Major
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: newbie

Attachments: Text File CLJS-1297-19-July-2015.patch    
Patch: Code and Test

 Description   

Records are maps and in Clojure they support reduce-kv (IKVReduce protocol).
This is not true in ClojureScript:

(defrecord Foobar [x y])
 (reduce-kv assoc {} (Foobar. 1 2))

Fails wit Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]



 Comments   
Comment by David Nolen [ 03/Jun/15 7:25 PM ]

Just seems like an oversight. Patch welcome, this one is a relatively easy one.

Comment by Daniel Skarda [ 04/Jun/15 2:53 AM ]

OK

I checked Clojure implementation. Records do not implement any reduce protocol on their own. For IKVReduce records use default implementation using reduce and destructuring. Is this approach OK?

Recently Alex Miller implemented many optimizations of reduce protocols in Clojure. Eg range returns an object which implements IReduce protocol so reduce (and transducers in general) can take advantage of it. Any plans for such optimizations in ClojureScript?

;;clojure/src/clj/clojure/core.clj:6523
;;slow path default
clojure.lang.IPersistentMap
(kv-reduce 
  [amap f init]
  (reduce (fn [ret [k v]] (f ret k v)) init amap))
Comment by David Nolen [ 04/Jun/15 9:05 AM ]

Going with the Clojure implementation is fine. Yes all of the optimizations in 1.7.0 are on the table for ClojureScript but these are separate issues from this one.

Comment by Samuel Miller [ 16/Jul/15 10:39 PM ]

Mind if I take this as my first cljs bug? Poking around quickly I think I know what needs to happen.

Comment by David Nolen [ 17/Jul/15 5:21 AM ]

Sure! Have you submitted your CA yet?

Comment by Samuel Miller [ 17/Jul/15 7:13 PM ]

Yes, I did yesterday.

Comment by Samuel Miller [ 20/Jul/15 9:52 PM ]

Here is a potential patch. I implemented a basic IKVreduce based on Daniel Skarda's comment. Note: I am a little fuzzy on macros still so please look over what I have. There is probably a better way. Also added a test for reduce-kv on records.

I ran the test on Linux on V8 and SpiderMonkey. I plan to get JSC and Nashorn working and tested this week but if someone wants to test them out before that would be great.

Comment by Sebastian Bensusan [ 23/Jul/15 6:45 PM ]

Experience report:

I just tested the patch in the Node Repl and it seems to work:

cljs.user=> (defrecord A [a b])
cljs.user/A
cljs.user=> (reduce-kv (fn [m k v] (assoc m k (inc v))) {} (A. 1 2))
{:a 2, :b 3}

and the provided tests passed in Spidermonkey, V8, and Nashorn (I don't have JSC installed).

For completeness: before applying the patch the same code fails with:

Error: No protocol method IKVReduce.-kv-reduce defined for type : [object Object]
Comment by David Nolen [ 10/Aug/15 10:22 PM ]

Is this the same approach taken by Clojure?

Comment by Samuel Miller [ 10/Aug/15 10:36 PM ]

You can see the relevant current Clojure code here...
https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L6526
I think it is the same. I literally just tried to translate it over into CLJS. I might of understood something wrong though.

Comment by David Nolen [ 11/Aug/15 6:10 AM ]

Yes that's the slow path. Please use the implementation used by defrecord instead. If defrecord doesn't have one then this patch is OK.

Comment by Samuel Miller [ 11/Aug/15 8:48 PM ]

As far as I can tell there is no implementation on defrecord itself however there are separate implementations on the the java classes PersistentVector, PersistentArrayMap, PersistentTreeMap, and PersistenHashMap in pure java. I am not sure if you would want to do something similar for Clojurescript.

I can also spend some time trying to make a more performant version.

Comment by António Nuno Monteiro [ 27/Jul/16 7:38 AM ]

Confirmed that Clojure uses the slow path via the IPersistentMap implementation in defrecord
https://github.com/clojure/clojure/blob/d920ad/src/clj/clojure/core.clj#L6712

Patch still applies and can also confirm it works for me.





[CLJS-1141] memoization of js-dependency-index and get-upstream-deps needs knobs Created: 18/Mar/15  Updated: 05/Oct/17

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJS_1141.patch     Text File CLJS-1141-with-js-dep-caching-latest.patch    
Patch: Code

 Description   

knobs should be exposed for more dynamic compilation environments like Figwheel which may desire to add dependencies to the classpath on the fly.



 Comments   
Comment by Bruce Hauman [ 21/Mar/15 3:51 PM ]

A patch that caches upstream dependencies in the compiler env.

Comment by Bruce Hauman [ 21/Mar/15 3:59 PM ]

Actually I'm going to submit another patch that includes the memoize calls in js-deps.

Comment by Bruce Hauman [ 28/Mar/15 12:50 PM ]

New patch that moves cljs.js-deps memoization to current env/compiler as well as get-upstream-deps.

Unfortunately there is a circular dep between cljs.env and cljs.js-deps, if we want to cache in env/compiler. I overcame this with a resolve.

Compile performance is either completely unchanged or slightly improved based on several test runs.

Comment by Bruce Hauman [ 28/Mar/15 2:22 PM ]

Hold off on this. Its not behaving as expected. Doesn't seem to be caching in certain situations.

Comment by David Nolen [ 28/Mar/15 2:26 PM ]

Thanks for the update. This will definitely not land until after the pending REPL/piggieback release anyhow.

Comment by Bruce Hauman [ 28/Mar/15 2:44 PM ]

Yeah there is an obvious bug and a subtle one. Hopefully will finish it up soonish.

Comment by Bruce Hauman [ 28/Mar/15 3:43 PM ]

Alright, this latest patch works. There was a subtle memoizing nil value bug.





[CLJS-1009] Allow deps.cljs to declare a foreign lib as remote Created: 05/Feb/15  Updated: 05/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-CLJS-1009-allow-remote-url-in-deps.cljs-foreign-libs.patch    
Patch: Code

 Description   

When using things like the Google Maps JS API [1] the Javascript that is required can't be bundled inside a jar as it depends on the used API key.

To be able to provide externs for those kind of libraries there should be a way to declare them as "remote" in deps.cljs.

[1] https://developers.google.com/maps/documentation/javascript/tutorial



 Comments   
Comment by Nathan Dao [ 08/Apr/17 6:43 PM ]

js_deps.cljs deliberately only allows upstream foreign-libs (defined in deps.cljs file) to be local file. Yet, this issue is still open, so I (hastily ) assume the local file enforcement was not intentional.

The patch should add back support for using url as foreign-libs in deps.cljs and remove unused codes in load-foreign-library*.





[CLJS-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 05/Oct/17

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

Type: Defect Priority: Minor
Reporter: Nikita Beloglazov Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File cljs-890.patch     File cljs-core-str-perf.diff    
Patch: Code

 Description   

Example

(str #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)}) ; => "42"

The problem in the fact that ClojureScript uses concatenation to convert values to strings and that doesn't work well with objects which have valueOf() method overriden.

Example in js:

var obj = {
    toString: function() { return 'hello'; },
    valueOf: function() { return 42; }
};
console.log(String(obj)); => 'hello'
console.log(obj.toString()); => 'hello'
console.log('' + obj); => '42'

Potential solution might be to use String() function. Using toString() won't work as described in this issue: http://dev.clojure.org/jira/browse/CLJS-847



 Comments   
Comment by Kevin Neaton [ 24/Nov/14 10:34 AM ]

Is there a valid use case where toString and valueOf are not in sync? E.g.

(not= (.toString x) (js/String (.valueOf x))

If not, is it "incorrect" for the two methods to be out of sync?

Comment by Nikita Beloglazov [ 24/Nov/14 10:40 AM ]

Here is an example of such use case: https://github.com/processing-js/processing-js/blob/master/src/Objects/Char.js
That's how I found this bug.

Comment by Kevin Neaton [ 24/Nov/14 10:49 AM ]

Thanks for the link. I see what you mean.

Comment by David Nolen [ 02/Dec/14 5:08 AM ]

The problem with going with String appears to be a massive performance hit to printing http://jsperf.com/string-vs-tostring2/6.

Unless a brilliant idea is proposed this seems best solved / worked around in user code.

Comment by Nikita Beloglazov [ 02/Dec/14 6:41 AM ]

Append performs better on strings and numbers, but it performs worse on objects so it is not a clear performance hit. If I heavily work with objects and use (str) to convert them into strings then I actually lose on performance with current implementation.
Anyway current implementation of str is incorrect as it doesn't honor toString method. And this is what str function supposed to do. I believe a compiler should be correct first and then worry about performance.

Comment by David Nolen [ 02/Dec/14 7:38 AM ]

Sorry going back over this I believe the issue is really that we simply need to backout CLJS-801.

Comment by David Nolen [ 02/Dec/14 7:41 AM ]

reverted CLJS-801 in master

Comment by Francis Avila [ 02/Dec/14 10:32 AM ]

CLJS-801 only deals with the str macro. Aren't we still going to have str function problem because of CLJS-847? https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 (also Yet if we use toString there, Safari 6.0.5 blows up. Maybe we need {{[o].join('')}}? Depending on where the bug is this may be wrong in Safari 6.0.5 too.

What we need to do very specifically is somehow get the return value of the (in ECMASCRIPT-ese) ToString abstract operation on the object (or the underlying ToPrimitive abstract operation with the String hint). String concat with the add operator

Options as I see it are:

  • x.toString() : Bad because of CLJS-847
  • {{[x].join('')}} : Should work (and does right thing for null/undefined), but I think we should test in Safari 6.0.5. Also very slow.
  • String
  • String.prototype.concat
  • String.prototype.slice(x,0) String.prototype.substring(x,0) String.prototype.substr(x, 0)
  • x.toString() normally, but String if we detect that we'll trigger CLJS-847. (Can specialize on startup.)
Comment by David Nolen [ 02/Dec/14 10:35 AM ]

Is there any evidence that higher usage of str is actually problematic?

Comment by Francis Avila [ 02/Dec/14 10:44 AM ]

String concat using the addition operator uses an un-hinted ToPrimitive abstract call (which will try x.valueOf() first then x.toString(), usually) and then {{ToString}}s the result of that, so it's not an option unless we are concating primitive values.

Details:

Comment by David Nolen [ 02/Dec/14 10:50 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

Comment by Francis Avila [ 02/Dec/14 11:01 AM ]

Is there any evidence that higher usage of str is actually problematic?

Kevin Neaton, who opened CLJS-847, was using a patch in production which only addressed the higher order case and he said the patch fixed the issue for them. He was unaffected by the str macro case because it either used ''+x already (with CLJS-801 applied) or it used {{[x].join('')}} (which hasn't been tested with Safari 6.0.5 yet, but probably works).

So if we had a problem using ''+x with the str macro, we will certainly have a problem with ''+x with a string function as long as CLJS-847 is applied.

I haven't pulled down master yet, but here is a test case which I bet will fail with the CLJS-847 patch:

(def tricky-obj #js {"toString" (fn [] "hello") "valueOf" (fn [] 42)})
(assert (= (apply str tricky-obj) "hello")) ;; will get "42"
Comment by Francis Avila [ 02/Dec/14 11:09 AM ]

I'm not really all that concerned about the specification, only if it matters in the wild. If this doesn't affect Safari 6.05 we don't care.

To be clear, there are two issues here:

CLJS-847: x.toString() fails on Safari 6.0.5. Workaround is ''+x (only done in str macro case).
CLJS-890: ''+x doesn't give expected results for objects which define valueOf. Expectation is that x.toString() is called, instead x.valueOf().toString(). Fix is to use array join instead of string concat in str macro, but it doesn't address the ''+x workaround from CLJS-847.

To make matters worse, it looks like the toString() error on Safari may only be triggered at certain JIT levels!

Comment by Francis Avila [ 02/Dec/14 11:10 AM ]

Workaround is ''+x (only done in str macro case).

I mean "Workaround is ''+x (only done in str function case)."

Comment by Nikita Beloglazov [ 08/Dec/14 6:14 PM ]

Can this bug be reopened meanwhile? If I understand correctly the fix should affect https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642 but this code still present in HEAD.

Comment by David Nolen [ 08/Dec/14 6:37 PM ]

We've switched to goog.string.buildString in master https://github.com/clojure/clojurescript/commit/94eb8a960fef6aaca4ba44b251cefbfa04d0f6ac

Comment by Nikita Beloglazov [ 08/Dec/14 8:32 PM ]

Yes, that works. Cool, thanks!

Comment by Thomas Heller [ 01/Jan/15 7:12 AM ]

Sorry for re-opening.

I was doing some profiling of my code and noticed a warning in the profiling output about cljs.core/str.

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

goog.string.buildString = function(var_args) {
  return Array.prototype.join.call(arguments, '');
};

Given that we don't ever call it with more than one argument it is probably not best implementation choice.

Maybe skip the call and inline it ala

(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x] (if (nil? x)
         ""
         (.join #js [x] "")))
  ([x & ys]
    (loop [sb (StringBuffer. (str x)) more ys]
      (if more
        (recur (. sb  (append (str (first more)))) (next more))
        (.toString sb)))))

I didn't follow this issue but why are we not using .toString? The buildString/array approach seems kind of hackish?

I'm not too sure about the overall impact but since cljs.core/str showed up pretty high in my profile I think this should be investigated further.

Comment by Thomas Heller [ 01/Jan/15 7:28 AM ]

Before:

;;; str
[], (str "1"), 1000000 runs, 254 msecs
[], (str 1), 1000000 runs, 266 msecs
[], (str nil), 1000000 runs, 80 msecs
[], (str "1" "2" "3"), 1000000 runs, 753 msecs

After:

;;; str
[], (str "1"), 1000000 runs, 82 msecs
[], (str 1), 1000000 runs, 86 msecs
[], (str nil), 1000000 runs, 79 msecs
[], (str "1" "2" "3"), 1000000 runs, 242 msecs

But I only tested V8, probably needs some verification.

Comment by Thomas Heller [ 01/Jan/15 7:39 AM ]
(defn str
  "With no args, returns the empty string. With one arg x, returns
  x.toString().  (str nil) returns the empty string. With more than
  one arg, returns the concatenation of the str values of the args."
  ([] "")
  ([x1]
     (.join #js [x1] ""))
  ([x1 x2]
     (.join #js [x1 x2] ""))
  ([x1 x2 x3]
     (.join #js [x1 x2 x3] ""))
  ([x1 x2 x3 x4]
     (.join #js [x1 x2 x3 x4] ""))
  ...)

Does perform even better.

;;; str
[], (str "1"), 1000000 runs, 40 msecs
[], (str 1), 1000000 runs, 43 msecs
[], (str nil), 1000000 runs, 96 msecs
[], (str "1" "2" "3"), 1000000 runs, 117 msecs

How many args should it inline?

Comment by David Nolen [ 01/Jan/15 12:43 PM ]

I'd be OK with up to 4 then variadic.

Comment by Thomas Heller [ 01/Jan/15 5:05 PM ]

There is some weird interaction between the code generated by the cljs.core/str macro and function.

The macro generates

(str "hello" 1 "world" :yo nil)

yields

[cljs.core.str("hello"),cljs.core.str((1)),cljs.core.str("world"),cljs.core.str(new cljs.core.Keyword(null,"yo","yo",1207083126)),cljs.core.str(null)].join('');

Given that str with 1 arg will basically unroll to

[["hello"].join(""), ...]

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

Comment by Francis Avila [ 02/Jan/15 11:14 AM ]

Chromes complains with: "Not optimized. Bad value context for arguments value", looking further at the implementation of goog.string.buildString

Chrome complains about the variadic function dispatch code in the same way, see CLJS-916 plus patch.

I think it might be safe to completely remove the macro since cljs.core/str would then do the same and the JIT is probably smart enough to figure this out (or even Closure when compiling).

The Closure compiler is not smart enough to remove the intermediate array, which is why I filed CLJS-801 (which this ticket rolled back). I don't think JITs can do it either.

I am beginning to wonder if we should ignore the Safari 6.0.5 problem in CLJS-847 that started all this string mess. To recap:

  1. CLJS-801 is accepted, which removes [123, x].join('') in the str macro case in favor of ''+123+(cljs.core/str$arity$1 x) style code, which the closure compiler can precompute. At this time, the one-arg cljs.core/str function (not macro) calls toString on its argument.
  2. CLJS-847 is filed. On Safari 6.0.5 at higher JIT levels, calling toString on some things (possibly only unboxed numbers? definitely not strings) throws a TypeError. This is unquestionably a bug in Safari. David fixes by making one-arg cljs.core/str function call js-str instead of toString. js-str uses string-concat ''+x.
  3. However, this breaks for objects that define valueOf (issue in current ticket), because in js ''+x is the same as ''+x.valueOf().toString() not ''+x.toString().
  4. David considers using String() and variations but rejects because of performance hit.
  5. David rolls back CLJS-801 from the string-concat back to the array-join style to fix.
  6. Nikita and I point out that rolling back CLJS-801 only fixes the str macro, not the string function, which still uses js-str and hence string-concat.
  7. David fixes the str function to use goog.string.buildString, which has the behavior of array.join. Behavior is now correct even on Safari 6.0.5.
  8. Thomas points out that buildString uses arguments in a way unoptimizable by v8, and now the str function (not macro) has a performance regression. He suggests using [].join() directly.

So, there's a lot of back and forth on this issue, and it's all because of a bug in Safari 6.0.5 which no one has been able to reproduce first-hand because Safari 6.0.5 is old and rare. For some perspective, Safari 6.0.x was only available on Lion and Mountain Lion between July 25,2012 and June 11,2013. Before July 25,2012 Lion used Safari 5.1.x and there was no Mountain Lion. On June 11, 2013, both Lion and Mountain Lion switched to Safari 6.1.x which does not suffer from the toString TypeError bug (I checked--I have an iMac with Lion on it). The only machines on Safari 6.0.5 are (Mountain) Lion machines which used software updates until late 2012-early 2013 and then stopped. I can't imagine this is a large number of people.

It is theoretically possible for me to run Safari 6.0.x on my Lion machine to actually test this, but I can't find a way to downgrade from 6.1.x.

I think the options are:

  1. Use array.join() for all stringification and take the performance hit (which we should quantify). Include a comment that this is only for Safari 6.0.x (only confirmed second-hand on 6.0.4 and 6.0.5) for future generations, who are going to think this is weird.
  2. Use CLJS-801 and toString (status quo before CLJS-847), and ignore this problem for Safari 6.0.x.
  3. Use CLJS-801, but add a number? check (with comment) to cljs.core/str$arity$1 for Safari 6.0.5. The number case should use js-str, and the rest toString. I think this will work, but again we have no way to test--we really need to get our hands on a Safari 6.0.x browser.

Of course we should benchmark these approaches but my hunch is that 2 is faster than 3 is faster than 1.

Comment by David Nolen [ 02/Jan/15 11:16 AM ]

We are not going to ignore Safari 6.0.X. Any decisions made about this ticket will include supporting it.

Comment by Francis Avila [ 10/Jan/15 4:12 AM ]

Update on some research I am doing into this.

I created a jsperf of alternative str implementations that I am trying out. Right now I've only looked at one-arg str. I discovered a few things:

  • {{''+[x]}} is a faster alternative to [x].join('').
  • Advanced compilation can compute {{''+[x]}} at compile time if x is a bool, str, undefined, null, or number, even through function calls! I.e. str_test.str_arr(123) compiles to "123" without macro magic.
  • However, using an intermediate array (even if a preallocated singleton) is still slower than the old (if (nil? x) "" (.toString x))
  • Using a switch statement is as least as fast as the str-tostr baseline, usually faster.
  • I am 99% sure all these implementations (except str-tostr, the baseline, which definitely fails) work on the dreaded Safari 6.0.x. If anyone has this version, point it at the jsperf link above and run the tests. I think Browserstack has this version of Safari.

I'm still investigating the variadic case (str x y z a b c). It might be better to use reduce instead of Stringbuffer+seq. (Stringbuffer just does ''+x now instead of an array-join.)

Comment by Thomas Heller [ 10/Jan/15 6:37 AM ]

Sorry, got side-tracked for a bit.

@Francis: Thanks for the recap.

Don't have Safari 6 available either, but seems wrong that we all have to suffer because is minor percentage still has this (667 users of 190k+ on my site). Don't have a solution cause I can't test whether it works, we might try String.concat.

"".concat(obj); // "42"
"".concat(obj, ""); // "hello"
String.prototype.concat(obj, "") // "hello"
String.prototype.concat("", obj) // "hello"

But no idea if String.concat works, also it behaves odd with regards to valueOf.

http://jsperf.com/js-string-concat-variants

Perf is also inconclusive since Firefox appears to be cheating.

Comment by Francis Avila [ 10/Jan/15 2:04 PM ]

Tested that jsperf with Safari 6.0.5 using Browserstack, results are there.

Note I could not reproduce CLJS-847 because str-tostr does not fail as expected. I will try harder now that I have a browser to test.

Comment by Francis Avila [ 10/Jan/15 6:55 PM ]

Still cannot reproduce CLJS-847.

This script includes my attempt at a minimum reproducible case. My theory was that certain types at higher jit levels would fail. I could not get any case to fail. I also tried flapping back and forth between types and using one type at a time, but still no failures.

In this thread I found this "minimal" script which the OP said he could get to fail reliably. I could not get it to fail. However the original post was from feb 15, 2013, which means the Safari he was using would have to be 6.0.2 or lower.

Hypotheses:

  1. This error does not affect 6.0.5 but maybe 6.0.4 or lower.
  2. BrowserStack's system somehow mitigates the bug, meaning we need a "real" Lion Safari 6.0.x to test.
  3. These tests only fail under the correct phase of the moon.

So I can code up a patch for str using the str-switch implementation (which is at least a bit faster on some browsers), but I have no idea if it may fail on Safari 6.0.5. I only know that it works so far. CLJS-801 should also be safe to reapply because the root cause of all issues is the implementation 1-arity of the cljs.core/str function.

I have also asked for Kevin's help back in CLJS-847. (Kevin was the original reporter of the Safari 6.0.x issue.)

Comment by Francis Avila [ 19/Jan/15 12:51 AM ]

Made a jsperf of variadic cases. Chrome seems to really prefer IReduce to seq+stringbuilder for vectors (other collections not tested), but there is no difference or a small slowdown on other browsers. Not sure if it's worth it.

Also updated arity-one cases with a str using switch and never using toString. Nearly 50% slower than using switch or toString on Chrome, smaller on Safari.

In terms of safety str-switch-notostr does not use toString at all so is probably safer. I think str-switch will likely work too, though, and is significantly faster. However I haven't been able to get any TypeErrors in Safari 6.0.5 so it's anyone's guess.

I suggest something like this as a new str (which doesn't use reduce, but could):

(defn str
 ([x]
  (case (js* "typeof ~{}" x)
   "string" x
   "object" (if (identical? nil x) "" (.toString x))
   ("boolean" "number") (js-str x)
   "undefined" ""
   (js-str #js [x])))                                       ;; insurance against Safari 6.0.x TypeError bug.
 ([a b] (js* "~{}+~{}" (str a) (str b)))
 ([a b c] (js* "~{}+~{}+~{}" (str a) (str b) (str c)))
 ([a b c d] (js* "~{}+~{}+~{}+~{}" (str a) (str b) (str c) (str d)))
 ([a b c d & more]
  (loop [s (str a b c d) [e f g h & r] more]
   (let [s' (js* "~{}+~{}+~{}+~{}+~{}" s e f g h)]
    (if (nil? r)
     s'
     (recur s' r))))))
Comment by Francis Avila [ 19/Jan/15 11:24 PM ]

First cut of a possible patch that resolves this while not breaking CLJS-847. Should wait for confirmation that this does not break Safari 6.0.x.

Comment by Francis Avila [ 19/Jan/15 11:34 PM ]

Oops forgot tests.

Comment by Francis Avila [ 03/Feb/15 10:24 AM ]

Update in CLJS-847: original reporter was not able to reproduce his original bug report in Safari 6.0.x running in BrowserStack. This may be because of BrowserStack, but it's the best we have.

Given how hard this bug is to reproduce, how few people it affects, and how significant the performance regression is, I still think we should go back to the simple (if (nil? x) "" (.toString x)) implementation. However, you could also try the patch on this ticket (using a typeof switch), which at least (handwaving) might fix this bug in Safari 6.0.x and is a little faster than a simple .toString in Chrome and not much slower elsewhere. (The reason I think it might avoid this bug in Safari is that it avoids calling .toString on non-Objects.)

Comment by Antonin Hildebrand [ 25/Apr/15 11:08 PM ]

I wonder if you considered swapping str function at runtime during CLJS init phase.

Implement str function using plain .toString() call (original solution). And at startup check for Safari 6.0.x presence and optionally swap str for implementation wrapping .toString() call in a try-catch block silencing TypeError exceptions by falling back to Safari 6.0.x friendly .toString() alternative.

We would get correct semantics in all cases. And price would be just slower printing execution on Safari 6.0.x not on all systems.





[CLJS-868] no arity warnings on recursive calls Created: 03/Oct/14  Updated: 05/Oct/17

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File cljs_868_14_Nov_2015.md     Text File cljs_868_14_Nov_2015.patch    
Patch: Code

 Description   

If a function recursively invokes itself within its own body the invoke will not be checked for arity mismatch.



 Comments   
Comment by Samuel Miller [ 10/Aug/15 10:06 PM ]

Took some time to look at this issue. Originally thought "Do what loop/recur does" but that does not take into account multi-arity. It seems like maybe the best option is to somehow use the second pass of the analyze(analyze-fn-methods-pass2). The entire information about the function is present and the warning section of the code gets triggered but because of no-warn is ignored. Any other ideas for a solution to this?

Comment by Samuel Miller [ 14/Nov/15 7:47 PM ]

So I am looking for feed back on this patch and I will try to explain the reasoning for each section.

The issue is that a function only knows about it's arity after it has been parsed once.
So we need to check arity issues on the second pass

First off, added two new variables.
-activate-second-pass-warnings:Boolean Basically if you want to have second-pass warnings turned on
-second-pass-cljs-warnings:Set Right now we only have :fn-arity but I figure might as well make it generic.

So first up if the modifications to the analyze-fn-methods-pass2 function.
Instead of using no-warn marco here we have some new functionality.
The goal is to turn everything off except the second-pass warnings

So if activate-second-pass-warnings is false just use no-warn else it will use the new section of code.

The default-warning-handler was also modified. After checking if a warning is on, it checks if the warning is a second-pass warning and
if that warning can now be activated. If activate-second-pass-warnings is false AND a warning is still on that implies it is a second pass warning
in the second pass so we activate it.

Also I tried to keep all modifications in cljs.analyzer.

Originally I had the cljs-warnings :fn-arity to false and it would only be turned on in the second pass.
However the repl section just sets everything to true (and turns off select parts like ns errors).
So I decided to not touch those sections and instead keep how other files interface with the analyzer the same.

Comment by Samuel Miller [ 16/Nov/15 10:58 PM ]

Just realized that I have the patch marked as .md instead of .patch





[CLJS-844] Optimize js->clj by switching to transients Created: 22/Aug/14  Updated: 05/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Darrick Wiebe Assignee: Darrick Wiebe
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File js-to-clj-using-transients.patch     Text File use-transducers-in-js-to-clj.patch     Text File use-transducers-in-js-to-clj.patch    
Patch: Code

 Comments   
Comment by David Nolen [ 23/Aug/14 1:19 PM ]

Did you any benchmarks on other JS engines?

Comment by Darrick Wiebe [ 23/Aug/14 2:14 PM ]

No. My main concern was to check if there was any startup overhead that might offset the basic efficiency improvements for processing small collections. Using into with a transducer turned out to be faster in every scenario, significantly in most.

Comment by David Nolen [ 23/Aug/14 2:19 PM ]

It would be nice to see a jsperf of before and after on this ticket. Thanks.

Comment by Darrick Wiebe [ 23/Aug/14 2:23 PM ]

Is there a existing one that I can work from?

Comment by David Nolen [ 23/Aug/14 2:35 PM ]

There is not. I generally just make a simple project, create an advanced compiled artifact and copy and paste it into jsperf.

Comment by Darrick Wiebe [ 24/Aug/14 7:24 PM ]

Turns out reducing into a transient is considerably better than using a transducer (which was itself a little faster) for this.

http://jsperf.com/js-clj-transducer-test

The code is at:

https://gist.github.com/pangloss/591d77231fda460c2fbe

Let me know if you want me to prepare an updated patch.

Comment by David Nolen [ 25/Aug/14 7:34 AM ]

Thanks for putting this together. Yes please provide an updated patch.

Comment by Darrick Wiebe [ 26/Aug/14 11:19 AM ]

Not sure whether the convention is to comment that I've uploaded a new patch. Regardless, I uploaded it yesterday.

Comment by Marcin Kulik [ 07/Oct/15 1:46 PM ]

I have tested and benchmarked the patch on a big js objects (5MB+ json files) and I confirm that new-js->clj3 function is more than 2x faster. It runs now in the player on https://asciinema.org and all seems good so far.

Comment by David Nolen [ 08/Oct/15 2:33 PM ]

Marcin thanks for the feedback. Experience reports always help push tickets forward.

Darrick, the patch need to be rebased to master. Please remove all patches except the one that will be applied.

Comment by Rohit Aggarwal [ 08/Oct/15 3:06 PM ]

[ClojureScript beginner here. so do ignore this if I am mistaken]

The patch of 25/Aug uses `(aget x k)` instead of `(goog.object/get x k)` for getting the value of a key if x is an object. I believe it isn't the right way even though it works.

This contains why the latter is preferred over the former.

Comment by David Nolen [ 09/Oct/15 12:33 PM ]

Rohit good catch. Yes that should be changed.

Comment by Thomas Spellman [ 21/Oct/16 3:01 AM ]

Added a patch, "js-to-clj-using-transients.patch", on Oct 16, 2016 that supersedes "use-transducers-in-js-to-clj.patch" from Aug, 2014. This patch changes cljs.core/js->clj to use transients. Also included is a change to both js->clj and clj->js to use gobject/get and gobject/set instead of aget and aset on JS object.

The JSperf shows a 17% speed improvement in Chrome on the first run, but about equal on the second run: https://jsperf.com/js-clj-transient-perf-test

The code for the JSperf is here: https://gist.github.com/thos37/41c0b38bea3270988a3275332686ab49

What would be the ideal test data for this?





[CLJS-633] Missing Java type hints in closure.clj Created: 22/Oct/13  Updated: 05/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Julien Eluard Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File cljs_ticket_633.patch    
Patch: Code

 Description   

A number of reflection warnings are generated from closure.clj. Fixing those probably would not improved performance but it would be cleaner anyway.



 Comments   
Comment by Julien Eluard [ 22/Oct/13 8:03 AM ]

Attached patch.

There are a couple warnings left, similar to:
Reflection warning, cljs/closure.clj:232:1 - reference to field url can't be resolved.
They appear to be due to some unexpected protocol method name? It looks harmless.





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

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

Type: Defect Priority: Minor
Reporter: Max Kreminski Assignee: Unassigned
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-2277] Document return value of js-delete Created: 27/Jul/17  Updated: 04/Oct/17

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

Type: Enhancement Priority: Trivial
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-CLJS-2277-Update-docstring-to-reflect-exist-status.patch    
Patch: Code

 Description   

Document that js-delete returns true upon success, false otherwise.






[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-2376] Add support for ES6 default imports/exports Created: 02/Oct/17  Updated: 02/Oct/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   

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






[CLJS-2371] ns form :rename-macros is inaccessible Created: 29/Sep/17  Updated: 01/Oct/17  Resolved: 01/Oct/17

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

Type: Defect Priority: Trivial
Reporter: Timothy Pratley Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(ns cljs.user
(:require [n.m :refer-macros [a] :rename-macros {a b}]))

Could not Analyze src/cljs/user.cljs
Only :as, :refer and :rename options supported in :require / :require-macros; offending spec: (power-turtle.m :rename-macros {all all2}) at line 1 src/cljs/user.cljs

The ClojureScript compiler has code to handle :rename-macros, but it is inaccessible due to an overly restrictive spec.



 Comments   
Comment by António Nuno Monteiro [ 29/Sep/17 11:07 AM ]

This is intentionally not supported. `:rename-macros` is not part of the require spec.

The code you're referring to is supposed to handle the `(require-macros :refer .. :rename)` case.

Comment by Timothy Pratley [ 29/Sep/17 11:38 AM ]

Hi António,

I think that if a require statement can refer macros, it should be able to rename the symbols...
mainly because it brings the syntax closer to Clojure.
Perhaps the goal should instead be to support

(ns foo (:require [n.m :refer-macros [a] :rename {a b}]))

What do you think?

Comment by Thomas Heller [ 29/Sep/17 12:12 PM ]

If the macro namespace has a corresponding CLJS namespace that self-requires you do not need any of the :*-macros keywords at all, so :rename,:refer just work without additional work.

This goes back to CLJS-948 and I'd consider it best practice these days. Using a .macros namespaces just makes things complicated to use.

;; my/lib.clj
(ns my.lib)
(defmacro foo ...)

;; my/lib.cljs
(ns my.lib
  (:require-macros [my.lib]))

;; my/app.cljs
(ns my.app
  (:require [my.lib :as lib :refer (foo) :rename {foo bar}]))

(bar)

Did you consider this?

Comment by Timothy Pratley [ 01/Oct/17 2:23 PM ]

Hi Thomas,

I did not consider that form; I was not aware of it.
Thank you for point it out... yes that works perfectly and is by far preferable.

Regards,
Timothy.

Comment by Timothy Pratley [ 01/Oct/17 2:25 PM ]

You can require/refer/rename without the -macro variants, which is preferable. See example by Thomas.





[CLJS-2300] Delegate clojure.string/capitalize to goog.string/capitalize Created: 04/Aug/17  Updated: 01/Oct/17

Status: In Progress
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: performance

Attachments: Text File CLJS-2300-2.patch     Text File CLJS-2300-3.patch     Text File CLJS-2300.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Since the implementation of clojure.string/capitalize (6 years ago), Closure library added an identical function (3 years ago), which is actually much faster:

cljs.user=> (simple-benchmark [s "abc"] (goog.string/capitalize s) 10000000)
[s "abc"], (goog.string/capitalize s), 10000000 runs, 1181 msecs
nil
cljs.user=>  (simple-benchmark [s "abc"] (clojure.string/capitalize s) 10000000)
[s "abc"], (clojure.string/capitalize s), 10000000 runs, 5295 msecs

We could just have ClojureScript's delegate to Closure's.



 Comments   
Comment by Mike Fikes [ 04/Aug/17 9:17 PM ]

Before/After tests look promising:

Before:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 166 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 380 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 833 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1541 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 53 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 438 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 903 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 1181 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 345 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 727 msecs

After:

Benchmarking with V8
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 28 msecs
Benchmarking with SpiderMonkey
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 263 msecs
Benchmarking with JavaScriptCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 5 msecs
Benchmarking with Nashorn
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 327 msecs
Benchmarking with ChakraCore
[s "a" f clojure.string/capitalize], (f s), 1000000 runs, 57 msecs
[s "aBcDeF" f clojure.string/capitalize], (f s), 1000000 runs, 63 msecs
Comment by Mike Fikes [ 04/Aug/17 10:53 PM ]

The benchmarks produced in the previous comment are with included in the attached patch.

The patch also includes a generative regression test, using the previous implementation as a reference.

All engines pass the test fine if you crank up the number of values tested, except for ChakraCore. ChakraCore appears to be buggy, and I'll follow up upstream with a ticket for them. At its core, if you capitalize "ß" you should get back "SS". With the original ClojureScript implementation, you get back "ﮰ" on ChakraCore, and with the revision in the patch (essentially Closure Library) you get back "鯪", neither of which are correct. So, this patch doesn't introduce a regression on ChakraCore.

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

ChakraCore bug: https://github.com/Microsoft/ChakraCore/issues/421

Comment by Mike Fikes [ 01/Oct/17 9:34 AM ]

CLJS-2300-2.patch rebases against current master.

Comment by Mike Fikes [ 01/Oct/17 11:50 AM ]

I took a look at ChakraCore's upper-case bug, and it is fairly non-trivial for them to fix. (The code relies on upper-casing strings in-place and thus can't handle things like ß -> SS .) Additionally, SpiderMonkey does the wrong thing mapping ß -> ß.

Given that engines are evidently prone to get this wrong, and we don't want to live with unit tests failing forever, attached CLJS-2300-3.patch essentially works around this by limiting the generative test to covering ASCII strings.





[CLJS-2375] Closure-Compiler: Intent to Remove AMD Module Support Created: 01/Oct/17  Updated: 01/Oct/17

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

Type: Defect Priority: Major
Reporter: Chad Killingsworth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Hi - I'm a maintainer of Closure-Compiler. The compiler currently has a pass to transform some types of AMD modules to CommonJS so that the CommonJS processing can rewrite the code. It's an old pass and nobody is maintaining it. I'd like to delete it.

I just wanted to check and make sure that Clojure isn't actively using it. If it is, we need to come up with some type of plan.

Thanks,

Chad Killingsworth



 Comments   
Comment by Juho Teperi [ 01/Oct/17 10:54 AM ]

We support AMD modules, but no, Cljs doesn't actively use them.

We'll need to be remove references to some options and classes when these are removed from Closure.





[CLJS-1576] cljs.js sourcemap support throws on non-latin1 characters Created: 17/Feb/16  Updated: 01/Oct/17  Resolved: 01/Oct/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 1.7.228
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Matt Huebert Assignee: Matt Huebert
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-1576-2017-rebased.patch     Text File CLJS-1576-2.patch     Text File CLJS-1576.patch     Text File CLJS-1576.patch     Text File CLJS-1576-rebased.patch    
Patch: Code

 Description   

In cljs.js/append-source-map we encode the source-map string in base64 without escaping non-latin1 characters. In Chrome, this throws the error: "DOMException: Failed to execute 'btoa' on 'Window': The string to be encoded contains characters outside of the Latin1 range."

Source: https://github.com/clojure/clojurescript/blob/master/src/main/cljs/cljs/js.cljs#L152

The problem & a couple of solutions are explained here: https://developer.mozilla.org/en-US/docs/Web/API/WindowBase64/Base64_encoding_and_decoding#The_Unicode_Problem



 Comments   
Comment by David Nolen [ 18/Feb/16 8:21 AM ]

Can bootstrapped users apply this and verify it works for them? Thanks.

Comment by Mike Fikes [ 18/Feb/16 10:03 AM ]

I tried this with Planck and I can confirmed that, with a function name in Japanese, sourceMappingURL does indeed change and then includes base-64 data that covers my entire set of functions (whereas previously it did not), but the Japanese function name appears to have been munged into some Latin-1 characters (which I suppose is the point of the patch).

With Planck, I can't confirm the overall functionality as Planck doesn't make use of this information with JavaScriptCore (it instead uses equivalent info stored in map files).

So, as far as I can tell, this patch is good in that it appears to be doing the right thing when run with the bootstrap compiler.

Comment by David Nolen [ 18/Mar/16 1:45 PM ]

OK, patch looks ok to me but it needs to be rebased to master.

Comment by Matt Huebert [ 21/Jul/16 7:06 AM ]

Same patch but rebased to master

Comment by David Nolen [ 22/Jul/16 7:28 AM ]

fixed https://github.com/clojure/clojurescript/commit/e8011e7fe95c81432ad9a1043422a807edb9c052

Comment by Matt Huebert [ 14/Aug/17 6:02 AM ]

After further use of source-mapping with files that contain unicode values, I discovered two bugs in my previous patch: we need to apply js/encodeURIComponent to the string first, and string/replace should have been passed a function as a second argument.

I have a patch ready to upload, and also pushed a repo with examples here where I test this against many unicode values / emoticons: https://github.com/mhuebert/base64-encrypt-test/blob/master/src/app/core.cljs

These btoa errors don't show up in non-browser environments so I don't see any existing test environment that would catch this stuff.

Comment by Matt Huebert [ 14/Aug/17 6:05 AM ]

New patch adding js/encodeURIComponent and fixed args to string/replace

Comment by Matt Huebert [ 01/Oct/17 7:02 AM ]

Rebased 2017 patch to master

Comment by David Nolen [ 01/Oct/17 10:03 AM ]

fixed https://github.com/clojure/clojurescript/commit/9778b34d9e988a28c64133c4751d235bbbd3e966





[CLJS-2342] Speed up printing of js object by using forEach and regex optimizations Created: 30/Aug/17  Updated: 01/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.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.





[CLJS-2294] Options checkers presume :optimizations is always set Created: 02/Aug/17  Updated: 30/Sep/17  Resolved: 30/Sep/17

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

Type: Defect Priority: Minor
Reporter: Juho Teperi Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2294-2.patch     Text File CLJS-2294-3.patch     Text File CLJS-2294-4.patch     Text File CLJS-2294.patch    

 Description   

Most opts checkers currently check against user provided options map, without implicit options added. This means that e.g. optimizations option is not necessarily set. Some options checkers depend on optimization option being set, and throw incorrect errors if optimization is not set, but e.g. shen source-map option is used.

I think the best solution is to remove separate all-opts options map, and instead just use the options map with implicit options added everywhere.



 Comments   
Comment by Juho Teperi [ 02/Aug/17 9:36 AM ]

I'm thinking build should call add-implicit-opts the first thing, and just bind that to opts so everything uses the same opts? No reason to have original opts and opts with defaults added separately?

Comment by Juho Teperi [ 02/Aug/17 10:06 AM ]

This patch removes all-opts and instead calls the add-implicit-opts the first thing in cljs.closure/build.

Comment by David Nolen [ 03/Aug/17 4:23 PM ]

I mean I'm pretty sure I'm OK with but can we please explain briefly what problem this is solving that you encountered?

Comment by Juho Teperi [ 03/Aug/17 4:28 PM ]

Right, the problem I encountered was that setting :source-map true without :optimizations option causes error. If :optimizations is nil, check-source-map goes to the branch that validates source-map options with optimizations enabled.

(cljs.closure/build
  "src"
  {:main "hello-world.core"
   :output-to "out/main.js"
   :output-dir "out"
   ; works if uncommented
   ; :optimizations :none
   :source-map true})


Exception in thread "main" java.lang.AssertionError: Assert failed: :source-map true must specify a file in the same directory as :output-to "out/main.js" if optimization setting applied
(or (nil? (:output-to opts)) (:modules opts) (string? source-map)), compiling:(/home/juho/tmp/problems/build.clj:3:1)
Comment by Juho Teperi [ 09/Sep/17 11:47 AM ]

Fixed patch to ensure it can be applied.

Comment by Juho Teperi [ 25/Sep/17 7:13 AM ]

Rebased patch

Comment by David Nolen [ 30/Sep/17 3:24 PM ]

fixed https://github.com/clojure/clojurescript/commit/300e326b83ba4110657bb646a9d396c5cb666501





[CLJS-2374] Print js/Infinity, js/-Infinity, js/NaN using new reader literals Created: 30/Sep/17  Updated: 30/Sep/17  Resolved: 30/Sep/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2374.patch    
Patch: Code and Test
Approval: Accepted

 Description   

In the Clojure 1.9 beta1 release you can round-trip

[##Inf ##-Inf ##NaN]
via code here

https://github.com/clojure/clojure/blob/861d48ea761418fcb59dc4eb1c63bd29c2f35231/src/clj/clojure/core_print.clj#L131-L143

This ticket asks for similar behavior in ClojureScript, whereas currenly on master the vector above is printed as

[Infinity -Infinity NaN]
.



 Comments   
Comment by Mike Fikes [ 30/Sep/17 9:09 AM ]

The benchmarks added focus on printing booleans and numbers, since
this is where the change was. It also adds a test for an empty JavaScript
object, since this is the first branch after the changed code.

A summary of the speedup is given for each value type and each engine,
followed by the full details.

Perhaps the two perf regressions that might be important for this candidate
change are slowdown of printing regular numbers in V8 (but perhaps that is
outweighed by the 1.40 speedup getting to everything else after the
code—the (js-obj) test, and general
slowdown for things after the code—the (js-obj) test—for
JavaScriptCore.

Speedup summary:

                 true    10  ##NaN  ##Inf ##-Inf (js-obj) 
            V8:  1.01  0.89   0.66   0.57   0.65    1.40
  SpiderMonkey:  1.00  1.08   1.23   1.11   1.05    0.95
JavaScriptCore:  1.31  1.25   1.05   0.94   0.89    0.71
       Nashorn:  1.09  1.04   1.15   1.16   1.07    1.12
    ChakraCore:  1.02  0.98   1.04   1.04   1.02    1.12

Before:

Benchmarking with V8
[x true], (pr-str x), 1000000 runs, 671 msecs
[x 10], (pr-str x), 1000000 runs, 998 msecs
[x js/NaN], (pr-str x), 1000000 runs, 844 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 734 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 740 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2683 msecs
Benchmarking with SpiderMonkey
[x true], (pr-str x), 1000000 runs, 3016 msecs
[x 10], (pr-str x), 1000000 runs, 3032 msecs
[x js/NaN], (pr-str x), 1000000 runs, 3504 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 3064 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 3000 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 6144 msecs
Benchmarking with JavaScriptCore
[x true], (pr-str x), 1000000 runs, 1083 msecs
[x 10], (pr-str x), 1000000 runs, 1076 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1223 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1179 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1191 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 2166 msecs
Benchmarking with Nashorn
[x true], (pr-str x), 1000000 runs, 3681 msecs
[x 10], (pr-str x), 1000000 runs, 2711 msecs
[x js/NaN], (pr-str x), 1000000 runs, 3247 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 3296 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 3155 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 6132 msecs
Benchmarking with ChakraCore
[x true], (pr-str x), 1000000 runs, 2590 msecs
[x 10], (pr-str x), 1000000 runs, 2461 msecs
[x js/NaN], (pr-str x), 1000000 runs, 2402 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 2389 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 2360 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 6866 msecs

After:

Benchmarking with V8
[x true], (pr-str x), 1000000 runs, 662 msecs
[x 10], (pr-str x), 1000000 runs, 1116 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1282 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1195 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1145 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 1920 msecs
Benchmarking with SpiderMonkey
[x true], (pr-str x), 1000000 runs, 3014 msecs
[x 10], (pr-str x), 1000000 runs, 2799 msecs
[x js/NaN], (pr-str x), 1000000 runs, 2839 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 2769 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 2844 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 6474 msecs
Benchmarking with JavaScriptCore
[x true], (pr-str x), 1000000 runs, 827 msecs
[x 10], (pr-str x), 1000000 runs, 859 msecs
[x js/NaN], (pr-str x), 1000000 runs, 1162 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 1253 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 1341 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 3037 msecs
Benchmarking with Nashorn
[x true], (pr-str x), 1000000 runs, 3369 msecs
[x 10], (pr-str x), 1000000 runs, 2619 msecs
[x js/NaN], (pr-str x), 1000000 runs, 2827 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 2844 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 2946 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 5492 msecs
Benchmarking with ChakraCore
[x true], (pr-str x), 1000000 runs, 2538 msecs
[x 10], (pr-str x), 1000000 runs, 2510 msecs
[x js/NaN], (pr-str x), 1000000 runs, 2315 msecs
[x js/Infinity], (pr-str x), 1000000 runs, 2296 msecs
[x js/-Infinity], (pr-str x), 1000000 runs, 2325 msecs
[x (js-obj)], (pr-str x), 1000000 runs, 6110 msecs
Comment by David Nolen [ 30/Sep/17 2:34 PM ]

fixed https://github.com/clojure/clojurescript/commit/d45012273029bd5df3973ea716394f368e1c44cc





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

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: 0
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





[CLJS-2372] Due to changes in infinity literals, hashing of infinity is now broken Created: 29/Sep/17  Updated: 30/Sep/17  Resolved: 30/Sep/17

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-2372-update-hash-to-use-the-new-infinity-litera.patch    
Patch: Code
Approval: Accepted

 Description   

Due to the addition of ##Inf and ##-Inf literals in clojure, tools.reader removed the experimental support for Infinity and -Infinity literals, conforming to the clojure reader version. This means that the code in cljs.core/hash needs to be updated to use ##Inf and ##-Inf rather than Infinity and -Infinity

Note: this requires a tools.reader version >= 1.1.0 to support the new ##Inf and ##-Inf literals



 Comments   
Comment by David Nolen [ 30/Sep/17 3:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/add0a41316839388da6331cdd45ce5d0a09a97fc





[CLJS-2373] Bump tools.reader to 1.1.0 Created: 30/Sep/17  Updated: 30/Sep/17  Resolved: 30/Sep/17

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Approval: Accepted

 Comments   
Comment by David Nolen [ 30/Sep/17 3:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/26bf9a5baf30bfbe81ef98b9db300b8c9232939f





[CLJS-2166] Add uri? predicate Created: 04/Jul/17  Updated: 26/Sep/17  Resolved: 26/Sep/17

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

Type: Defect Priority: Minor
Reporter: Rick Moynihan Assignee: Sameer Rahmani
Resolution: Completed Votes: 0
Labels: core

Attachments: Text File CLJS-2166.patch     Text File CLJS-2166.patch     Text File CLJS-2166.patch     Text File CLJS-2166.patch     Text File CLJS-2166.patch    
Patch: Code and Test
Approval: Screened

 Description   

Clojure introduced a `uri?` predicate in 1.9. Clojurescript currently doesn't have one.

@dnolen suggested on Slack that it would "probably need to tie to Closure Uri type".



 Comments   
Comment by Sameer Rahmani [ 11/Sep/17 5:01 PM ]

This commit adds support for `uri` to Clojurescript under `cljs.core/uri?`. It returns `true` if the given argument is an instance of `goog.Uri`, `false` otherwise.

Comment by Francis Avila [ 14/Sep/17 2:32 PM ]

The patch has unnecessary whitespace changes and it incorrectly commits a (def clojurescript-version "1.9.923").
`

Comment by David Nolen [ 14/Sep/17 2:35 PM ]

I agree with Francis's comments.

Comment by Sameer Rahmani [ 14/Sep/17 3:09 PM ]

I'll fix them

Comment by David Nolen [ 15/Sep/17 7:33 AM ]

I don't see why we need a try/catch there. We should remove that. Also don't we need to require goog.Uri?

Comment by Sameer Rahmani [ 15/Sep/17 8:09 AM ]

Patch Updated.

Comment by David Nolen [ 25/Sep/17 1:37 AM ]

Just require goog.Uri. You don't need to require goog.Uri in the tests.

Comment by Sameer Rahmani [ 25/Sep/17 9:48 AM ]

patch updated

Comment by David Nolen [ 26/Sep/17 3:01 PM ]

fixed https://github.com/clojure/clojurescript/commit/35ad08e6728d83d0e7e1cce1d724f080eb5abce4





[CLJS-1198] cljs.test may report summary before all async tests complete Created: 12/Apr/15  Updated: 25/Sep/17  Resolved: 25/Sep/17

Status: Closed
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3196
Fix Version/s: 0.0-3255

Type: Defect Priority: Major
Reporter: Jenan Wise Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: None

Attachments: File cljs-1198-1    

 Description   

cljs.test may report summary before all async tests complete.

E.g, if you have an async test that looks like this:

(deftest a-test
  (async done
         (testing "a slow async test"
           (go
             (<! (timeout 1000))
             (is (= 0 1))
             (done)))))

then the report output may look like this:

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

<1 second elapses>

FAIL in () (cljs_test_with_slow_async_example/core_test.js:201:49)
expected: (= 0 1)
  actual: (not (= 0 1))

Minimal repo: https://github.com/jenanwise/cljs-test-with-slow-async-example



 Comments   
Comment by Leon Grapenthin [ 15/Apr/15 12:28 PM ]

I am quite surre that the problem has been introduced with this commit: https://github.com/clojure/clojurescript/commit/9bf486b22cebf5aba4154d07f3ad52e990ddc305

@David
I don't understand the intent of the commit message "cljs.test needs to default to sync, remove broken validation"

Why should the execution strategy default to :sync?

From my reading of the commit this would imply that, unless one provides map fixtures, tests are executed without support for async testing.

This is what happens. The problem goes away if I one adds e. g.

(use-fixtures :each {:before (fn [_])
:after (fn [_])})

I don't see why you removed the guard around async tests in the :sync execution. Its purpose was to throw when execution strategy is :sync an and async test is encountered.

If you can help with the intent of your commit, I'd be glad to provide the fix for myself. What validation was broken?

Comment by David Nolen [ 15/Apr/15 1:41 PM ]

Leon, when I changed how top-level fn emission worked for cross module code motion even though I could see good code getting generated I couldn't run the tests because of the validation bit. After poking around I couldn't make heads or tails of the invariant the code was trying to maintain and so I removed it. All the tests started working again.

Happy to see the invariants re-introduced if the code more clearly documents intent so I can understand it

Comment by Leon Grapenthin [ 15/Apr/15 2:26 PM ]

Can you specify on what you mean by the invariant?

Comment by David Nolen [ 15/Apr/15 2:28 PM ]

Leon, I have no idea what the check was doing nor why it was done that way, only that it prevented me from successfully running more important tests. So you will have to explain the approach to me

Comment by Leon Grapenthin [ 16/Apr/15 2:51 PM ]

Here I try to give you a quick overview of what is going on in the code you have removed/altered:

It solved the problem brought up in our discussion regarding the CLJS-988: The style in which clojure.test requires fixtures to be written didn't allow one to determine when an async test is over. Hence you came up with this requirement: http://dev.clojure.org/jira/browse/CLJS-988?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#issue-tabs

This is the way I have chosen to implement it:

In test-vars-block we create a block that looks up fixtures in the current env and decides whether to support async testing via execution-strategy.

In any case except for wrapped fixtures we run with support for async testing by executing the provided test functions in a block so that they can return async tests which are then picked up by the block runner. Also, via test-var-block additional code is wrapped around and added after them for reporting purposes.

In the case where wrapped fixtures are provided, we can't just create a test-var-block in the same fashion because flow control is passed to the wrapping fixtures and they don't execute or return a block. The only way work around this is to provide them directly with a function that invokes a nested block runner. Implicitly reusing test-var-block here was a decision made for obvious DRY reasons.

Now if async tests are used in the test function, the nested block runner could return before testing is finished, continuing testing too early. This is why the execution strategy is called sync: It expects the nested block runners to finish synchronously.

Per your requirment, testing should be aborted if an async test is encountered in the latter case. We can only know whether a test is async once it has been called and returned something. I didn't see a way to find out what was returned in the :sync result-expr directly, since test-var doesn't return what the test has returned. Hence I picked to look at what the test has returned in the test-var-block and throw if an async test is returned and async tests are disabled, storing this condition in the test-env. Today I see a strong alternative to that: Directly wrap the :test fn in the var in the :sync result-expr to throw if it returns an async test. It surely seems cleaner. You'd get a true invariant because the test-env would be out of the game. Notice though that anyhting thrown there will be catched by the reporting exception handler in test-var-block and testing would continue, so it has to rethrow to truely abort testing. Directly looking up the test-env in the async macro would have been another alternative which I have discarded for its dirtiness because it requires users to invoke async during testing to work.

I hope this helped you to understand the code. To make the code more understandable one could rename execution-strategy to async-supported? and return a boolean.

In any case, the :async execution-strategy needs to be the default, otherwise returned async test objects are not expanded into the root block and this CLJS-1198 happens.

I can provide a patch with the aforementioned improvements on sunday if you like. Finally I have no idea why the check prevented your tests from running - Can you tell me which tests I can use to potentially reproduce the problem?

Comment by Leon Grapenthin [ 19/Apr/15 8:32 AM ]

Patch notes:

  • changes default execution-strategy back to :async
  • prevents async tests from running when execution-strategy is sync

@David: I had to dissect test-var-block a bit because apparently alter-meta! is broken for vars. Please see commented lines 549 and 552 - Also I couldn't check whether this resolves the issue you had with tests being prevented from running after the compiler changes.

Comment by Sebastian Bensusan [ 01/May/15 3:36 PM ]

Experience report: tested this patch with the problematic repo and some other slow async test cases including fixture variations and it seems to work as expected. It also worked with ClojureScript's own tests (tested on all platforms but JavaScriptCore).

Hope this helps.

Comment by David Nolen [ 05/May/15 6:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/c9bf3f4aadf79d5042a27a6ef0e4cd5db4c26604

Comment by Jenan Wise [ 11/May/15 12:31 PM ]

Many thanks!

Comment by Alex Miller [ 25/Sep/17 6:56 AM ]

Just reopened to delete spam comment.





[CLJS-1915] cljs.test: Index out of bounds for stack element w/o line/column Created: 28/Jan/17  Updated: 25/Sep/17  Resolved: 28/Jan/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1915.patch    
Patch: Code and Test

 Description   

The cljs.test/js-line-and-column function does not properly handle stack elements that have no line and column information encoded in them. In particular, if you pass in something like "eval@[native code]" (which can be part of a stack from JavaScriptCore), you will get an index out of bounds exception.



 Comments   
Comment by David Nolen [ 28/Jan/17 11:11 AM ]

fixed https://github.com/clojure/clojurescript/commit/2f8a1529955acc943ac8082ab5848b2cba54bc4d

Comment by Jinseop Kim [ 24/Sep/17 3:12 AM ]

I got error as follow: "SyntaxError: illegal character[Learn More] test.js:246:90".
I think you should change 'NaN' to 'js/NaN'.

— test.js
246: return new cljs.core.PersistentVector(null, 2, 5, cljs.core.PersistentVector.EMPTY_NODE, #NaN,##NaN, null);

Comment by Jinseop Kim [ 25/Sep/17 3:54 AM ]

https://dev.clojure.org/jira/browse/CLJS-2352





[CLJS-2370] Tests failing after CLJS-2352 Created: 24/Sep/17  Updated: 25/Sep/17  Resolved: 25/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: test

Attachments: Text File CLJS-2370.patch    
Patch: Code and Test

 Comments   
Comment by David Nolen [ 25/Sep/17 1:35 AM ]

fixed https://github.com/clojure/clojurescript/commit/62e78a830543de0806b0ebcaf01ad22a5f253cfb





[CLJS-2368] Self-host: Never compile macro namespaces with `:optimize-constants true` Created: 23/Sep/17  Updated: 25/Sep/17  Resolved: 25/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: António Nuno Monteiro
Resolution: Completed Votes: 0
Labels: bootstrap

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

 Description   

Macro namespaces may be evaluated in a compiler environment that is different from the one which has emitted the constants table, which leads to cryptic errors because the constants are not available in the evaluating environment.



 Comments   
Comment by David Nolen [ 24/Sep/17 4:43 AM ]

Looks like this one needs a rebase

Comment by David Nolen [ 25/Sep/17 1:34 AM ]

fixed https://github.com/clojure/clojurescript/commit/7916a6782b672c80a8c31621b0e66d3dc88db73a





[CLJS-2364] Bump Closure Compiler to the Sep 2017 version Created: 16/Sep/17  Updated: 25/Sep/17  Resolved: 25/Sep/17

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

Type: Task Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2364.patch    

 Comments   
Comment by David Nolen [ 25/Sep/17 1:33 AM ]

https://github.com/clojure/clojurescript/commit/6e5cb9046458cdacb783d2b2e0c376525a21518e

Comment by David Nolen [ 25/Sep/17 1:34 AM ]

https://github.com/clojure/clojurescript/commit/6e5cb9046458cdacb783d2b2e0c376525a21518e





[CLJS-2340] Have js-keys delegate directly to good.object/getKeys Created: 29/Aug/17  Updated: 24/Sep/17  Resolved: 24/Sep/17

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

Type: Enhancement Priority: Major
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2340.patch    
Patch: Code
Approval: Accepted

 Description   

js-keys is implemented using goog.object/forEach. This essentially results in JavaScript that does a for over each key in the object, (just like goog.object/getKeys), but instead of directly accumulating the keys in a JavaScript array, using forEach ends up incurring unnecessary overhead extracting (and ultimately discarding) the value associated with each key, and making a callback for each key. In :none mode, you can see that goog.object/getKeys can be twice as fast as js-keys at times (at least in JavaScriptCore—work for this ticket should probably include a new benchmark.)

This ticket asks that js-keys be implemented by directly delegating to good.object/getKeys. There are a few places, in printing, and in js->clj that could benefit from this speedup (and with the simple delegation, good.object/getKeys could even be inlined by hand at call sites if it improved perf).



 Comments   
Comment by Mike Fikes [ 29/Aug/17 7:06 PM ]
Speedup summary:

            V8:  7.2,  8.7
  SpiderMonkey: 29.5, 29.2
JavaScriptCore:  7.8,  4.8
       Nashorn:  6.2,  5.5
    ChakraCore:  3.6,  5.8


Before:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 72 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 96 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 177 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 146 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 133 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 78 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 804 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 749 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 91 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 129 msecs


After:

Benchmarking with V8
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 10 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 11 msecs

Benchmarking with SpiderMonkey
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 6 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 5 msecs

Benchmarking with JavaScriptCore
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 17 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 16 msecs

Benchmarking with Nashorn
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 128 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 136 msecs

Benchmarking with ChakraCore
[obj (js-obj "a" 1 "b" 2) f js-keys], (f obj), 400000 runs, 25 msecs
[obj (js-obj "a" 1 "b" 2 "c" 3 "d" 4 "e" 5 "f" 6) f js-keys], (f obj), 400000 runs, 22 msecs
Comment by David Nolen [ 24/Sep/17 4:44 AM ]

fixed https://github.com/clojure/clojurescript/commit/5aa12a3cb965a3657a4d9e58a6c168b8ac8bfc81





[CLJS-2367] Self-host: :def-emits-var leaks into loaded namespace processing Created: 20/Sep/17  Updated: 24/Sep/17  Resolved: 24/Sep/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-2367.patch    
Patch: Code and Test
Approval: Accepted

 Description   

If you require a namespace, any defs in that namespace should return the value, not the Var. On the other hand, defs evaluated at the REPL return Vars because :def-emits-var is set to true.

In self-hosted ClojureScript, defs in loaded namespaces produce Vars if :def-emits-var is set to true.

Here is a minimal repro:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval st
  '(require (quote foo.core))
  {:context :expr
   :def-emits-var true
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns foo.core) (def b (def a 3))"}))}
  prn)

(cljs.js/eval st
  'foo.core/b
  {:context :expr
   :eval cljs.js/js-eval}
  prn)

The last form should cause

{:value 3}

to be printed, but instead you get

{:value #'foo.core/a}
.

Similarly, for a macros namespace, this minimal repro

(cljs.js/eval st
  '(require-macros (quote bar.core))
  {:context :expr
   :def-emits-var true
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns bar.core) (def b (def a 3)) (defmacro c [] b)"}))}
  prn)

(cljs.js/eval st
  '(bar.core/c)
  {:context :expr
   :eval cljs.js/js-eval}
  prn)

Should cause

{:value 3}

to be printed, but currently this will cause things to really derail with

Error: failed compiling constant: #'bar.core$macros/a; ...


 Comments   
Comment by David Nolen [ 24/Sep/17 4:41 AM ]

fixed https://github.com/clojure/clojurescript/commit/4607affd1f399839b44eef1aa01e0645cee539d4





[CLJS-2352] Compiler emits invalid JS code for NaN/Inf/-Inf if used with CLJ >= 1.9.0-alpha20 Created: 09/Sep/17  Updated: 24/Sep/17  Resolved: 24/Sep/17

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

Type: Defect Priority: Major
Reporter: Shogo Ohta Assignee: David Nolen
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File CLJS-2352-2.patch     Text File CLJS-2352.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Since 1.9.0-alpha20, Clojure has its own external representations for NaN/Inf/-Inf: ##NaN / ##Inf / ##-Inf, respectively.

The current CLJS compiler emits code as is for those values, so if it is used with the latest Clojure, it will end up emitting invalid JS code:

=> (let [env (assoc (ana/empty-env) :context :expr)]
     (comp/emit-str (ana/analyze env ##NaN)))
"##NaN"
=> (let [env (assoc (ana/empty-env) :context :expr)]
     (comp/emit-str (ana/analyze env ##Inf)))
"##Inf"
=> (let [env (assoc (ana/empty-env) :context :expr)]
     (comp/emit-str (ana/analyze env ##-Inf)))
"##-Inf"


 Comments   
Comment by Shogo Ohta [ 12/Sep/17 12:43 AM ]

Added a patch.
I tested the code on my local env and saw the things worked fine, but I'm not sure I should have added CLJ 1.9.0-alpha for test profile.

Comment by David Nolen [ 15/Sep/17 3:26 PM ]

Patch looks fine but could we please add a test case.

Comment by Shogo Ohta [ 17/Sep/17 12:54 AM ]

Updated the patch.

Is it OK to directly use the raw Java constants for NaN/Infinity/-Infinity in test cases since we can't use new literals for those values yet until the dependent tools.reader is updated to 1.1.0?

Comment by David Nolen [ 24/Sep/17 4:40 AM ]

fixed https://github.com/clojure/clojurescript/commit/89914d2ead964122f99e638edda0cd96d330cb66





[CLJS-2339] Significant code reload slowdown with :npm-deps Created: 29/Aug/17  Updated: 24/Sep/17  Resolved: 24/Sep/17

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

Type: Enhancement Priority: Minor
Reporter: Petter Eriksson Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: performance

Attachments: Text File CLJS-2339-2.patch     Text File CLJS-2339.patch    
Patch: Code
Approval: Accepted

 Description   

After migrating our dependencies from cljsjs to node_modules we noticed that figwheel took a lot longer to reload our code.

I asked in #clojurescript if anyone had an idea of what could be done and @anmonteiro wrote:
@petterik might just be because we're processing all your node dependencies through Closure on every reload feel free to open a JIRA ticket, we could probably employ a caching strategy somewhere

Versions used:
clojurescript "1.9.908"
lein-figwheel "0.5.13"

npm-depm deps:
:react-helmet "5.1.3"
:react "15.6.1"
:react-dom "15.6.1"
as well as some devDependencies in package.json.



 Comments   
Comment by Petter Eriksson [ 29/Aug/17 1:23 PM ]

If needed to repro, here are the additional devDependencies:
"phantomjs": "1.9.19",
"foundation-cli": "2.2.3",
"bower": "1.8.0"

Comment by David Nolen [ 15/Sep/17 3:32 PM ]

This ticket will need more information. It might just be a Figwheel issue.

Comment by Tony Kay [ 17/Sep/17 9:58 PM ]

OK, so I have some additional interesting facts. It does compile and work (where "work" is defined as "renders a thing from the npm module").

cljs: 1.9.908
cljsbuild: 1.1.7 via lein
Heap size: 2g
npm-deps: react, react-dom, and @blueprintjs/core
cljs deps: Om Next
verbose: true

Project is an om-next app, so cljsjs.react is in the classpath

Building the application without the npm deps and no code that refers to them: 46 seconds (CPU time)
Adding the NPM deps (with install true, but no code that uses them) increases this just be the amount of the install: 59 seconds
using the npm deps increases compile time to: 3 minutes 50 seconds

Critically: with verbose on, I can see that om/next.cljs takes about 15s in the first two cases, and 3 minutes in the final one. In other words: the thing that is slow is compiling next.cljc. Nothing else gets slower.

I'm not sure if this is even going to work "right" when it does compile, since I'm not sure how the cljsjs.react and npm react can co-exist (this is where I assume Om Next probably just needs to be made to use npm instead of cljsjs?)

But, since I saw that Petter was pulling in similar deps, he might be hitting a similar problem with cljsjs.react and npm react both being "in scope" so to speak.

When I use it from figwheel, the time between reloads becomes unusable. I assume it is related, but I have no data to back that up.

EDIT: I had missed the new doc on :global-exports. I'm going to try that and add my results.

Comment by Tony Kay [ 17/Sep/17 10:51 PM ]

So, I fixed the build to be "correct" with `:global-exports so that I only have the NPM version of react and react-dom around (excluded cljsjs/react and react-dom from Om Next). The compile time for next.cljc is still about 3 minutes (compared to the "normal" 15s)

BUT, I then removed blueprint from my `:npm-deps` (and usage), but kept react, react-dom, and a use of react (the NPM one) The time to compile next.cljc dropped to about a minute! Still 4x slower than "normal", but 4x faster than with blueprint in the npm deps. It seems that a file using an npm dep see a significant slowdown that is somehow proportional to the amount of total npm deps code "in view".

Obviously, Om Next uses React, but not blueprint. Yet, it's compile time is significantly affected.

What Antonio said about processing them all through Closure certainly sounds relevant. Kind of a let down to go from recent speed-ups in compiler speed to this sudden jolt of a performance hit when we finally get a better interface to libraries

Comment by António Nuno Monteiro [ 17/Sep/17 11:35 PM ]

Patch attached.

Comment by Tony Kay [ 17/Sep/17 11:48 PM ]

That patch fixes the build issue on plain cljsbuild.

Figwheel now starts quickly (first complete compile), but a simple file change on a small project takes 12s to hot code reload, making it pretty unusable.

Comment by António Nuno Monteiro [ 18/Sep/17 12:01 AM ]

So it looks like this is a 2-part problem.

The first one, which my patch solved, is related to CLJS compilation (where we were performing unnecessary computations on every symbol analysis – which slowed down proportionally to the number of JS modules processed).

The second problem is that we process every JS module on every code reload: the solution for this one is implementing a strategy for figuring out if we need to process JS modules again (e.g. based on last modified timestamps of source files, just like `cljs.compiler/requires-compilation?`).

Comment by António Nuno Monteiro [ 18/Sep/17 10:42 PM ]

The attached CLJS-2339-2.patch contains the changes in the previous patch and also solves the issues around reloading, only running the foreign libraries that are modules through Closure if any of the source files in the dependency graph have changed.

I'd appreciate if people who are seeing the issue can try out the patch and report back.

Comment by Tony Kay [ 19/Sep/17 11:01 PM ]

So, I did some profiling with visualvm, and gave the results to Antonio. They were showing the vast majority of the time being consumed by `pipe`, under the direction of the node module discovery functions. His new version of the second patch definitely improves reload speed considerably. My hot code reload went from about 14 seconds (via patch 1) to 2 seconds with the new version of patch 2. This is on a project with Om Next, and some largish node libraries.

Comment by David Nolen [ 24/Sep/17 4:36 AM ]

fixed https://github.com/clojure/clojurescript/commit/245bdee2c35e19a9685b7a0849f26fce8bdaf7ca





[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-2260] Convert :constant AST node to tools.analyzer format Created: 18/Jul/17  Updated: 23/Sep/17

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

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

Attachments: Text File const-ast-2.patch     Text File const-ast.patch    
Patch: Code

 Description   

Part of https://dev.clojure.org/jira/browse/CLJS-1461

Work on :const node:

  • rename :constant op to :const
  • add :val entry


 Comments   
Comment by David Nolen [ 15/Sep/17 3:24 PM ]

This patch needs a rebase.

Comment by Amanda Walker [ 23/Sep/17 8:13 PM ]

Here is the rebased patch.





[CLJS-2298] REPLs should automatically load user.(cljs|cljc) files at root of Java classpath Created: 04/Aug/17  Updated: 22/Sep/17

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Sameer Rahmani
Resolution: Unresolved Votes: 0
Labels: repl

Approval: Vetted




[CLJS-2344] de-dupe externs Created: 31/Aug/17  Updated: 21/Sep/17

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: 0
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.






[CLJS-2365] Self-host: Unable to reload namespace while in it Created: 18/Sep/17  Updated: 20/Sep/17

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

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


 Description   

There is an unreleased self-host regression where requiring a namespace while in that namespace triggers circular dependency detection.

As a concrete example, let's say you are in a REPL, and you require a namespace, go into that namespace (using in-ns), exercise it a little, and then change the code to fix something and then reload it. This now fails on master for self-hosted code.

A repro following this example is the following:

(require 'cljs.js)

(def st (cljs.js/empty-state))

(cljs.js/eval st
  '(require (quote foo.core))
  {:context :expr
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns foo.core) (def x 1)"}))}
  prn)

(cljs.js/eval st
  '(require (quote foo.core) :reload)
  {:context :expr
   :ns 'foo.core
   :eval cljs.js/js-eval
   :load (fn [_ cb]
           (cb {:lang :clj
                :source "(ns foo.core) (def x 2)"}))}
  prn)

This causes the following on master, but not with the 1.9.908 release:

{:error #error {:message "Circular dependency detected foo.core -> foo.core", :data {:tag :cljs/analysis-error}}}

(Strictly speaking, the above example is not minimal in that :reload is not required in order to reproduce it: It will happen if you simply attempt to require the namespace while "in" it.)



 Comments   
Comment by Mike Fikes [ 18/Sep/17 10:19 AM ]

Git bisect result implicates CLJS-2356:

238028ccc51afe45de98fb853be4396a71afb602 is the first bad commit
commit 238028ccc51afe45de98fb853be4396a71afb602
Author: Antonio Nuno Monteiro <anmonteiro@gmail.com>
Date:   Sun Sep 10 21:24:22 2017 -0700

    CLJS-2356: Self-host: circular dependency detection is not correct

:040000 040000 a93d466e3f1ea042e730fb78ca911f92319880d0 29108cab4b45247e641d430d85dda85c436e95fe M	src
Comment by Mike Fikes [ 18/Sep/17 3:22 PM ]

Here is the result from some analysis by António and me: There are a few places in the self-host compiler code which make use of :def-emits-var to deduce that self-host is being used to implement a REPL. In particular, the code being exercised in the ticket description works if you include :def-emits-var true in the options passed. But, if you are evaluating a "load" form, such as require and others, if :def-emits-var is set, then it can be seen that code inside the loaded namespaces gets compiled with def (and derived) forms producing Vars. (This can be seen if you make use of the self-hosted caching option.) Perhaps this is incorrect behavior (I believe it doesn't occur with JVM ClojureScript, for example). With the current behavior, if a REPL chooses to not set :def-emits-var true for "load" forms, then you encounter the issue described in the ticket description. So, in short, the underlying issue in this ticket could either be fixed or deemed OK, and the other issue mentioned here regarding def forms inside required namespaces could be split off as a separate ticket that could be pursued on its own.

Comment by Mike Fikes [ 20/Sep/17 1:10 PM ]

With CLJS-2367, I suspect this ticket becomes a non-issue, because REPLs can unconditionally (always) set :def-emits-var.





[CLJS-2356] Self-host: circular dependency detection is not correct Created: 10/Sep/17  Updated: 18/Sep/17  Resolved: 11/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-2356.patch    
Patch: Code and Test
Approval: Accepted

 Description   

Patch should be applied after CLJS-2354



 Comments   
Comment by David Nolen [ 11/Sep/17 6:36 AM ]

fixed https://github.com/clojure/clojurescript/commit/238028ccc51afe45de98fb853be4396a71afb602

Comment by Mike Fikes [ 18/Sep/17 10:20 AM ]

See regression in CLJS-2365.





[CLJS-2362] Make node REPL debuggable/inspectable again Created: 15/Sep/17  Updated: 17/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: Andrea Richiardi
Resolution: Unresolved Votes: 0
Labels: nodejs

Attachments: File cljs-2362.diff    
Approval: Screened

 Description   

Currently, when trying to enable debug mode a warning is thrown:

Clojure 1.9.0-alpha17
user=> (require '[cljs.repl :as repl])
nil
user=> (require '[cljs.repl.node :as node])         
nil
user=> (repl/repl (node/repl-env :debug-port 5002))
(node:2152) [DEP0062] DeprecationWarning: `node --debug` and `node --debug-brk` are invalid. Please use `node --inspect` or `node --inspect-brk` instead.

I can take care of this easy patch.



 Comments   
Comment by Andrea Richiardi [ 15/Sep/17 12:24 AM ]

Patch attached, a bit rusty , hopefully all good

Comment by David Nolen [ 15/Sep/17 7:36 AM ]

Is this going to work for older version of Node?

Comment by Andrea Richiardi [ 15/Sep/17 1:46 PM ]

Should work for node >= 6.3, do you want me to branch on the version?

Comment by Thomas Heller [ 15/Sep/17 2:42 PM ]

Given that CLJS itself does not use this info and just passes it along it might be a better solution to provide a generic option that takes a seq of strings to use when launching the node process.

In shadow-cljs I have

(shadow/node-repl {:node-args ["--inspect" "--inspect-brk" "--whatever-you-need"]})

This is more flexible and doesn't have to deal with versioning issues since it is left to the user.

Comment by Andrea Richiardi [ 17/Sep/17 1:59 PM ]

I like Thomas' approach way better, branching on the version would need to execute node version,not really pretty.

Many boot tasks have a :options key for that (converting {:key} to --key). Thomas version is even simpler, the only difficulty would be to avoid breaking :debug-port as it is now.





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

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

Type: Enhancement Priority: Minor
Reporter: Christian Fortin 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.






[CLJS-2289] Node packages with browser entry don't work with Closure Created: 31/Jul/17  Updated: 15/Sep/17  Resolved: 15/Sep/17

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

Type: Enhancement Priority: Major
Reporter: Juho Teperi Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: js-modules, npm-deps

Approval: Vetted

 Description   

Closure doesn't yet support package.json browser field: https://github.com/google/closure-compiler/issues/2433

However, our, module_deps.js / brower-resolve, and use this field.

One package that has problems is https://github.com/PaulLeCam/react-leaflet which depends on https://github.com/BerkeleyTrue/warning which uses browser entry.
index-node-modules-dir reuturns a list of all JS files in node_modules, this includes both warning/warning.js and warning/browser.js. index-node-modules only includes warning/browser.js. Because Closure doesn't understand package.json browser field, it can't resolve the warning require when processing React-leaflet: https://github.com/PaulLeCam/react-leaflet/blob/master/lib/Pane.js#L47

(cljs.closure/index-node-modules-dir)
;; =>
{:file "/home/juho/Source/x/y/node_modules/warning/warning.js", :module-type :es6, :provides ["warning/warning.js" "warning/warning" "warning"]}
{:file "/home/juho/Source/x/y/node_modules/warning/browser.js", :module-type :es6, :provides ["warning/browser.js" "warning/browser"]}
(cljs.closure/index-node-modules ["react-leaflet"])
;; =>
{:file "/home/juho/Source/x/y/node_modules/warning/browser.js", :module-type :es6, :provides ["warning" "warning/browser.js" "warning/browser"]}


 Comments   
Comment by António Nuno Monteiro [ 03/Aug/17 5:31 PM ]

This is now fixed in Closure Compiler master (https://github.com/google/closure-compiler/pull/2598).





[CLJS-2338] Add support for renamePrefix and renamePrefixNamespace closure compiler options Created: 29/Aug/17  Updated: 15/Sep/17  Resolved: 15/Sep/17

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

Type: Enhancement Priority: Major
Reporter: Pieter du Toit Assignee: Pieter du Toit
Resolution: Completed Votes: 1
Labels: closure

Attachments: Text File CLJS-2338.patch     Text File CLJS-2338.patch    
Approval: Accepted

 Description   

Adding support for these closure compiler options would allow for a mechanism to avoid name collisions



 Comments   
Comment by Pieter du Toit [ 29/Aug/17 6:01 AM ]

Patch attached

Comment by David Nolen [ 29/Aug/17 6:35 AM ]

Pieter, thanks for the patch. Have you submitted your contributor agreement?

Comment by David Nolen [ 29/Aug/17 6:37 AM ]

One question I have about the patch, is there a default prefix name if not supplied by the user?

Comment by Pieter du Toit [ 29/Aug/17 7:05 AM ]

Yes, I have submitted my contributor agreement. The patch does not specify defaults, if the user does not supply :rename-prefix or :rename-prefix-namespace, then the compiler options passed to the closure compiler would be unchanged.

Comment by David Nolen [ 29/Aug/17 8:05 AM ]

Pieter sorry for being unclear, what happens if :rename-prefix-namespace is specified but :rename-prefix is not specified?

Comment by Pieter du Toit [ 29/Aug/17 9:40 AM ]

AFAICT, the options can be specified independently, the only documentation I could find on it is in Closure CommandLineRunner and here.

For my own use case, I only need to set :rename-prefix, which I have tested both with and without :modules and works as expected.

I did some further testing now with only specifying :rename-prefix-namespace (which calls setRenamePrefixNamespace), it has no impact on a build without :modules, and results in an error "{prefix} is not defined" when using with :modules. Specifying both :rename-prefix and :rename-prefix-namespace for a build with :modules results in the same "{prefix} is not defined" error.

I am less confident in :rename-prefix-namespace so I could modify the patch to remove the option ? I also noticed that I would need to modify the patch to add any new options to known-opts

Comment by Thomas Heller [ 29/Aug/17 11:26 AM ]

FWIW I'm using setRenamePrefixNamespace for a feature in shadow-cljs [1]. The effect is that only one global variable will be created and everything else will be a property of that variable. I'm using $CLJS as the prefix namespace and where :advanced mode would otherwise create a xY variable (just an example, name doesn't matter), it would now create a $CLJS.xY property instead.

I have never used :rename-prefix but my understanding is that if would just turn xY into $CLJSxY so you'd still end up with a bunch of global variables.

In my case I use it combined with :modules and only setRenamePrefixNamespace needs to be set and is otherwise unrelated to :rename-prefix.

[1] https://github.com/thheller/shadow-cljs/blob/21e9a3a279ed7a34239cc3c7cc65715f22289163/src/main/shadow/cljs/closure.clj#L481

Comment by David Nolen [ 30/Aug/17 6:21 AM ]

Ok so they sound like independent options then. Pieter, I'm fine with the patch granted it's updated to augment known-opts.

Comment by Pieter du Toit [ 30/Aug/17 9:01 AM ]

David, thanks, I have attached the updated patch

Comment by Pieter du Toit [ 30/Aug/17 9:17 AM ]

Thomas, thanks, I get the same result (no prefix namespace) with a shadow-cljs release build as with cljs (when using the new :rename-prefix-namespace and no :modules)

shadow-cljs.edn
{:dependencies
 []

 :source-paths
 ["src"]

 :builds
 {:app
  {:target :browser
   :output-dir "shadow/js"
   :modules
   {:main {:entries [moduleb]}}}}}
moduleb.cljs
(ns moduleb)

(def env "dev")

(defn init []
  (js/console.log (str "Module B init for env: " env)))

(init)
Comment by Thomas Heller [ 31/Aug/17 3:45 AM ]

The option itself is not supported as of now as I'm using the default cljs.closure/set-options function in shadow-cljs. When it is added to CLJS it will become available in shadow-cljs.

I'm currently using the feature internally for the :npm-module target which compiles code so it can be directly consumed by JS tools (or node). I'm setting that manually though so the :rename-prefix-namespace option is not recognized.

Comment by David Nolen [ 15/Sep/17 3:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/7a8803ef70cb84c686341353e7ab29928487e388





[CLJS-2355] Fix tests after CLJS-2349 Created: 10/Sep/17  Updated: 15/Sep/17  Resolved: 15/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: test

Attachments: Text File CLJS-2355.patch    
Patch: Code and Test
Approval: Accepted

 Description   

warn-on-reflection doesn't exist in CLJS



 Comments   
Comment by David Nolen [ 15/Sep/17 3:29 PM ]

fixed https://github.com/clojure/clojurescript/commit/fc940017c93a14748e0d93612ddf626fcb0aaae6





[CLJS-2360] ClojureScript ignores first two arguments passed to a macro when using vargs Created: 13/Sep/17  Updated: 15/Sep/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)






[CLJS-2361] Self-host: circular dependency detection doesn't handle REPL self-require Created: 13/Sep/17  Updated: 15/Sep/17  Resolved: 15/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-2361.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 15/Sep/17 3:22 PM ]

fixed https://github.com/clojure/clojurescript/commit/3c0df9c6e712ea1cb2e8a469b5ceaf0dff17d458





[CLJS-2351] Setting :arglists metadata when vararg is present Created: 08/Sep/17  Updated: 15/Sep/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.


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.






[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-1461] Convert analyzer to conform to tools.analyzer's spec Created: 28/Sep/15  Updated: 13/Sep/17

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

Type: Enhancement Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Ambrose Bonnaire-Sergeant
Resolution: Unresolved Votes: 6
Labels: None


 Description   

tools.analyzer has a handy common AST format for map-based analysis results. We should use this format in the ClojureScript analyzer so tooling that already accepts this format can work seamlessly with ClojureScript.

Work in progress: https://github.com/frenchy64/clojurescript/pull/7

Order of work:

  1. Patch 1 ready for review
    • :const
      • rename :constant op to :const
      • add :val entry
  2. :def
    • rename :var-ast entry to :var I misunderstood :var-ast, :var entry is already there and perfectly fine.
    • add :ns entry
  3. :the-var
    • rename :var-special op to :the-var
  4. :throw
    • rename :throw entry to :exception
  5. :try
    • rename :try entry to :body
  6. :letfn
    • rename :expr entry to :body
  7. :let/:loop
    • rename :expr entry to :body
  8. :quote
    • add :quote op
  9. :deftype
    • rename :deftype* op to :deftype
  10. :defrecord
    • rename :defrecord* op to :defrecord
  11. :host-field/:host-method
    • split :dot op into :host-field/:host-method
  12. :invoke
    • rename :f to :fn
  13. :js-object/:js-array
    • split :js-value op into :js-object/:js-array
  14. :with-meta
    • rename :meta op to :with-meta
  15. :var/:binding/:local/:js-var
    • split :var op into :var/:binding/:local/:js-var
    • emit :local op if :js-shadowed-by-local
    • change :local to be #{:fn :letfn :let :arg ...}
  16. :fn-method
    • add :fn-method op to :methods of a :fn
    • :variadic -> :variadic?
    • :expr -> :body
    • add :fixed-arity
  17. :children
    • move to tools.analyzer :children format
      • :children are a sequence of keyword keys
      • ensure all sequence children are vectors
    • replace :children calls with a compatible function from AST -> children
  18. Unit tests
    • add them all at the end
  19. AST format documentation
    • modify from tools.analyzer's

Extra stuff:

  • argument validation in 'var parsing
  • :max-fixed-arity -> :fixed-arity
  • :case node overhaul
    • which format do we follow?
    • TAJ vs TAJS


 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 13/Sep/17 6:56 AM ]

In the body of this issue I have proposed a sequence of potential patches such that each patch will be easily screenable.

Can I have some feedback on this? Should some of these steps be combined/split?





[CLJS-2358] two (inc) calls give different result (1 and NaN) even though argument evaluates to the same value (nil) Created: 11/Sep/17  Updated: 11/Sep/17  Resolved: 11/Sep/17

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

Type: Defect Priority: Major
Reporter: Markus Bertheau Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Chromium Version 60.0.3112.113 (Developer Build) Built on Ubuntu , running on Ubuntu 16.04 (64-bit)



 Description   

cljs.user=> (apply max [])
nil
cljs.user=> (inc nil)
1
cljs.user=> (inc (apply max []))
NaN

I expect (inc (apply max [])) to evaluate to 1, since (inc nil) evaluates to 1 and (apply max []) evaluates to nil.



 Comments   
Comment by David Nolen [ 11/Sep/17 1:46 PM ]

This is not a bug. (apply max []) in Clojure throws because that's an invalid arity.





[CLJS-2357] Self-host: run all async tests Created: 10/Sep/17  Updated: 11/Sep/17  Resolved: 11/Sep/17

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

Type: Enhancement Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap, test

Attachments: Text File CLJS-2357.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 11/Sep/17 6:37 AM ]

fixed https://github.com/clojure/clojurescript/commit/33ce3d5c1c5d9fd4c38fbc6798a8d7085338eb4d





[CLJS-2354] Self-host: `compile-str` doesn't handle `clojure` -> `cljs` aliasing Created: 10/Sep/17  Updated: 11/Sep/17  Resolved: 11/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: bootstrap

Attachments: Text File CLJS-2354.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 11/Sep/17 6:35 AM ]

fixed https://github.com/clojure/clojurescript/commit/4b5fd54dc3081830adaddef206e6b80e0ed83df9





[CLJS-2353] use portable `node-module-dep?` function in analyze-deps Created: 10/Sep/17  Updated: 11/Sep/17  Resolved: 11/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: npm-deps

Attachments: Text File CLJS-2353.patch    
Patch: Code
Approval: Accepted

 Comments   
Comment by David Nolen [ 11/Sep/17 6:33 AM ]

fixed https://github.com/clojure/clojurescript/commit/fe28810661256907f1402ac1c7c8556db1cf616a





[CLJS-2345] File paths emitted as args to cljs.core.load_file without escaping fails on Windows Created: 31/Aug/17  Updated: 09/Sep/17  Resolved: 09/Sep/17

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: David Nolen
Resolution: Completed Votes: 1
Labels: windows

Attachments: Text File CLJS-2345.patch    
Patch: Code
Approval: Accepted

 Description   

(Context: https://github.com/funcool/promesa/issues/52)

The compiler currently doesn't escape file paths when emitting calls to cljs.core.load_file. This is bad in cases where a library has a JS dependency with a path segment whose first character corresponds to an ASCII control code (a, b, t, n, v, f, r, e); this can result in a control code being where a backslash and letter should be, e.g.:

// Compiled by ClojureScript 1.9.908 {:target :nodejs}
goog.provide('promesa.impl');
goog.require('cljs.core');
goog.require('promesa.protocols');
cljs.core.load_file(".cljs_node_repl\bluebird\bluebird.js");

(Note the unescaped backslashes, and thus the backspace char.)

Patch incoming.



 Comments   
Comment by David Nolen [ 09/Sep/17 9:46 AM ]

fixed https://github.com/clojure/clojurescript/commit/b74538b4e9947973175cf197704c18b10a5ce6bf





[CLJS-2350] Fix circular dependencies test Created: 08/Sep/17  Updated: 09/Sep/17  Resolved: 09/Sep/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: test

Attachments: Text File CLJS-2350.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 09/Sep/17 9:28 AM ]

fixed https://github.com/clojure/clojurescript/commit/f88fa62ae7b0cf403a5f356ac20be0466aa11a73





[CLJS-2349] Port reset-vals! and swap-vals! over from Clojure Created: 07/Sep/17  Updated: 09/Sep/17  Resolved: 09/Sep/17

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

Type: Enhancement Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: core

Attachments: Text File CLJS-2349.patch    
Patch: Code and Test
Approval: Accepted

 Description   

same as https://github.com/clojure/clojure/commit/9fdbd8cd56524911d120e4631cc53c572ebdd33d



 Comments   
Comment by David Nolen [ 09/Sep/17 9:15 AM ]

fixed https://github.com/clojure/clojurescript/commit/35a360943ffade5fc319203512010458c054e241





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

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

Type: Defect Priority: Major
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-150] Regular expressions don't support Javascript mode flags Created: 16/Feb/12  Updated: 08/Sep/17  Due: 24/Feb/12  Resolved: 08/Sep/17

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

Type: Defect Priority: Minor
Reporter: Bobby Calderwood Assignee: Bobby Calderwood
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently, the compiler and cljs.core allow for Java mode flags. Javascript doesn't support many of these, and supports one flag not supported by Java - 'g'.

ClojureScript regular expressions should support only Javascript regex mode flags: 'i', 'm', and 'g'. This applies to Regex literals in the compiler as well as (re-pattern).

This is a defect in the implementation of CLJS-116.



 Comments   
Comment by David Nolen [ 16/Feb/12 3:33 PM ]

The defect existed prior to CLJS-116. The problem is that we're using the Clojure reader and g is not a valid flag for a Java RegexPattern.

Comment by Francis Avila [ 28/Feb/14 1:04 AM ]

This ticket should be rejected. A regular expression created with the global flag is stateful (i.e., the lastIndex property is checked and used by the exec and test methods.) On sufficiently old browsers (pre js 1.5), this makes the RegExp object itself stateful, i.e., not instances, but the RegExp constructor is mutated!

Using a regex with the global flag set will already ruin the results of re-seq, re-find, etc. I could see re-seq using a clone of the input regex with the global flag set as an optimization to avoid string slicing, but we certainly shouldn't provide a public interface to create them.

See also CLJS-776

Comment by Luke [ 28/Aug/17 1:51 AM ]

This ticket should be rejected.
Javascript obfuscator https://javascript-obfuscator.org/





[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: 0
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-2348] Node ns require doesn't create top-level object Created: 04/Sep/17  Updated: 04/Sep/17  Resolved: 04/Sep/17

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

Type: Defect Priority: Major
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: nodejs


 Description   

Some node packages export an object directly which in turn can not be instantiated.

(ns demo.core (:require [web3 :as Web3]))

(new Web3)

ReferenceError: Web3 is not defined



 Comments   
Comment by Leon Grapenthin [ 04/Sep/17 3:53 PM ]

misreport





[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: 0
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-2343] Double require guard bypassed if :refer-macros Created: 30/Aug/17  Updated: 30/Aug/17

Status: Open
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3196, 1.9.908
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errors

Approval: Vetted

 Description   

If you have this code,

(ns foo.core
  (:require [cljs.test])
  (:require [clojure.string]))

it will trigger the "Only one :require form is allowed per namespace definition" diagnostic.

But this diagnostic is bypassed if you use inline macro spec sugar:

(ns foo.core
  (:require [cljs.test :refer-macros [deftest]])
  (:require [clojure.string]))

This causes the compiler to derail with this:

clojure.lang.ExceptionInfo: Only :as, :refer and :rename options supported in :require / :require-macros; offending spec: [cljs.test :refer-macros [deftest]] at line 1 <cljs repl> {:file "<cljs repl>", :line 1, :column 1, :root-source-info {:source-type :fragment, :source-form (ns foo.core (:require [cljs.test :refer-macros [deftest]]) (:require [clojure.string]))}, :tag :cljs/analysis-error}
	at clojure.core$ex_info.invokeStatic(core.clj:4617)
	at cljs.analyzer$error.invokeStatic(analyzer.cljc:693)
	at cljs.analyzer$error.invoke(analyzer.cljc:693)
	at cljs.analyzer$error.invokeStatic(analyzer.cljc:695)
	at cljs.analyzer$basic_validate_ns_spec.invokeStatic(analyzer.cljc:2256)
	at cljs.analyzer$parse_require_spec.invokeStatic(analyzer.cljc:2344)
	at cljs.analyzer$parse_require_spec.invoke(analyzer.cljc:2343)
	at clojure.lang.AFn.applyToHelper(AFn.java:171)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.core$apply.invokeStatic(core.clj:652)
	at clojure.core$partial$fn__4765.doInvoke(core.clj:2534)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$map$fn__4785.invoke(core.clj:2644)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.Cons.next(Cons.java:39)
	at clojure.lang.RT.boundedLength(RT.java:1749)
	at clojure.lang.RestFn.applyTo(RestFn.java:130)
	at clojure.core$apply.invokeStatic(core.clj:650)
	at cljs.analyzer$fn__2052$fn__2062.invoke(analyzer.cljc:2622)
	at clojure.core.protocols$fn__6755.invokeStatic(protocols.clj:167)
	at clojure.core.protocols$fn__6755.invoke(protocols.clj:124)
	at clojure.core.protocols$fn__6710$G__6705__6719.invoke(protocols.clj:19)
	at clojure.core.protocols$seq_reduce.invokeStatic(protocols.clj:31)
	at clojure.core.protocols$fn__6738.invokeStatic(protocols.clj:75)
	at clojure.core.protocols$fn__6738.invoke(protocols.clj:75)
	at clojure.core.protocols$fn__6684$G__6679__6697.invoke(protocols.clj:13)
	at clojure.core$reduce.invokeStatic(core.clj:6545)
	at cljs.analyzer$fn__2052.invokeStatic(analyzer.cljc:2575)
	at cljs.analyzer$fn__2052.invoke(analyzer.cljc:2556)
	at clojure.lang.MultiFn.invoke(MultiFn.java:251)
	at cljs.analyzer$analyze_seq_STAR_.invokeStatic(analyzer.cljc:3333)
	at cljs.analyzer$analyze_seq_STAR__wrap.invokeStatic(analyzer.cljc:3336)
	at cljs.analyzer$analyze_seq.invokeStatic(analyzer.cljc:3361)
	at cljs.analyzer$analyze_form.invokeStatic(analyzer.cljc:3530)
	at cljs.analyzer$analyze_STAR_.invokeStatic(analyzer.cljc:3577)
	at cljs.analyzer$analyze.invokeStatic(analyzer.cljc:3603)
	at cljs.repl$evaluate_form$__GT_ast__6169.invoke(repl.cljc:471)
	at cljs.repl$evaluate_form.invokeStatic(repl.cljc:472)
	at cljs.repl$eval_cljs.invokeStatic(repl.cljc:631)
	at cljs.repl$eval_cljs.invoke(repl.cljc:618)
	at cljs.repl$repl_STAR_$read_eval_print__6300.invoke(repl.cljc:880)
	at cljs.repl$repl_STAR_$fn__6306$fn__6315.invoke(repl.cljc:922)
	at cljs.repl$repl_STAR_$fn__6306.invoke(repl.cljc:921)
	at cljs.compiler$with_core_cljs.invokeStatic(compiler.cljc:1252)
	at cljs.repl$repl_STAR_.invokeStatic(repl.cljc:882)
	at cljs.repl$repl.invokeStatic(repl.cljc:1001)
	at cljs.repl$repl.doInvoke(repl.cljc:933)
	at clojure.lang.RestFn.invoke(RestFn.java:410)
	at cljs.repl.node$_main.invokeStatic(node.clj:234)
	at cljs.repl.node$_main.invoke(node.clj:233)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.core$apply.invokeStatic(core.clj:646)
	at clojure.main$main_opt.invokeStatic(main.clj:314)
	at clojure.main$main_opt.invoke(main.clj:310)
	at clojure.main$main.invokeStatic(main.clj:421)
	at clojure.main$main.doInvoke(main.clj:384)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)





[CLJS-2337] Missing renamePrefixNamespace compiler option Created: 28/Aug/17  Updated: 29/Aug/17  Resolved: 29/Aug/17

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

Type: Enhancement Priority: Major
Reporter: Xiaomin Wu Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: compiler
Environment:

any



 Description   

right now if we use the ":modules" option with ":optimization :advanced" flag, compile will expose a lot of global functions/vars. From documentation, if we have "renamePrefixNamespace", compile will put all the globale function/vars under the "renamePrefixNamespace". However Clojure Script compiler is missing this flag.



 Comments   
Comment by David Nolen [ 29/Aug/17 6:37 AM ]

dupes CLJS-2338 which has a patch.





[CLJS-2336] Call alength once in areduce and amap Created: 26/Aug/17  Updated: 27/Aug/17  Resolved: 27/Aug/17

Status: Closed
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: Completed Votes: 0
Labels: performance

Attachments: Text File CLJS-2336.patch    
Patch: Code
Approval: Accepted

 Description   

Make a single call to alength in areduce and amap, essentially porting relevant aspects of CLJ-1765 and CLJ-1901 to ClojureScript.



 Comments   
Comment by Mike Fikes [ 26/Aug/17 5:32 PM ]
Before:

Benchmarking with V8
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 63 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 83 msecs

Benchmarking with SpiderMonkey
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 26 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 796 msecs

Benchmarking with JavaScriptCore
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 92 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 98 msecs

Benchmarking with Nashorn
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 1258 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 1433 msecs

Benchmarking with ChakraCore
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 9 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 67 msecs


After:

Benchmarking with V8
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 56 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 79 msecs

Benchmarking with SpiderMonkey
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 23 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 787 msecs

Benchmarking with JavaScriptCore
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 93 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 104 msecs

Benchmarking with Nashorn
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 991 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 1252 msecs

Benchmarking with ChakraCore
[arr (to-array (range 1000000))], (reset! x (areduce arr i ret 0 (+ ret (aget arr i)))), 1 runs, 9 msecs
[arr (to-array (range 1000000))], (amap arr i ret (* 10 (aget arr i))), 1 runs, 71 msecs
Comment by David Nolen [ 27/Aug/17 4:55 PM ]

fixed https://github.com/clojure/clojurescript/commit/998933f5090254611b46a2b86626fb17cabc994a





[CLJS-2335] Avoid alength on strings Created: 26/Aug/17  Updated: 26/Aug/17  Resolved: 26/Aug/17

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

Type: Defect Priority: Minor
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2335.patch    
Patch: Code
Approval: Accepted

 Description   

There are a handful of places where alength is called (improperly) on string instances. While preserving identical JavaScript codegen .-length interop could be used instead.



 Comments   
Comment by David Nolen [ 26/Aug/17 6:14 PM ]

fixed https://github.com/clojure/clojurescript/commit/bee66e064aca8fd9edb021cabf686c42bc8b6c31





[CLJS-2334] Also gather dependencies from foreign-lib modules Created: 22/Aug/17  Updated: 25/Aug/17  Resolved: 25/Aug/17

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

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

Attachments: Text File CLJS-2334.patch    
Patch: Code and Test

 Description   

Currently dependencies of foreign libs passed as modules are not gathered and processed in Closure.

It would be nice to make npm-deps and foreign libs passed as modules just work.

(b/build "src"
  {:output-dir "out"
   :main 'foo.core
   :output-to "out/main.js"
   :asset-path "/out"
   :foreign-libs [{:file "src/foreign/lib.js"
                   :module-type :es6
                   :provides ["js.lib"]}]
   :npm-deps {:react "15.6.1"
              :react-dom "15.6.1"}
   :install-deps true
   :optimizations :none
   :compiler-stats true
   :verbose true})
import React from 'react';

export var main = function() {
    console.log("This is my JSX component: " + React.DOM.div(null, 'hi'));
};


 Comments   
Comment by António Nuno Monteiro [ 23/Aug/17 12:42 PM ]

Patch attached with fix.

Comment by David Nolen [ 25/Aug/17 2:45 PM ]

fixed https://github.com/clojure/clojurescript/commit/79041d10ce11e9e2f15c261a9a4174c6a7066834





[CLJS-2333] module-deps.js doesn't correctly compute `main` if aliased in browser field Created: 21/Aug/17  Updated: 25/Aug/17  Resolved: 25/Aug/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2333.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 25/Aug/17 2:44 PM ]

fixed https://github.com/clojure/clojurescript/commit/88e1f39d5653f154da6b6362bced3c3cb5c15e3b





[CLJS-2155] building fileUrl on windows in repl.cljc results with java.net.UnknownHostException Created: 03/Jul/17  Updated: 19/Aug/17

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

Type: Defect Priority: Major
Reporter: Vojimir Golem Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Windows OS



 Description   

I think that the following line might cause the problem on windows:
https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/repl.cljc#L713

For file param e.g. "src\duct3\client.cljs"

(str "file://" (.getAbsolutePath file))

evaluates on windows as:
"file://C:\Projects\Playground\duct3\src\duct3\client.cljs"

which is not legal file Url (https://en.wikipedia.org/wiki/File_URI_scheme#Windows)

and final result is: java.net.UnknownHostException (java treat that URL as FTP address).



 Comments   
Comment by Vojimir Golem [ 19/Aug/17 10:02 AM ]

Permalink to mentioned line:
https://github.com/clojure/clojurescript/blob/98656d305e6447c62e36849ee615532d53211fdd/src/main/clojure/cljs/repl.cljc#L736





[CLJS-2332] module_deps.js doesn't process `export from` correctly Created: 18/Aug/17  Updated: 19/Aug/17  Resolved: 19/Aug/17

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

Type: Defect Priority: Major
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2332.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 19/Aug/17 5:42 AM ]

fixed https://github.com/clojure/clojurescript/commit/98656d305e6447c62e36849ee615532d53211fdd





[CLJS-2330] Don't set `"browser"` field for Closure if target is :nodejs Created: 18/Aug/17  Updated: 18/Aug/17  Resolved: 18/Aug/17

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

Type: Enhancement Priority: Minor
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2330.patch    
Patch: Code
Approval: Accepted

 Comments   
Comment by David Nolen [ 18/Aug/17 1:32 PM ]

fixed https://github.com/clojure/clojurescript/commit/adc7deaafc54f6f7d27cc631db4e381d222a818c





[CLJS-2331] Extend :global-exports to auto-alias and rewrite occurrences of declared globals Created: 18/Aug/17  Updated: 18/Aug/17

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: foreign-libs, global-exports

Approval: Accepted

 Description   

In order to lower the barrier to adopting `:npm-deps` we could push `:global-exports` a bit further. Instead of just using it to support foreign-libs, we can also use it to automatically make libraries node_modules compatible. This can be done by auto generating a namespace alias if not provided and rewriting global access for matching symbols. Some libs may refer to globals without explicit requires and we should warn in that case.






[CLJS-1678] variadic defn can be called for missing fixed arities, overlapping arity Created: 11/Jun/16  Updated: 18/Aug/17

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: core


 Description   

For defns with a variadic arity: if invoked with a missing fixed arity, they use the variadic method instead of erroring; if invoked with a fixed arity that is the max fixed arity, variadic mathod instead of fixed form is invoked.

(defn f-hole
  ([a] 1)
  ([a b c d & args] "4 or more"))

(f-hole 1 2) ; =>"4 or more", should be error

(defn f-overlap-mfa
  ([a b] 2)
  ([a b & c] "2+"))

(f-overlap-mfa 1) ;=> "2+", should be error
(f-overlap-mfa 1 2) ;=> "2+", should be 2
(f-overlap-mfa 1 2 3) ;=> "2+", correct

A way to fix the f-hole bug is to emit a "case X:" into the switch statement for all X with no signature or less than max-fixed-arity.

The f-overlap-mfa I'm not sure why is happening and didn't investigate deeply.



 Comments   
Comment by Francis Avila [ 11/Jun/16 8:31 AM ]

Sorry, filed against CLJ instead of CLJS!

Comment by Rohit Aggarwal [ 12/Jun/16 9:41 AM ]

The behaviour I am seeing for f-overlap-mfa is:

(f-overlap-mfa 1) ;=> "2+"
(f-overlap-mfa 1 2) ;=> 2
(f-overlap-mfa 1 2 3) ;=> "2+"

So the two argument result is different for me than you, Francis Avila.

The call with just one argument does give a warning though:

WARNING: Wrong number of args (1) passed to cljs.user/f-overlap-mfa





[CLJS-1702] Warning when using private vars Created: 07/Jul/16  Updated: 18/Aug/17

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: warnings

Approval: Vetted

 Description   

Currently no warning or error of any kind is given. Throwing an error and forcing users to go through vars is somewhat less attractive since vars dump information like file, line, etc. A warning would be a simple way to flag users that they are treading into dangerous territory. Downstream tooling error handling can make it a hard error if they like.






[CLJS-1753] cljs.pprint does not print default tagged literals/elements Created: 16/Aug/16  Updated: 18/Aug/17

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

Type: Defect Priority: Major
Reporter: Miroslav Kubicek Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: printing
Environment:

Win 7 64bit


Approval: Vetted

 Description   

Hi there!
I am having troubles making the cljs pretty print (cljs.pprint/pprint) behave the same way as the regular cljs or clj print function when it goes down to tagged elements that are by default used to denote custom records.

See example below - pr, pr-str, print and print-str functions all use the default approach toward creating (edn-like) tagged element for MyRecord and all produce the same result:
#cljs.user.MyRecord{:value "a"}

On the other hand pprint just ignores the record's tag and simply just traverses/prints it as a map:
{:value "a"}

Is there some setting and/or parameter in cljs.pprint namespace I am missing? I looked briefly at the code, but it seems it uses print-str by default - so maybe it just traverses the graph depth-first and does not check for every node's type? At this stage this seems like a bug to me as the expected behavior of the pprint function is that it would behave the same way print and other core functions do.

THIS WORKS:

cljs.user=> (defrecord MyRecord [value])
cljs.user/MyRecord
cljs.user=> (pr (MyRecord. "a"))
#cljs.user.MyRecord{:value "a"}
nil
cljs.user=> (pr-str (MyRecord. "a"))
"#cljs.user.MyRecord{:value \"a\"}"
cljs.user=> (print (MyRecord. "a"))
#cljs.user.MyRecord{:value a}
nil
cljs.user=> (print-str (MyRecord. "a"))
"#cljs.user.MyRecord{:value a}"

BUT THIS DOESN'T:

cljs.user=> (cljs.pprint/pprint (MyRecord. "a"))
{:value "a"}
("{:value \"a\"}\n")
cljs.user=> (with-out-str (cljs.pprint/pprint (MyRecord. "a")))
"{:value \"a\"}\n"

According to github the head revision of the cljs.pprint namespace has not changed since 1.7.28 so I'd assume all versions up to the current one are affected.

Thanks for help!



 Comments   
Comment by David Nolen [ 08/Jul/17 10:10 AM ]

Patch must supply a test case.

Comment by António Nuno Monteiro [ 08/Jul/17 3:13 PM ]

Not sure if this is a bug. Running this at a Clojure REPL produces the same results:

user=> (defrecord MyRecord [value])
user.MyRecord
user=> (require '[clojure.pprint :as pprint])
nil
user=> (pprint/pprint (MyRecord. "a"))
{:value "a"}
nil
Comment by Miroslav Kubicek [ 09/Jul/17 12:23 AM ]

Good catch, that is indeed interesting... but I'd still argue that this is definitely a bug (which just seems to be present in clj as well) - pretty print should work in the same way as print does, only prettier (aka more readable). It should not have its own idiosyncrasies. Every other print function (even including spit) works this way, so I can not imagine what would be the reason (or a use case) for pprint not to honor tagged literals specification of clojure and print things its own way.





[CLJS-2002] Don't throw when no *print-fn* is set Created: 07/Apr/17  Updated: 18/Aug/17

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: David Nolen
Resolution: Unresolved Votes: 1
Labels: errors, warnings

Attachments: Text File CLJS-2002.patch    
Approval: Vetted

 Description   

Currently calling (enable-console-print!) causes a bunch of code to be retained in :advanced mode even if you never print.

While that is not ideal it doesn't cause runtime errors. Not calling it and trying to print however will throw an exception which will potentially break your app.

No *print-fn* fn set for evaluation environment

So we end up in a no-win situation for :advanced builds where a "forgotten" prn may break your app in production or "maybe" bloating your file size by retaining all the print-related things.

I think the no-print-fn condition should never throw, maybe just try to write a warning using console.log. Or just dropping the prn altogether.



 Comments   
Comment by David Nolen [ 13/Jul/17 8:29 PM ]

Let's move the old behavior to `string-print` only.

Comment by Mike Fikes [ 13/Jul/17 9:01 PM ]

The attached CLJS-2002.patch moves the throw to string-print and using nil as the init for the two Vars solves CLJS-2231. But it doesn't address the ticket as written: Leaving an inadvertent prn in your code leads to a call to string-print which throws.

Comment by David Nolen [ 13/Jul/17 10:06 PM ]

https://github.com/clojure/clojurescript/commit/797e247fbef676544060a57da995f058db061f37 partially addresses this issue. Keeping this open and moving to lower priority, we should revisit.





[CLJS-2270] Support AOT compilation of macro namespaces (bootstrap) Created: 24/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: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bootstrap

Approval: Vetted




[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-2221] cljs.util/relative-name still has issues on case-insensitive platforms Created: 13/Jul/17  Updated: 18/Aug/17

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

Type: Defect Priority: Minor
Reporter: Mark Hepburn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Windows (file-system is case-insensitive)
Emacs + Cider, and cmd.exe


Attachments: Text File CLJS-2221.patch     File path-for-path-handling.diff    
Patch: Code
Approval: Vetted

 Description   

cljs.util/relative-name currently works by converting paths to strings and manipulating those. This can cause issues where the strings are identical apart from case: in my case relating to a file from deps.cljs, under Emacs (System/getProperty "user.dir") produces a path with a lower-case drive-letter ("c:\\Users
..."), while the URL argument to relative-name has an upper-case drive-letter, eg "C:\\Users
...".

I believe a more robust approach is to use java.nio.file.Path, which handles paths in a platform-specific way. Patch attached, but happy to take alternative suggestions of course.



 Comments   
Comment by Mark Hepburn [ 13/Jul/17 12:51 AM ]

(I haven't done a sweep of other path manipulation functions, but that might not be a bad idea either if this approach is accepted)

Comment by David Nolen [ 13/Jul/17 7:50 AM ]

Thanks! Please follow the patch guidelines here https://clojurescript.org/community/patches

Have you submitted your Clojure CA? https://clojure.org/community/contributing

Comment by Francis Avila [ 13/Jul/17 9:17 AM ]

java.nio.file.Path is new in Java 7, but Clojure officially supports Java 6. Using Path would therefore mean that ClojureScript requires Java 7+.

(Not saying this isn't the right approach, but just FYI that our Java version dependency might increase.)

Comment by David Nolen [ 13/Jul/17 10:22 AM ]

ClojureScript already has a hard dependency on Java 7 due to Google Closure and its dependencies.

Comment by Mark Hepburn [ 13/Jul/17 6:32 PM ]

Thanks Francis, I'll admit that did cross my mind and while I thought Java7 was the minimum, I forgot to check.

Thanks David; I've signed the CA and I'll attach the re-formatted patch.

Comment by Mark Hepburn [ 13/Jul/17 6:33 PM ]

Renamed according to guidelines, and comment expanded.

Comment by Oliver George [ 30/Jul/17 8:26 PM ]

A possible corner case for discussion. Test is my own, not in code base at present.

(deftest ^:windows test-relative-name2
  (let [initial (System/getProperty "user.dir")]
    (System/setProperty "user.dir" "C:\\Users\\user\\clojurescript")
    (is (= (util/relative-name (io/file "C:\\Temp\\clojurescript\\out\\index.js")) "C:\\Temp\\clojurescript\\out\\index.js"))
    (System/setProperty "user.dir" initial)))
FAIL in (test-relative-name2) (util_tests.clj:38)
expected: (= (util/relative-name (io/file "C:\\Temp\\clojurescript\\out\\index.js")) "C:\\Temp\\clojurescript\\out\\index.js")
  actual: (not (= "index.js" "C:\\Temp\\clojurescript\\out\\index.js"))

Code currently assumes the file passed is in the path and just plucks off N dirs. No sure if this is guaranteed. Not sure what is best result if it does happen.

  • Could assert to prove it. This has the advantage of providing a nice error for code which violates the assumption.
  • Could pass back original file path in case it's not.




[CLJS-2326] Indexing node_modules can't find `main` when it doesn't have an extension Created: 17/Aug/17  Updated: 18/Aug/17  Resolved: 18/Aug/17

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

Type: Defect Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2326.patch    
Patch: Code and Test
Approval: Accepted

 Description   

This problem is present in both module-deps and cljs.closure/index-node-modules-dir

A notorious example is the `bootstrap` package, which lists `dist/js/bootstrap` in the main entry, where bootstrap is boostrap.js



 Comments   
Comment by David Nolen [ 18/Aug/17 7:46 AM ]

Needs a rebase to master.

Comment by António Nuno Monteiro [ 18/Aug/17 10:57 AM ]

Replaced patch with a rebased version.

Comment by David Nolen [ 18/Aug/17 12:08 PM ]

fixed https://github.com/clojure/clojurescript/commit/694a623018ff2918eba88a9570dbfa685019c6f3





[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-2328] Args are not provided to *main-cli-fn* with optimizations Created: 17/Aug/17  Updated: 18/Aug/17  Resolved: 18/Aug/17

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

Type: Defect Priority: Minor
Reporter: Yegor Timoshenko Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2328.patch    
Patch: Code
Approval: Accepted

 Description   
(ns example.core)

(defn -main [& argv]
  (enable-console-print)
  (println argv))

(set! *main-cli-fn* -main)

This works when compiled with simple optimizations, and doesn't with advanced optimizations.

Proposed fix is to use nodejs/process instead of js/process here:
https://github.com/clojure/clojurescript/blob/fb90282f41799fe509b143e7870eaf4f9885de41/src/main/cljs/cljs/nodejscli.cljs#L21



 Comments   
Comment by António Nuno Monteiro [ 17/Aug/17 4:22 PM ]

Patch attached with fix.

Comment by Yegor Timoshenko [ 17/Aug/17 4:56 PM ]

Antonio, why not just use cljs.nodejs/process? It uses externs.

Comment by António Nuno Monteiro [ 17/Aug/17 4:59 PM ]

`cljs.nodejs/process` is the same as `js/process`. That wasn't the problem.

`argv` was being munged so we need to access it as a string. Or provide externs for it.

Comment by Yegor Timoshenko [ 17/Aug/17 5:22 PM ]

I see. Thanks!

Comment by David Nolen [ 18/Aug/17 7:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/799325ac84a7cc3acdd68ce98df61328779c63d7





[CLJS-2327] module_deps.js doesn't know about browser field advanced usage Created: 17/Aug/17  Updated: 18/Aug/17  Resolved: 18/Aug/17

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

Type: Defect Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2327.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 18/Aug/17 7:46 AM ]

fixed https://github.com/clojure/clojurescript/commit/5180e7e2891a92d56b7f1a3f1268e615a96dfd33





[CLJS-2325] konan cannot handle `export ... from` imports Created: 16/Aug/17  Updated: 16/Aug/17  Resolved: 16/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: modules, npm-deps

Approval: Vetted

 Description   

We use a very small dependency Node.js `konan` to supply import detection. Unfortunately `konan` only handles `require` and `import`, it doesn't check for `export ... from`. Given how trivial this code is we should just probably write our own logic for this in `module_deps.js`.



 Comments   
Comment by David Nolen [ 16/Aug/17 1:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/56a880cb09d57e287a3eba4839c4f5688958850f





[CLJS-2309] :module foreign-libs order not preserved Created: 08/Aug/17  Updated: 16/Aug/17  Resolved: 16/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: modules

Attachments: Text File CLJS-2309.patch    
Patch: Code and Test
Approval: Vetted

 Description   

It appears foreign-libs order is not preserved. Since these do not provide real Closure require/provide information it is important they get added to their respective module in the correct order. Patch should provide a test case.



 Comments   
Comment by António Nuno Monteiro [ 13/Aug/17 5:49 PM ]

Attached a patch with a test that demonstrates the issue is no longer reproducible.

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

Test patch applied https://github.com/clojure/clojurescript/commit/0b8cff8027fa7fc5b2d325df0206ba90d780f7a6. Keeping the issue open for now.

Comment by David Nolen [ 15/Aug/17 5:42 PM ]

Got a repro from Tony Kay today. This is definitely an issue. I suspect due to the fact that we don't dep sort after adding preloads.

Comment by Tony Kay [ 15/Aug/17 6:09 PM ]

I tested both :simple and :advanced optimizations against my larger project (both were failing before). The tip of master (905) is working for me now.

Comment by David Nolen [ 16/Aug/17 12:21 AM ]

fixed in master

Comment by David Nolen [ 16/Aug/17 12:22 AM ]

was fixed by https://github.com/clojure/clojurescript/commit/6130143bbc07e49c3d6e9313377226b9551be434





[CLJS-2322] Require only `@cljs-oss/module-deps` to be installed to figure out Node.js dep graph Created: 14/Aug/17  Updated: 15/Aug/17  Resolved: 15/Aug/17

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

Type: Enhancement Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2322.patch    
Patch: Code
Approval: Accepted

 Comments   
Comment by David Nolen [ 15/Aug/17 3:51 PM ]

fixed https://github.com/clojure/clojurescript/commit/c66e1385f69d078360f84ef5c60b538d757bab9e





[CLJS-2324] module-graph doesn't munge :requires when indexing inputs Created: 15/Aug/17  Updated: 15/Aug/17  Resolved: 15/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: modules

Approval: Accepted

 Description   

This means lookups will fail and deps will get dropped



 Comments   
Comment by David Nolen [ 15/Aug/17 3:33 PM ]

appears fixed with https://github.com/clojure/clojurescript/commit/86b482a0390b2d90d8bee1c88ef248e339e35fcc, but needs test case.

Comment by David Nolen [ 15/Aug/17 3:51 PM ]

added test case https://github.com/clojure/clojurescript/commit/3567cc0155f436617978eb75be08f17ec2811472





[CLJS-2323] Wrong return value for data readers with records Created: 15/Aug/17  Updated: 15/Aug/17  Resolved: 15/Aug/17

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

Type: Defect Priority: Critical
Reporter: Andrew Rudenko Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Patch: Code
Approval: Accepted

 Description   

ClojureScript generates wrong code for data readers which returns records.

Minimal case to reproduce:

dr.cljc
(ns dr)

(defrecord Position [line col])

#?(:cljs (.log js/console #dr/pos {:line 1 :col 5}))
data_readers.cljc
{dr/pos dr/map->Position}
build.clj
(require 'cljs.build.api)

(cljs.build.api/build "." {:output-to "main.js"})

To compile:

java -cp ~/Downloads/cljs.jar:. clojure.main build.clj

It generates

console.log(new cljs.core.PersistentArrayMap(null, 2, [new cljs.core.Keyword(null,"line","line",212345235),(1),new cljs.core.Keyword(null,"col","col",-1959363084),(5)], null));

So, it just ignores record and emit code only for underlying map.



 Comments   
Comment by David Nolen [ 15/Aug/17 12:34 PM ]

This works now in master as of https://github.com/clojure/clojurescript/commit/f5af28f814896d3a538cba717331b651a5ef9239.

We should provide a test case before closing.

Comment by David Nolen [ 15/Aug/17 12:40 PM ]

fixed https://github.com/clojure/clojurescript/commit/4bc7e87f74a760e570428565a6473796628adc04





[CLJS-2285] Relative path exception thrown on Windows using :preload Created: 30/Jul/17  Updated: 15/Aug/17

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

Type: Defect Priority: Major
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Windows


Attachments: Text File CLJS-2285b.patch     Text File CLJS-2285.patch    
Patch: Code

 Description   

Seems like a small bug in appending "/" in the strip-user-dir helper. This code was recently updated.

Steps to repeat from CLJS-2036 can be used. I think it boils down to:

1. Add a :preload namespace to the build
2. Require a foreign lib in the preload ns

Expect: Builds fine

Actual: `IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path` full exception below.

Works on 1.9.671.

Fails on 1.9.854.

Exception in thread "main" java.lang.IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path, compiling:(C:\Repos\canidep\scripts\build.clj:36: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)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
        at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:207)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
        at java.lang.reflect.Method.invoke(Unknown Source)
        at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
        at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:207)
        at user$eval5.invokeStatic(form-init8570637412295703479.clj:1)
        at user$eval5.invoke(form-init8570637412295703479.clj:1)
        at clojure.lang.Compiler.eval(Compiler.java:6927)
        at clojure.lang.Compiler.eval(Compiler.java:6917)
        at clojure.lang.Compiler.load(Compiler.java:7379)
        at clojure.lang.Compiler.loadFile(Compiler.java:7317)
        at clojure.main$load_script.invokeStatic(main.clj:275)
        at clojure.main$init_opt.invokeStatic(main.clj:277)
        at clojure.main$init_opt.invoke(main.clj:277)
        at clojure.main$initialize.invokeStatic(main.clj:308)
        at clojure.main$null_opt.invokeStatic(main.clj:342)
        at clojure.main$null_opt.invoke(main.clj:339)
        at clojure.main$main.invokeStatic(main.clj:421)
        at clojure.main$main.doInvoke(main.clj:384)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:383)
        at clojure.lang.AFn.applyToHelper(AFn.java:156)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: C:\Repos\canidep\src\uilib.js is not a relative path
        at clojure.java.io$as_relative_path.invokeStatic(io.clj:414)
        at clojure.java.io$file.invokeStatic(io.clj:426)
        at clojure.java.io$file.invoke(io.clj:418)
        at cljs.closure$write_javascript.invokeStatic(closure.clj:1698)
        at cljs.closure$write_javascript.invoke(closure.clj:1691)
        at cljs.closure$source_on_disk.invokeStatic(closure.clj:1740)
        at cljs.closure$source_on_disk.invoke(closure.clj:1735)
        at cljs.closure$build$fn__6925.invoke(closure.clj:2536)
        at clojure.core$map$fn__4785.invoke(core.clj:2646)
        at clojure.lang.LazySeq.sval(LazySeq.java:40)
        at clojure.lang.LazySeq.seq(LazySeq.java:49)
        at clojure.lang.Cons.next(Cons.java:39)
        at clojure.lang.RT.next(RT.java:688)
        at clojure.core$next__4341.invokeStatic(core.clj:64)
        at clojure.core$dorun.invokeStatic(core.clj:3033)
        at clojure.core$doall.invokeStatic(core.clj:3039)
        at clojure.core$doall.invoke(core.clj:3039)
        at cljs.closure$build.invokeStatic(closure.clj:2536)
        at cljs.closure$build.invoke(closure.clj:2444)
        at cljs.build.api$build.invokeStatic(api.clj:205)
        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$eval8894.invokeStatic(build.clj:36)
        at user$eval8894.invoke(build.clj:36)
        at clojure.lang.Compiler.eval(Compiler.java:6927)
        at clojure.lang.Compiler.load(Compiler.java:7379)
        ... 42 more


 Comments   
Comment by Oliver George [ 30/Jul/17 5:24 AM ]

The problem is highlighted by this:

In cljs.util/relative-name the user-path uses "/" as the separator

(.getFile (.toURL (io/file (System/getProperty "user.dir"))))

=> "/C:/Repos/canidep/"

But the code which appends "/" if it's missing uses File/separator

File/separator

=> "\\"
Comment by Oliver George [ 30/Jul/17 8:27 PM ]

I prefer the patch on CLJS-2221 to solve this problem.

Comment by Oliver George [ 30/Jul/17 8:35 PM ]

Make test for util/relative-path platform specific. Test specific Windows
behaviour.

Does not fix bug.

Comment by Inge Solvoll [ 15/Aug/17 7:10 AM ]

Something similar happens when following this tutorial https://ajpierce.github.io/posts/react-figwheel-npm-2/

When starting figwheel, I get this stacktrace:

java.lang.IllegalArgumentException: c:\dev\splort\node_modules\react-dom\index.js is not a relative path
at clojure.java.io$as_relative_path.invokeStatic (io.clj:414)
clojure.java.io$file.invokeStatic (io.clj:426)
clojure.java.io$file.invoke (io.clj:418)
cljs.closure$write_javascript.invokeStatic (closure.clj:1698)
cljs.closure$write_javascript.invoke (closure.clj:1691)
cljs.closure$process_js_modules$fn__15415.invoke (closure.clj:2348)

Is this the same entry point to the error, or does it need a separate issue?





[CLJS-2319] cljs.core/mod handling of floats inconsistent with Clojure & JavaScript Created: 13/Aug/17  Updated: 15/Aug/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    

 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`.





[CLJS-1986] Support for ES6 generators Created: 19/Mar/17  Updated: 14/Aug/17  Resolved: 16/Jun/17

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

Type: Enhancement Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: Text File generators.patch    
Patch: Code

 Description   

The attached patch adds support for ES6 generators (supported in node for a while now).

Generator functions can be declared through the :generator metadata that this patch adds support for. There is a new special token, `yield`.



 Comments   
Comment by António Nuno Monteiro [ 19/Mar/17 12:06 PM ]

I don't see how this patch is desirable. The ClojureScript compiler emits ES3 compatible code, which doesn't include generators.

In general, I think it's best to raise issues like this in the mailing list or the Clojurians slack before wasting time doing work that's not going to be accpeted.

Comment by James Laver [ 19/Mar/17 12:25 PM ]

Why is it that we emit ES3 compatible code in particular (I wasn't aware)? Google closure compiler has had support for compiling generators to ES3 for some time now https://github.com/google/closure-compiler/wiki/ECMAScript6

Comment by David Nolen [ 20/Mar/17 2:38 PM ]

I agree that this issues like this need some discussion first. Feel free to start one on the mailing list / Slack or IRC.

Comment by David Nolen [ 16/Jun/17 9:58 AM ]

Adding special forms to ClojureScript like this just isn't desirable. While it's possible that we decide to add something around generators it needs plenty of discussion first before we embark on any work.

Comment by Dusan Maliarik [ 14/Aug/17 6:01 AM ]

Did the discussion start on this one? As of ES3 compatible code, I think that's something for Closure Compiler to handle, although I don't see the practicality in this restriction. Targeting ES5 is just fine for everyone, and CC is perfectly capable of transpiling generators down to ES5.





[CLJS-2321] Do not automatically call `set-loaded!` on the user's behalf Created: 13/Aug/17  Updated: 13/Aug/17  Resolved: 13/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs.loader, modules

Approval: Vetted

 Description   

The current design prevents users from building functionality on top of `cljs.loader` and introduces many composition issues.



 Comments   
Comment by David Nolen [ 13/Aug/17 3:08 PM ]

Need to update the documentation. Possibly fix some tests, we should also add a docstring to `cljs.loader` marking the alpha status.

Comment by António Nuno Monteiro [ 13/Aug/17 3:18 PM ]

This is probably as easy as reverting CLJS-2157 (commit https://github.com/clojure/clojurescript/commit/15c1eb2d511c2f494400183bab1d9eca611777d6)

Comment by David Nolen [ 13/Aug/17 4:58 PM ]

fixed https://github.com/clojure/clojurescript/commit/6b602b7fdd3967b2084e382c70678b921582fa18





[CLJS-2310] should not emit goog.require for foreign-libs under :modules Created: 08/Aug/17  Updated: 13/Aug/17  Resolved: 13/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: modules

Approval: Screened

 Description   

ModuleManager will attempt to eval the module but will encounter goog.require("foreign-lib") which will fail. This patch requires a test.



 Comments   
Comment by David Nolen [ 09/Aug/17 7:54 AM ]

This issue is not quite a simple as it sounds. ModuleManager doesn't appear to know what namespaces it is loading, it only knows what files compose a module. The sources of a module will be loaded via XHR, concatenated and then eval'ed. This is why a statement like goog.require("cljsjs.react") will fail, since that namespace is never actually provided.

However, for modules loaded via script tags on the page we cannot elide goog.require for foreign libs.

This seems to suggest we may want to load modules uniformly? That is, under :none, :output-to files (with entry points using cljs.loader) should perhaps use cljs.loader to load the module. There are some alternatives that are also probably worth consideration.

Comment by David Nolen [ 11/Aug/17 4:42 PM ]

Using cljs.loader to load the module in the :output-to file introduces a bunch of other problems as far as I can tell, likely related to loading files a second time. Instead :output-to files should load foreign libs separately via goog.require before everything else.

Comment by David Nolen [ 13/Aug/17 4:48 PM ]

fixed https://github.com/clojure/clojurescript/commit/8bdcc9a4dc0b011d89886cea6ea7bed92d435ea8, but missing the test, due to some relative path issues, I don't have time to look at the moment.





[CLJS-2320] Warn if attempting to load a module which has no entry points which require `cljs.loader` Created: 13/Aug/17  Updated: 13/Aug/17  Resolved: 13/Aug/17

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

Type: Defect Priority: Critical
Reporter: David Nolen Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: cljs.loader, modules

Approval: Vetted

 Comments   
Comment by David Nolen [ 13/Aug/17 4:44 PM ]

Declined due to CLJS-2321





[CLJS-2318] module-deps.js doesn't respect the package.json `module` field Created: 12/Aug/17  Updated: 13/Aug/17  Resolved: 13/Aug/17

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

Type: Defect Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2318.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 13/Aug/17 10:52 AM ]

fixed https://github.com/clojure/clojurescript/commit/1163381724c0e8ab11e998e9ab02d7a752848acc





[CLJS-2316] Upgrade Closure Compiler to August release Created: 09/Aug/17  Updated: 11/Aug/17  Resolved: 11/Aug/17

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

Type: Task Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-2316.patch    
Approval: Accepted

 Comments   
Comment by David Nolen [ 10/Aug/17 6:08 AM ]

I tried to apply but it looks like the zip for this hasn't been uploaded yet.

Comment by António Nuno Monteiro [ 11/Aug/17 10:48 AM ]

Replaced the patch with one that doesn't depend on the dl.google.com link, but instead downloads the Closure Compiler JAR from Maven just like the other dependencies.

Comment by David Nolen [ 11/Aug/17 2:19 PM ]

Patch appears to need a rebase to master.

Comment by David Nolen [ 11/Aug/17 3:22 PM ]

fixed https://github.com/clojure/clojurescript/commit/5ee5f3f1e35b77eb0953c696b79889699c9c56ba





[CLJS-2288] Fix tests failing on Windows due to platform specific file separators Created: 31/Jul/17  Updated: 11/Aug/17  Resolved: 11/Aug/17

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

Type: Defect Priority: Major
Reporter: Oliver George Assignee: Oliver George
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

Windows


Attachments: Text File CLJS-2288-2.patch     Text File CLJS-2288-cr1.patch    

 Description   

There are tests which fail on Windows due to assumptions about the file separator used. We can get these test working on windows by avoiding the assumption.



 Comments   
Comment by Oliver George [ 31/Jul/17 6:04 PM ]

Sample of fixes and FIXMEs needed to get cljs.closure-tests working on Windows.

Provided for feedback. Not ready for merging.

Comment by David Nolen [ 01/Aug/17 6:36 AM ]

I would expect (io/file "foo" "bar.js") not (io/file "foo/bar.js").

Comment by Juho Teperi [ 01/Aug/17 11:06 AM ]

:provides values should always use /.

Proper solution is probably to fix index-node-modules-dir to replace }} with {{/. Similar to how module_deps.js works.

I attached a patch which should fix this, but I don't have acccess to Windows just now. The changes to test-cljs-1882 looked correct, so I included that in this patch.

Comment by Oliver George [ 02/Aug/17 5:51 PM ]

I think we're parking this ticket for now. Could be closed or revisited once the work on CLJS-2291 is committed.

Comment by David Nolen [ 11/Aug/17 3:08 PM ]

Resolved in master.





[CLJS-2317] Upgrade Google Closure Library Created: 10/Aug/17  Updated: 11/Aug/17  Resolved: 11/Aug/17

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

Type: Task Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: closure-library

Attachments: Text File CLJS-2317.patch    
Approval: Accepted

 Comments   
Comment by David Nolen [ 11/Aug/17 8:11 AM ]

Can we please remove the whitespace changes in this patch?

Comment by António Nuno Monteiro [ 11/Aug/17 10:31 AM ]

Reverted the whitespace changes in the replaced patch.

Comment by David Nolen [ 11/Aug/17 2:57 PM ]

fixed https://github.com/clojure/clojurescript/commit/92433701acc6f86665b81349dc8c9eb4048ec464





[CLJS-2312] Miss-compile: Uncaught SyntaxError: Unexpected token default Created: 09/Aug/17  Updated: 11/Aug/17  Resolved: 11/Aug/17

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

Type: Defect Priority: Major
Reporter: Dieter Komendera Assignee: António Nuno Monteiro
Resolution: Completed Votes: 0
Labels: compiler

Attachments: Text File CLJS-2312.patch     Zip Archive default-demo2.zip     Zip Archive defaults-demo.zip    
Patch: Code
Approval: Vetted

 Description   

This is a regression since 1.9.854.

This:

(ns foo.main
  (:require [cljs.js :as cljs]
            [cljs.tools.reader :as r]
            [cljs.analyzer :refer [*cljs-warning-handlers*]]
            [cljs.tools.reader.reader-types :as rt]
            [clojure.string :as string]))

(defn foo [default]
  (js/console.log default))

(foo "bar")

compiles to

goog.provide('foo.main');
goog.require('cljs.core');
goog.require('cljs.js');
goog.require('cljs.tools.reader');
goog.require('cljs.analyzer');
goog.require('cljs.tools.reader.reader_types');
goog.require('clojure.string');
foo.main.foo = (function foo$main$foo(default$){
return console.log(default);
});
foo.main.foo.call(null,"bar");

Which results in Uncaught SyntaxError: Unexpected token default at runtime.

The compiler itself is also affected:
https://github.com/clojure/clojurescript/blob/master/src/main/clojure/cljs/compiler.cljc#L500

When removing all the requires in the test case, it compiles fine.

Attaching a test case.



 Comments   
Comment by Dieter Komendera [ 09/Aug/17 9:18 AM ]

I was wrong in my initial assumption that this has anything to do with requiring stuff from the cljs* namespace, instead this happens with :language-out set to es5.

Comment by David Nolen [ 10/Aug/17 6:07 AM ]

I don't really understand the provided patch. It could use a description (on the commit itself). It appears to revert the default changes and then only avoids munging if the var has a namespace?

Comment by António Nuno Monteiro [ 11/Aug/17 10:50 AM ]

The patch has been updated with an inline comment detailing the proposed solution.

Comment by David Nolen [ 11/Aug/17 2:16 PM ]

fixed https://github.com/clojure/clojurescript/commit/20a6e1fcd8724ad65a3776112dcc7bfd5124a094





[CLJS-2315] module_deps.js can't resolve JSON modules Created: 09/Aug/17  Updated: 11/Aug/17  Resolved: 11/Aug/17

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

Type: Defect Priority: Critical
Reporter: António Nuno Monteiro Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: js-modules

Attachments: Text File CLJS-2315.patch    
Patch: Code and Test
Approval: Accepted

 Comments   
Comment by David Nolen [ 10/Aug/17 6:47 AM ]

After applying the patch, running the test for this patch fails.

Comment by António Nuno Monteiro [ 10/Aug/17 11:53 AM ]

Replaced the patch with one that includes files that I had forgotten to stage.

Comment by David Nolen [ 11/Aug/17 8:12 AM ]

fixed https://github.com/clojure/clojurescript/commit/8e4f49d73a83972f27de478424f8ca8de7d6742a