<< Back to previous view

[CLJS-1084] once a REPL require is borked cannot recover Created: 05/Mar/15  Updated: 05/Mar/15

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

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


 Description   

Once a (require ...) has failed cannot recover, we should restore the old namespace if this occurs if possible.






[CLJS-1083] cljs.closure/source-for-namespace does not do proper namespace munging Created: 05/Mar/15  Updated: 05/Mar/15

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

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


 Description   

(require 'foo.bar.function) will not munge function to {{function$}






[CLJS-1082] analysis memoization bug Created: 05/Mar/15  Updated: 05/Mar/15

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

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


 Description   

cljs.compiler/compile-file removes ns entries from :cljs.analyzer/namespaces in the compilation environment but cljs.analyzer/analyze-file uses :cljs.analyzer/analyzed-cljs as memoization hook. This means namespace analysis will get dropped and never restored.






[CLJS-937] local fn name should be lexically munged Created: 30/Dec/14  Updated: 05/Mar/15

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

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


 Description   
(ns foo.bar)

(fn console []
  ...)

The local name should be foo$bar$$console.



 Comments   
Comment by David Nolen [ 30/Dec/14 3:12 PM ]

See CLJS-833

Comment by David Nolen [ 03/Mar/15 9:13 AM ]
(ns foo.bar)

(defn console []
  (fn log []
    ...))

The name of the inner fn should be foo$bar$$console$log.

This has an added benefit for older JavaScript runtimes like Rhino. Rhino will not associate vars with anonymous functions. Today we supply a name but wrongly by leaking a bare name to the global scope. By always scoping the function to its lexical environment we avoid arbitrary top level pollution in addition to namespace recoverability in Rhino by splitting on $$ when parsing stacktraces into canonical form.

Comment by David Nolen [ 03/Mar/15 6:53 PM ]

This also benefits WebKit debugging.





[CLJS-897] AOT core.cljs Created: 02/Dec/14  Updated: 05/Mar/15  Resolved: 04/Mar/15

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

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


 Comments   
Comment by David Nolen [ 08/Dec/14 4:22 PM ]

As pointed out by Colin Fleming it will be important to cache both optimized and unoptimized ClojureScript source - specifically the static-fns build option toggled.

Comment by David Nolen [ 04/Mar/15 11:44 PM ]

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

Comment by David Nolen [ 04/Mar/15 11:45 PM ]

:static-fns is always the default for core now. This optimization is about speeding up dev builds. Production builds will need further consideration.

Comment by Thomas Heller [ 05/Mar/15 3:58 AM ]

I guess I'm too late to comment on this but I think special case AOT compilation/caching of cljs/core.cljs is bad. We should rather provide a solid caching implementation that can generate this itself rather than shipping it with each release. Yes, the first "boot" will be slower since it has to generate the cache but each start after will have an up-to-date cache for not only core.cljs but also every other file. That will mean the startup performance will be much better for everything instead of just this one single special case.

We can easily tweak things like :static-fns by setting it to true for every file from a .jar for example.

I think it would be better to bring :cache-analysis to a point where it can default to true instead of special case code for cljs/core.cljs.

Don't quite see the win in this, just looks like technical debt to me.

PS: Sorry I keep commenting on closed issues, you are moving to fast for me these days David.

Comment by David Nolen [ 05/Mar/15 7:43 AM ]

The strategy of this ticket is precisely the same as the one taken by Clojure, the end result is not small for day to day coding, particularly for REPLs and the fact that many sophisticated ClojureScript programs do not reach the >9000 lines of cljs.core.

As far as smarter caching for 3rd party libraries in JARs - see CLJS-1067.

Yes :cache-analysis true will one day become default, not there yet.





[CLJS-899] AOT cache core.cljs analysis Created: 02/Dec/14  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Comments   
Comment by David Nolen [ 04/Mar/15 11:45 PM ]

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





[CLJS-1081] Add script to create uberjar Created: 04/Mar/15  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Comments   
Comment by David Nolen [ 04/Mar/15 6:20 PM ]

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





[CLJS-1078] Nashorn REPL should use persistent code cache Created: 04/Mar/15  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Description   

See https://blogs.oracle.com/nashorn/entry/improving_nashorn_startup_time_using



 Comments   
Comment by David Nolen [ 04/Mar/15 6:14 PM ]

fixed https://github.com/clojure/clojurescript/commit/9e00804036a22084f9aa500ca7a9d6b5b990a341





[CLJS-978] Analysis caching doesn't account for constants table Created: 15/Jan/15  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Description   

This means advanced optimizations cannot leverage analysis caching.



 Comments   
Comment by Thomas Heller [ 16/Jan/15 4:48 AM ]

Currently the constants use a generated id, we could instead use the constant itself as the key.

:test/keyword creates cljs.core.constant$keyword$123 where 123 is the sequential id which makes it unusable between compiles. We could instead turn it into cljs.core.constant$keyword$_COLON_test_SLASH_keyword (to reuse current munge). We would then just need to keep track of which namespace uses which constant and emit this as well in the analysis cache.

Might be missing something, will investigate a bit later.

Comment by David Nolen [ 16/Jan/15 5:59 AM ]

The constants table needs to work for all compile time constants - vectors, sets, maps, etc.

Comment by David Nolen [ 04/Mar/15 7:53 AM ]

CLJS-897 CLJS-899 depend on this one

Comment by David Nolen [ 04/Mar/15 8:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/592d28a7e6daf4e9fa9ce8b46661aea9d455c410

Comment by Thomas Heller [ 04/Mar/15 1:55 PM ]

@David: I'm not sure what your plan is with AOT cljs core but I think your fix has some issues. The constants id is sequential and when restoring from cache the constants just go through the normal path of register-constant!. When the dependency order is changed (eg. namespace added/removed or even just removing one constant from a dependency) the sequence might be in a different state when restoring which will cause different ids than used in the cached file. I might be wrong though, should probably verify first.

Comment by David Nolen [ 04/Mar/15 2:18 PM ]

The ids don't matter for anything but advanced builds, they don't need to be persisted at all.

Comment by David Nolen [ 04/Mar/15 2:34 PM ]

Thomas actually upon further consideration I do see your point. But it's hard for me to see an obvious case that wouldn't work now that :recompile-dependents is the default and core is always analyzed first. A minimal failing case would be helpful if you can come up with one. If you can a separate ticket would be great.

Comment by David Nolen [ 04/Mar/15 2:46 PM ]

Hrm ok I see the issue for cached files for advanced builds. But it seems switching from sets to vectors for :cljs.analyzer/constants and the fact that core is analyzed first + :recompile-dependents covers any potential problems?

Comment by Thomas Heller [ 04/Mar/15 6:10 PM ]

I wonder if we could

  • keep a {the-constant some-uuid-key} map for each namespace
  • emit the-constant in the namespace output IF used the first time (checks dependencies when compiling)
  • if the-constant is found in a namespace higher up refer to its id
(ns my-base-ns)

(def x :const)

(ns other-ns (:require my-base-ns))

(def y :const)

turns into

// my_base_ns.js
goog.provide("my_base_ns");
cljs.constants$keyword$aaaa-bbbb-cccc-dddd = cljs.core.keyword("const");
my_base_ns.x = cljs.constants$keyword$aaaa-bbbb-cccc-dddd;

// other_ns.js
goog.provide("other_ns")
other_ns.y = cljs.constants$keyword$aaaa-bbbb-cccc-dddd;

should work for other constants as well.

Not sure if that made sense, way too tired. Will think about it tommorrow.





[CLJS-1057] Nashorn REPL should not use EDN rep for errors Created: 22/Feb/15  Updated: 04/Mar/15

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

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


 Description   

Nashorn REPL should just convert the JVM stacktrace into a string form that mirrors JavaScript stack property and implement IParseStacktrace. This seems like pointless work but it simplifies cljs.repl/pst which needs to grab the error value out of Nashorn runtime anyway. The current strategy will not work work cljs.repl/pst.






[CLJS-1080] Analysis cache should keep constants in visit order Created: 04/Mar/15  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Description   

Track seen in a set but keep the constants visit order via vector.



 Comments   
Comment by David Nolen [ 04/Mar/15 5:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/547d03217046ad4341276bded320fe64c480bed0





[CLJS-1079] add way to execute arbitrary fn upon watch build completion Created: 04/Mar/15  Updated: 04/Mar/15  Resolved: 04/Mar/15

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

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


 Description   

Something which only runs on successful builds



 Comments   
Comment by David Nolen [ 04/Mar/15 5:12 PM ]

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





[CLJS-1056] declaring an already-defined var clobbers its metadata Created: 22/Feb/15  Updated: 04/Mar/15  Resolved: 03/Mar/15

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

Type: Defect Priority: Major
Reporter: Michael Griffiths Assignee: David Nolen
Resolution: Declined Votes: 0
Labels: None


 Description   

For example, in om.core, path is defined at https://github.com/omcljs/om/blob/master/src/om/core.cljs#L106 and then declared later in the file at https://github.com/omcljs/om/blob/master/src/om/core.cljs#L152. Examining its meta at the REPL using

(meta #'om.core/path)

yields:

{:arglists (),
 :test nil,
 :name path,
 :column 1,
 :line 152,
 :file "resources/public/js/out/om/core.cljs",
 :doc nil,
 :ns om.core}

Note that {:arglists} is empty and {:line} points to the line of the declare.

Examining the compiler env using e.g.

(get-in @cljs.env/*compiler* [:cljs.analyzer/namespaces 'om.core :defs 'path])

yields:

{:file "file:/Users/griffithsm/.m2/repository/om/om/0.6.2/om-0.6.2.jar!/om/core.cljs",
 :line 113,
 :column 1,
 :end-line 113,
 :end-column 23,
 :declared true,
 :test true,
 :name om.core/path}


 Comments   
Comment by David Nolen [ 03/Mar/15 5:21 PM ]

The behavior is the same as Clojure. This bug should be filed with the library.

Comment by Michael Griffiths [ 04/Mar/15 5:01 AM ]

Will do, thanks for looking into it!

Comment by Nicola Mometto [ 04/Mar/15 5:47 AM ]

David, I'm not sure that it is true that this is the same behaviour as Clojure.
In clojure not only

(declare foo)
(defn foo [a])
(:arglists (meta #'foo))

returns ([a]) but every `def` replaces the current var metadata with the new meta.

user=> (def ^:foo foo)
#'user/foo
user=> (meta #'foo)
{:ns #<Namespace user>, :name foo, :file "NO_SOURCE_PATH", :column 1, :line 6, :foo true}
user=> (def ^:bar foo)
#'user/foo
user=> (meta #'foo)
{:bar true, :ns #<Namespace user>, :name foo, :file "NO_SOURCE_PATH", :column 1, :line 9}
Comment by David Nolen [ 04/Mar/15 6:16 AM ]

Nicola none of those cases are what this ticket is about. The ticket is about (declare foo) after defn foo [] ...). There may be other issues but it's not this one.

Comment by Nicola Mometto [ 04/Mar/15 6:33 AM ]

Ah, sorry, I got the declare/defn order wrong reading the ticket description.





[CLJS-1034] Support REPL-defined functions in stacktrace infrastructure Created: 12/Feb/15  Updated: 03/Mar/15  Resolved: 03/Mar/15

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

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


 Description   

Some new machinery has recently been added to ClojureScript facilitating dealing with exception stack traces and their source-mapping with the REPL-author-facing documentation here.

As an enhancement, support REPL-defined functions which have namespaces / functions, but no source to refer to. Clojure REPLs accomplish this via NO_SOURCE_FILE; this enhancement ticket asks for the same in ClojureScript.

To accomplish this probably involves expanding the definition of the canonical stactrace format (allowing :file key to me missing or with a nil value, or some such, along with similar ideas for :line and :column), along with some special conditional handling in the underlying source-mapping machinery to conditionally handle REPL-defined functions as a special case.



 Comments   
Comment by Mike Fikes [ 12/Feb/15 8:19 PM ]

A motivating example for this enhancement is detailed in a ticket for the Ambly REPL.

Comment by David Nolen [ 03/Mar/15 6:29 PM ]

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

Comment by Mike Fikes [ 03/Mar/15 6:58 PM ]

Confirmed fixed.

The only issue I could see in testing is that REPL-defined functions appear as munged, but David points out that can be covered with CLJS-937.





[CLJS-1065] Fix inconsistencies in Quick Start Guide Created: 24/Feb/15  Updated: 03/Mar/15

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

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


 Description   

Good feedback from Micah Martin:

  • Compiler in REPL seems broken: https://gist.github.com/slagyr/7d52ba4ba98e96172456
  • Not interested in Node.js so I skipped all that
  • Development Mode: Okay... this is where I'm confused.
  • The wiki says include 'hello.js'.
  • hello-dev.html explicitely includes goog.base.js.
  • hello-dev.html also requires hello.core.
  • The generated hello.js loads goog.base if it isn't already.
  • out/cljs_deps.js looks like cljsbuild's :output-to file
  • cljbuild doesn't generate anything like sample/hello.js
  • maybe I'll learn about all these difference later in the guide
  • It seems that "Development Mode" requires a modest but significant change in the way the js is included in the page. This is not obvious in the guide. It would be nice if no change was needed.
  • At the end of the Quick Start page... where do I go from here? Would be nice if there was a recomendation.
  • The 'lein cljsbuild' page doesn't address the confusion mentioned above
  • "Source maps" mentions :source-map-path... Am I supposed to add this to [:cljsbuild :builds :name :compiler]? Doesn't seem to do anything.

In the end, I have better picture of the ClojureScript landscape. I see where my assumptions failed me, and huzzah, got source maps working.



 Comments   
Comment by David Nolen [ 26/Feb/15 8:55 AM ]

The Rhino REPL issue has been addressed in master.

Comment by David Nolen [ 03/Mar/15 6:35 PM ]

Master is now setup to build an AOTed uberjar. We should attach this to releases moving forward. Then Quick Start can written in the style of Clojure, just run Java against a single JAR. This in fact considerably cooler than what Google Closure Compiler can offer, we ship with a scripting language which means a command line centric interface can be avoided for the time being and AOT means we can start fast.





[CLJS-964] Redefining exists? does not emit a warning like redefining array? does. Created: 06/Jan/15  Updated: 03/Mar/15

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

Type: Defect Priority: Major
Reporter: Uday Verma Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-2665"]



 Description   

ClojureScript:cljs.user> (def array? 2)
WARNING: array? already refers to: /array? being replaced by: cljs.user/array? at line 1 <cljs repl>
2
ClojureScript:cljs.user> (def exists? 2)
2
ClojureScript:cljs.user>



 Comments   
Comment by Mike Fikes [ 06/Jan/15 11:41 PM ]

Additionally, if you define exists? as as a function in your namespace, as in

(defn exists? [] 1)

You need to call it indirectly via its var:

ClojureScript:cljs.user> (@#'exists?)
1

If you instead call it directly, you will get this (this is in the Node.js REPL; similar occurs in JavaScriptCore environment):

ClojureScript:cljs.user> (exists?)
clojure.lang.ExceptionInfo: Wrong number of args (2) passed to: core/exists? at line 1 <cljs repl> {:tag :cljs/analysis-error, :file "<cljs repl>", :line 1, :column 1}
	at clojure.core$ex_info.invoke(core.clj:4403)
	at cljs.analyzer$error.invoke(analyzer.clj:297)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1562)
	at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1605)
	at cljs.analyzer$analyze$fn__1673.invoke(analyzer.clj:1696)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1689)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1684)
	at cljs.repl$evaluate_form.invoke(repl.clj:100)
	at cljs.repl$evaluate_form.invoke(repl.clj:96)
	at cljs.repl$eval_and_print.invoke(repl.clj:188)
	at cljs.repl$repl_STAR_.invoke(repl.clj:322)
	at user$eval3528.invoke(NO_SOURCE_FILE:3)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.eval(Compiler.java:6666)
	at clojure.core$eval.invoke(core.clj:2927)
	at clojure.main$eval_opt.invoke(main.clj:288)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	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: clojure.lang.ArityException: Wrong number of args (2) passed to: core/exists?
	at clojure.lang.AFn.throwArity(AFn.java:429)
	at clojure.lang.AFn.invoke(AFn.java:36)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.AFunction$1.doInvoke(AFunction.java:29)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1568)
	... 21 more

None of this occurs for the other predicates defined near exists? in cljs.core, like array?, true?, etc.

Comment by Thomas Heller [ 07/Jan/15 4:58 AM ]

I suspect that is due to exists? only being a macro with no function in cljs.core. array? and other predicates exist as an inlining macro and a function.

(defn ^boolean array? [x]
  (cljs.core/array? x))

Should just be a matter of creating a similar function for exists?.

Comment by Thomas Heller [ 07/Jan/15 5:00 AM ]

Oops no. Won't be that simple.

;; TODO: x must be a symbol, not an arbitrary expression
(defmacro exists? [x]
  (bool-expr
    (core/list 'js* "typeof ~{} !== 'undefined'"
      (vary-meta x assoc :cljs.analyzer/no-resolve true))))




[CLJS-1049] Include macro information in var resolution functions Created: 20/Feb/15  Updated: 03/Mar/15

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

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


 Description   

The fundamental issue around CLJS-964. Macro information doesn't appear in any of the analysis helpers.



 Comments   
Comment by David Nolen [ 22/Feb/15 12:48 PM ]

This is an important problem. Because this information is missing macros are not of part of the various reflection helpers that are now a big part of ClojureScript REPLs.





[CLJS-112] clojure.data.json -- Read and write JSON strings to/from clojure data structures Created: 16/Dec/11  Updated: 03/Mar/15  Due: 31/Dec/11  Resolved: 03/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: Bobby Calderwood Assignee: Bobby Calderwood
Resolution: Declined Votes: 2
Labels: None

Attachments: Text File 0001-Added-clojure.data.json.patch    
Patch: Code

 Description   

Reading and writing JSON is a common requirement in both server- and client-side applications. ClojureScript needs a way to transform ClojureScript data into JSON, and vice-versa.



 Comments   
Comment by David Nolen [ 03/Mar/15 12:19 PM ]

transit is the answer to this one





[CLJS-37] A way to create js objects and arrays from cljs maps and vectors, without copying if possible. Created: 26/Jul/11  Updated: 03/Mar/15  Resolved: 03/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: Chouser Assignee: Unassigned
Resolution: Declined Votes: 4
Labels: None

Attachments: Text File 0001-Add-as-js.-CLJS-37.patch    
Patch: Code and Test

 Description   

There's currently no convenient way to create a js object from a map. Arrays can be created using the `array` function, but this always creates a copy. In cases where you know the object or array will not be modified, it would be a shame to still pay for a copy of the object. This is especially true when the cljs map or vector is a literal such that nothing else can have a reference to it anyway.



 Comments   
Comment by Chouser [ 26/Jul/11 11:07 PM ]

My plan is a macro `as-js` that takes a single expression. If it's a literal vector or map, it will expand to a js* form that will emit the appropriate array or object literal. In the case of a map, all the keys must be constants (string, keyword, or symbol) in order to for this to work since literal object keys in javascript aren't evaluated. Keyword and symbol keys would be converted to strings.

If the argument to the `as-js` macro isn't a literal (or is a map with non-constant keys), the macro will expand to a call to an `-as-js` protocol fn. When called on a vector, the vector will return its internal array. When called on an ObjMap whose keys are all s convtrings, its internal strobj will be returned. If any of the ObjMap's keys are not strings, then no appropriate js obj exists and a new one will be created, converting keys to strings just like the macro above. HashMaps never contain an appropriate js object, so a new one would be created, again converting keys to strings.

So in the common case of maps whose keys are known at compile time, such as (as-js {:foo "bar" :baz (str "qu" "ux")}), this would at compile time become the js obj literal {"foo": "bar", "baz" cljs.core.str("qu", "ux")}. When the map or vector is not a literal, the least possible copying is done to produce the correct obj or array at runtime.

...why do I have this annoying feeling that I may have done something too compound here?

Comment by David Nolen [ 28/Oct/11 6:58 PM ]

Some work towards a solution: https://github.com/clojure/clojurescript/compare/37-support-for-js-literals

Comment by Chouser [ 28/Oct/11 8:53 PM ]

The macro-based solution I outlined above is unacceptable.

The key issue is that Rich wants the reader to actually create the target type (js array or object) at read time, so special reader syntax is required. I think the syntax was to be #js[1 2 3] for arrays and #js{k v k v} for objects. Then of course there needs to be support in the compiler to generate code that evaluates the array elements and the object values (but not their keys, since the js literal produced can't have expressions for keys).

Comment by Aaron Brooks [ 29/Oct/11 12:28 PM ]

Does a special reader syntax ask for something readable across other platforms? i.e. #native[]/#native{}

It seems that this would enhance the ability of ClojureScript libraries to be reusable across other platforms. It's easier to get around macro/function differences between platforms, however reader syntax differences would cut off the options for sharing source files between Clojure platforms.

Comment by Nicolas Buduroi [ 11/Apr/12 12:30 AM ]

Any update on this issue?

Comment by David Nolen [ 11/Apr/12 2:41 PM ]

Waiting on whether Clojure becomes a JSON superset.

Comment by David Nolen [ 03/Mar/15 12:18 PM ]

Not going to do this.





[CLJS-865] js-dependency-index won't work when running from an uberjar Created: 26/Sep/14  Updated: 03/Mar/15  Resolved: 03/Mar/15

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

Type: Defect Priority: Major
Reporter: Dan Johansson Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None
Environment:

Window7
[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2322"]
java 1.7



 Description   

I'm having problems running an uberjar with [lein-light-nrepl "0.0.18"].
I've tracked the problem down to the filter in cljs.js-deps/goog-resource.
It expects to find goog/deps.js in the google jar file but it is in the uberjar file.



 Comments   
Comment by Dan Johansson [ 29/Sep/14 2:27 AM ]

I use this workaround for now:

(defn load-nrepl []
  (require 'cljs.js-deps)
  
  (alter-var-root (find-var 'cljs.js-deps/goog-resource)
                  (fn [of]
                    (fn [path]
                      (first
                       (enumeration-seq (.getResources (.getContextClassLoader (Thread/currentThread)) path))))))
  
  (require 'lighttable.nrepl.handler))

It just removes the filter.
I guess the filter is there to make sure you get the correct goog/deps.js file? I don't know what assumptions to make about build environments to suggest a generic solution.

Comment by Dan Johansson [ 29/Sep/14 2:28 AM ]

This is the error I get:

Caused by: java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil
	at clojure.core$_cache_protocol_fn.invoke(core_deftype.clj:544)
	at clojure.java.io$fn__8628$G__8610__8635.invoke(io.clj:69)
	at clojure.java.io$reader.doInvoke(io.clj:102)
	at clojure.lang.RestFn.invoke(RestFn.java:410)
	at cljs.js_deps$goog_dependencies_STAR_.invoke(js_deps.clj:241)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.core$memoize$fn__5097.doInvoke(core.clj:5846)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at cljs.js_deps$js_dependency_index.invoke(js_deps.clj:265)
	at cljs.env$default_compiler_env.invoke(env.clj:45)
	at cljs.env$default_compiler_env.invoke(env.clj:42)
	at lighttable.nrepl.cljs__init.load(Unknown Source)
	at lighttable.nrepl.cljs__init.<clinit>(Unknown Source)
	... 52 more
Comment by James Cash [ 29/Sep/14 3:58 PM ]

I was able to work around this same issue by explicitly including the google-closure-library jar in the classpath (i.e. instead of `java -jar my-uber.jar`, `java -cp google-closure-library-$VERSION.jar:my-uber.jar my-uber.core`)

Comment by Taha Siddiqi [ 08/Oct/14 10:19 PM ]

This is what worked for me.

(defn goog-resource [original-fn path]
  (if-let [resource (io/resource path)]
    resource
    (original-fn path)))

(alter-var-root #'cljs.js-deps/goog-resource (fn [f] (partial goog-resource f)))
Comment by David Nolen [ 09/Oct/14 3:35 PM ]

I'd be happy to remove the hack for Google Closure if someone can demonstrate it's no longer needed. If I recall we now remove the empty files from the thirdy party JAR so this shouldn't be a problem anymore.

Comment by Joseph Moore [ 03/Dec/14 11:43 PM ]

Adding the google-closure library jar to my war file in immutant (from lein immutant war) fixed lighttable's nrepl under wildfly as well, thank you James

Comment by David Nolen [ 03/Mar/15 12:05 PM ]

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





[CLJS-1077] analyze-deps infinite recursive loop Created: 02/Mar/15  Updated: 03/Mar/15  Resolved: 03/Mar/15

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

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

Attachments: Text File CLJS-1077.patch    

 Description   

The first arity of analyze-deps unconditionally invokes itself:

https://github.com/clojure/clojurescript/blob/r2913/src/clj/cljs/analyzer.clj#L1102



 Comments   
Comment by David Nolen [ 03/Mar/15 9:23 AM ]

fixed https://github.com/clojure/clojurescript/commit/42a263f7b9ee780417119d8b63f8fed5f892ffda





[CLJS-1076] :nashorn target Created: 02/Mar/15  Updated: 02/Mar/15

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

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


 Description   

To run well on Nashorn the target should supply CLOSURE_IMPORT_SCRIPT as well as setTimeout or setImmediate for core.async.






[CLJS-1073] :dynamic is misspelled as :dyanmic for *target* in core.cljs Created: 02/Mar/15  Updated: 02/Mar/15  Resolved: 02/Mar/15

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

Type: Defect Priority: Trivial
Reporter: Matthew Davidson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, dynamic, target
Environment:

N/A



 Description   

At the top of core.cljs, line 20:

(def ^{:dyanmic true
:jsdoc ["@define {string}"]}
target "default")

I'm not sure there are any consequences so far, since a search doesn't show target being rebound anywhere.



 Comments   
Comment by David Nolen [ 02/Mar/15 2:37 PM ]

thanks https://github.com/clojure/clojurescript/commit/8785fa9c9ff38c0fcebc32757c077070397018b7





[CLJS-1075] Generic inline source map support Created: 02/Mar/15  Updated: 02/Mar/15

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

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


 Description   

Currently hard coded to REPLs. Would simplify jsbin and similar integration.






[CLJS-1074] Externs inference Created: 02/Mar/15  Updated: 02/Mar/15

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

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


 Description   

Given all externs generally need to be supplied for js/foo we could probably automatically compute externs based on js/foo usage in user code. For this to work correctly we need to account for property access through js/foo i.e. (def Bar js/foo.Bar). This should infer that Bar is also a foreign object. Things gets more complicated for higher order cases, we probably want to support a ^js type hint.

Finally externs inference needs to account for externs likely already supplied by the user - i.e. don't emit dupes, Google Closure will complain.






[CLJS-1072] Calling .hasOwnProperty("source") in Clojurescript's string/replace will break with ES6 Created: 01/Mar/15  Updated: 02/Mar/15  Resolved: 02/Mar/15

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

Type: Defect Priority: Major
Reporter: Matthew Davidson Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: bug, cljs, es6, replace, string
Environment:

Firefox Developer Edition
Mac OS X 10.10.1
Clojurescript 0.0-2913 (but I checked, and it's still present in current master branch)



 Description   

The "replace" function in string.cljs identifies RegExp objects by calling .hasOwnProperty to check for the "source" property. If true, then it's a regex. However, in ES6, the "source" property will become part of the RegExp prototype and not the actual object, so .hasOwnProperty("source") will return false instead.

Documentation on the RegExp change is at http://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/source under the "Specifications" section. The change hit Firefox two weeks ago, according to this bug report, https://bugzilla.mozilla.org/show_bug.cgi?id=1120169, so it will still be 2-3 months before it appears in the main release channel. But given that ES6 is aiming for a June release date, we should expect other browsers to show this error soon enough.

To fix the issue, the Javascript "in" operator would work, if there's a way to compile cljs to use it. Otherwise, the next best thing might be to check to see if the "source" property is non-nil. Basically, the test:

(.hasOwnProperty match "source")

could become:

(.-source match)

It's not a major code change at all, but I'm happy to provide a patch and verify tests if you prefer.



 Comments   
Comment by Elliot Block [ 02/Mar/15 12:54 AM ]

Just to slightly summarize/reiterate, this bug means that `string/replace` is always broken if you pass it a RegExp as the `match` (under the new ES6 rules, already live in FF Dev Ed.)

This further breaks plenty of applications, like it immediately breaks the `secretary` router found in many Om apps.

Comment by David Nolen [ 02/Mar/15 7:46 AM ]

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





[CLJS-1064] ex-info is not printable Created: 24/Feb/15  Updated: 01/Mar/15  Resolved: 01/Mar/15

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

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


 Description   
(str (ex-info "foo" {}))

Fails under master.



 Comments   
Comment by David Nolen [ 01/Mar/15 12:26 PM ]

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





[CLJS-1071] Convert symbol keys in :closure-defines to munged strings Created: 01/Mar/15  Updated: 01/Mar/15  Resolved: 01/Mar/15

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 0
Labels: None


 Comments   
Comment by David Nolen [ 01/Mar/15 9:54 AM ]

fixed https://github.com/clojure/clojurescript/commit/1f023467e95cd081851fbb71601bc2dd639ad002





[CLJS-1014] Support Closure Defines under :none Created: 06/Feb/15  Updated: 01/Mar/15  Resolved: 01/Mar/15

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

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


 Description   

Because Google Closure never runs under :none Closure Defines do not work.



 Comments   
Comment by David Nolen [ 06/Feb/15 9:51 AM ]

This is easy to make work with :main. Can probably be made to work without as well?

Comment by David Nolen [ 01/Mar/15 9:24 AM ]

fixed https://github.com/clojure/clojurescript/commit/9d7f4f2a7ba73438cf9f5622ee16dfa3b66d6a9c





[CLJS-1070] top-level boolean inference does not work Created: 28/Feb/15  Updated: 28/Feb/15

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

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


 Description   

Problem for using boolean Closure defines






[CLJS-1068] CLJS_NODE_TARGET define Created: 27/Feb/15  Updated: 28/Feb/15  Resolved: 28/Feb/15

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

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


 Description   

While we have our own means of tracking in the analyzer/compiler this it would be nice to expose a user-land closure define so that users can customize their own code bases now that this is out of scope for conditional reading.



 Comments   
Comment by David Nolen [ 28/Feb/15 12:15 PM ]

fixed https://github.com/clojure/clojurescript/commit/801861cc03d5dc3c783b18a375eec37725e6671b





[CLJS-1069] Generic :jsdoc support Created: 28/Feb/15  Updated: 28/Feb/15  Resolved: 28/Feb/15

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

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


 Description   

Currently :jsdoc support is not generalized for use. This means users cannot easily leverage Google Closure defines.



 Comments   
Comment by David Nolen [ 28/Feb/15 12:14 PM ]

fixed https://github.com/clojure/clojurescript/commit/3f38fb9507a89b44c5890bb18be635944e7aceca





[CLJS-1063] Regression for Rhino REPL part of Quick Start tutorial Created: 24/Feb/15  Updated: 28/Feb/15  Resolved: 28/Feb/15

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

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


 Description   

Curiously bin/repljs appears to work. When trying the tutorial we get a bunch of "already declared" errors which seems like a REPL bootstrapping issue.



 Comments   
Comment by David Nolen [ 28/Feb/15 10:24 AM ]

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





[CLJS-1067] Shared AOT cache for dependencies in JARs Created: 26/Feb/15  Updated: 27/Feb/15

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

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


 Description   

3rd party library code in JARs shouldn't be recompiled across dev and prod configurations. There should be a shared AOT cache for all builds within a project for all non-project local source.






[CLJS-1066] Rhino REPL regression Created: 26/Feb/15  Updated: 26/Feb/15  Resolved: 26/Feb/15

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

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


 Description   

Uncovered by CLJS-1065



 Comments   
Comment by David Nolen [ 26/Feb/15 8:52 AM ]

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





[CLJS-911] Cljs's clojure.string.replace replacement fn takes different args to Clj's clojure.string.replace Created: 17/Dec/14  Updated: 24/Feb/15

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

Type: Defect Priority: Minor
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

ClojureScript 0.0-2411



 Description   

Clojure's `clojure.string.replace` takes a replacement fn which receives args `[[group1 group2 ...]]`.
ClojureScript's `clojure.string.replace` takes a replacement fn which receives args `[group1 group2 ...]` (i.e. & args).

It's my understanding that something like the `clojure.string` ns is intended partly to help pave over superficial API differences like this.

Modding ClojureScript's `string.replace` to match Clojure's behaviour would be trivial, but this would be a breaking change for anyone that's come to rely on the faulty[?] behaviour.

Would you like a patch for this? Can submit for http://dev.clojure.org/jira/browse/CLJS-794 while I'm at it (both involve a change to `clojure.string/replace`).

Thanks!



 Comments   
Comment by Joseph Smith [ 24/Feb/15 2:33 PM ]

+1 fixing this (preferably making the ClojureScript version work like the Clojure version).





[CLJS-1062] Incorrect deftype/defrecord definition leads to complex error messages Created: 24/Feb/15  Updated: 24/Feb/15

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: File deftype-fields.diff    

 Description   

Every once in a while I forget to define fields in deftype/defrecord. In this case compilation fails with a low level error message hard to understand.
Clojure compilation prints a better error message in this case: "No fields vector given."

I propose to backport Clojure error message.






[CLJS-1061] Expose disable-console-print! Created: 23/Feb/15  Updated: 24/Feb/15

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

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


 Description   

enable-console-print! is convenient from some developers on a team who like println to print to the console. However, for others, like myself, it is more desirable to print directly to the REPL (e.g. w/in Emacs) instead of the browser console. On occasion enable-console-print! get's checked and can cause errors that result from calling println before the *print-fn* is set. It then becomes a hassle to either comment out the println or the enable-console-print!. Having a disable-console-print! which could restore the previous *print-fn*, whatever that may have been, would be nice to have.

As it currently stands, one must do this manually, which frankly is kind of awkward and not immediately obvious. It is much like having a light you can turn on but not off unless you cut the main power.



 Comments   
Comment by Thomas Heller [ 24/Feb/15 6:01 AM ]

I think the whole print-fn handling is not optimal. The problem is that each namespace can set it and will override the previous setting and the last one wins. While this is most likely one of "my" namespaces and "my" print-fn it may be someone else's while initializing (loading dependant namespaces). So you might run into issues where some log output goes to console and some goes to util.print or whichever other method the library author decided to use. Some libraries already call (enable-console-print!) as the very first thing their namespace.

What I recently added to shadow-build is an option to make this a compiler option (eg. {:print-fn :console}). Code can just use prn, println and never worry about that *print-fn* might not be set. I think this is a very convenient option that can default to :console and one less hurdle new users may stumble over. I don't need to initialize println in Clojure either.

Maybe this option should make it into core.





[CLJS-1060] simplify nREPL integration Created: 23/Feb/15  Updated: 23/Feb/15

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

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


 Description   

-setup and -teardown need to be elidable. If cljs.env/*compiler* already bound use that. The eval of (ns cljs.user ...) form for auto-import of cljs.repl helpers needs to respect *cljs-ns* and restore it afterwards.






[CLJS-1047] externs checking for js/foo Created: 19/Feb/15  Updated: 23/Feb/15

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

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


 Description   

Worth looking into validating `js/foo` forms again the known externs set. Can probably be done by leveraging the Closure JS Parser.



 Comments   
Comment by Michael Griffiths [ 22/Feb/15 12:03 PM ]

Would you consider making the results of parsing available to tooling (e.g. in cljs.env/*compiler*)? I would use this to add support for autocompletion of js/ forms to CIDER.

Comment by David Nolen [ 23/Feb/15 8:15 AM ]

Definitely open to the idea of exposing this information to other tooling when we get there.





[CLJS-1058] Arities listed in doc output are wrapped in (quote ...) Created: 22/Feb/15  Updated: 23/Feb/15  Resolved: 22/Feb/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

The arities listed in the doc output have an extra (quote ...) wrapping them.

Example:

ClojureScript:cljs.user> (doc +)
-------------------------
cljs.core/+
(quote ([] [x] [x y] [x y & more]))
  Returns the sum of nums. (+) returns 0.
nil


 Comments   
Comment by Mike Fikes [ 22/Feb/15 4:38 PM ]

This is a regression that occurred with the commit for CLJS-1055.

Comment by David Nolen [ 22/Feb/15 11:36 PM ]

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

Comment by Mike Fikes [ 23/Feb/15 6:37 AM ]

Confirmed fixed.





[CLJS-1059] Simple interface wanted to convert cljs forms to js Created: 22/Feb/15  Updated: 23/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Mitchell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: analyzer, compiler

Patch: Code

 Description   

In our project (a clojurescript debugger) we want to convert cljs forms or a sequence of forms into javascript so that they can be executed in the javascript console.

We would like something similar to closure/compile-form-seq (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L308)

However, we need to supply, the namespace requires and locals in an env like this

{:ns {:name "test.core" :requires {(quote gstring) (quote goog.string)}} :locals {}}

This code seems to do what we want.

(defn compile-form-seq
    \"Compile a sequence of forms to a JavaScript source string.\"
    [forms env]
    (env/ensure
    (compiler/with-core-cljs nil
      (fn []
        (with-out-str
            (doseq [form forms]
              (compiler/emit (analyzer/analyze env form))))))))

I am not sure why I need env/ensure.

Would you be able to patch compile-form-seq to provide the needed interface, or suggest what we should be doing.

Thanks
Stu



 Comments   
Comment by Mike Thompson [ 22/Feb/15 10:09 PM ]

Just to be clear:
1. when our debugger is at a breakpoint,
2. the user can type in an expression at the repl
3. in response, our debugger has to compile the user-typed-in expression to javascript (and then execute it, showing a result)
4. taking into account any local bindings. <---- this is the key bit.

To satisfy point 4, our tool extracts all the 'locals' from the current call-frame, and then supplies all these local bindings in env/locals, so the compiler doesn't stick a namespace on the front of them.

For example, if there was a local binding for 'x' in the callstack, and the user's repl-entered-expression involves 'x', then we want the compiler to leave the symbol 'x' alone and to not put some namespace on the front of it. In the final javascript, it must still be 'x', not 'some.namespace.x'

Our method to achieve this is to put 'x' into env/locals when compiling – and it all works. Except, with the recent changes this has become more of a challenge. Hence this ticket asking for a way to pass in env.

Comment by Thomas Heller [ 23/Feb/15 3:19 AM ]

You could wrap the user expression in an fn, that would allow you to skip messing with the locals. The REPL basically does the same trick for *1,*2,...

(fn [x]
  ~user-expression-here)




[CLJS-1030] Add cljs.repl/pst for printing error values Created: 11/Feb/15  Updated: 22/Feb/15  Resolved: 22/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1030-2.patch     Text File CLJS-1030-3.patch     Text File CLJS-1030.patch    

 Description   

This would mirror clojure.repl/pst and leverage the new stacktrace mapping support.



 Comments   
Comment by Mike Fikes [ 11/Feb/15 9:04 PM ]

We now have *e and cljs.repl/IParseStacktrace (as well as optional cljs.repl/IPrintStacktrace) support so the pieces needed to do this appear to exist.

Comment by Mike Fikes [ 12/Feb/15 12:33 AM ]

I investigated: Since the values of interest (*e and others) are in the JavaScript engine, but the REPL has the stack trace parsing/mapping/printing support in the Clojure implementation, one approach is to define a new REPL special function, pst.

I was interested in feedback on whether this strategy makes sense, and if so, I'd be happy to pursue fleshing out a proper patch that supports this, mirroring clojure.repl/pst as much as possible.

Defining cljs.repl/pst as:

(defn pst
  ([repl-env env args] (pst repl-env env args nil))
  ([repl-env env args opts]
    (let [desc-js (comp/emit-str (ana/no-warn (ana/analyze env '(pr-str *e) opts)))
          desc-ret  (-evaluate repl-env "filename" 1 desc-js)
          rst-js (comp/emit-str (ana/no-warn (ana/analyze env '(.-stack *e) opts)))
          rst-ret (-evaluate repl-env "filename" 1 rst-js)
          ret {:value (:value desc-ret) :stacktrace (:value rst-ret)}]
      (display-error repl-env ret nil opts))))

and hooking in an appropriate 'pst in the cljs.repl/default-special-fns implementation yields promising behavior that could be polished:

ClojureScript:cljs.user> (ffirst 1)
Error: 1 is not ISeqable
	 cljs.core/seq (.ambly_jsc_repl/cljs/core.cljs:727:13)
	 cljs.core/first (.ambly_jsc_repl/cljs/core.cljs:736:7)
	 cljs.core/ffirst (.ambly_jsc_repl/cljs/core.cljs:1155:3)
nil
ClojureScript:cljs.user> *e
#<Error: 1 is not ISeqable>
ClojureScript:cljs.user> (pst)
#<Error: 1 is not ISeqable>
	 cljs.core/seq (.ambly_jsc_repl/cljs/core.cljs:727:13)
	 cljs.core/first (.ambly_jsc_repl/cljs/core.cljs:736:7)
	 cljs.core/ffirst (.ambly_jsc_repl/cljs/core.cljs:1155:3)

Another approach I tried, but failed with, is to mimic the way doc works; this fails because it doesn't allow you access to the REPL environment.

Comment by David Nolen [ 12/Feb/15 5:53 AM ]

Yeah I don't see how it can easily be done outside of a special function. Happy to see a patch along these lines.

Comment by Mike Fikes [ 14/Feb/15 9:50 PM ]

Patch attached.

Here is a sample run in the Ambly REPL (long paths elided for readability) illustrating exercising various arities:

ClojureScript:cljs.user> (ffirst 1)
Error: 1 is not ISeqable
	 cljs.core/seq (.../cljs/core.cljs:727:13)
	 cljs.core/first (.../cljs/core.cljs:736:7)
	 cljs.core/ffirst (.../cljs/core.cljs:1155:3)
nil
ClojureScript:cljs.user> (pst)
Error: 1 is not ISeqable
	 cljs.core/seq (.../cljs/core.cljs:727:13)
	 cljs.core/first (.../cljs/core.cljs:736:7)
	 cljs.core/ffirst (.../cljs/core.cljs:1155:3)

ClojureScript:cljs.user> (pst 2)
Error: 1 is not ISeqable
	 cljs.core/seq (.../cljs/core.cljs:727:13)
	 cljs.core/first (.../cljs/core.cljs:736:7)

ClojureScript:cljs.user> (def foo *e)
#<Error: 1 is not ISeqable>
ClojureScript:cljs.user> (pst foo)
Error: 1 is not ISeqable
	 cljs.core/seq (.../cljs/core.cljs:727:13)
	 cljs.core/first (.../cljs/core.cljs:736:7)
	 cljs.core/ffirst (.../cljs/core.cljs:1155:3)

ClojureScript:cljs.user> (pst foo 1)
Error: 1 is not ISeqable
	 cljs.core/seq (.../cljs/core.cljs:727:13)
Comment by Mike Fikes [ 15/Feb/15 8:41 AM ]

I'm going to submit a simpler patch that employs (.-message e) eliminating the additional function that strips the unreadable bits off.

Comment by Mike Fikes [ 15/Feb/15 9:00 AM ]

Simpler patch that uses (.-message e) and additionally strips description off front of stacktrace if it is already there (otherwise it appears twice in Node REPL).

Comment by Mike Fikes [ 15/Feb/15 9:22 AM ]

I upgraded to Node v0.12.0 and the patches in this ticket are somewhat thwarted by the same kind of stuff described at the bottom of CLJS-1018.

Comment by David Nolen [ 15/Feb/15 9:27 AM ]

Mostly looks good but even with the second patch I still see the error twice at the top. I tested using rlwrap ./scripts/noderepljs in the project repo.

Comment by Mike Fikes [ 15/Feb/15 9:34 AM ]

Right. The code to eliminate the double message worked under the previous version of node. Under Node v0.12.0 the (.-message e) is no longer strictly the first part of what is in the stack trace text.

For this aspect, it makes me feel that, just like parsing of the stack trace into a canonical vector form is delegated to REPLs, the same is perhaps the right approach here for the message part. Dunno yet, but it is feeling like a bit of a mess with the Node v0.12.0 error indicator appearing in parts of the message and stack trace making naive processing fragile.

I have no problem holding off on this patch if your opinion is that this needs to be better addressed. (We just need to sort out where to address it.)

Comment by Mike Fikes [ 15/Feb/15 11:58 AM ]

Third patch eliminates the doubling in Node.js REPL. The way it accomplishes this is to synthesize the first part of what a Node.js stack looks like and to remove it if it finds a match.

I've tested this on Node v0.12.0 and v0.10.25, as well as Ambly, and it works.

The negative is that this essentially has a little of Node v0.12.0 aspects in the base ClojureScript real code.

Comment by David Nolen [ 22/Feb/15 12:45 PM ]

fixed https://github.com/clojure/clojurescript/commit/5218a4f4d88db4483eefea80da66acfac20cc907





[CLJS-1055] cljs.repl/doc should support namespaces and special forms Created: 22/Feb/15  Updated: 22/Feb/15  Resolved: 22/Feb/15

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

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


 Comments   
Comment by David Nolen [ 22/Feb/15 10:11 AM ]

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





[CLJS-1054] add clojure.repl/source functionality to cljs.repl Created: 22/Feb/15  Updated: 22/Feb/15  Resolved: 22/Feb/15

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

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


 Comments   
Comment by David Nolen [ 22/Feb/15 9:09 AM ]

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





[CLJS-1053] REPLs need import special fn Created: 22/Feb/15  Updated: 22/Feb/15  Resolved: 22/Feb/15

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

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


 Comments   
Comment by David Nolen [ 22/Feb/15 8:21 AM ]

fixed https://github.com/clojure/clojurescript/commit/4bf09f3232a77856aa46944b5814af06348b9c77





[CLJS-1038] Namespace aliasing does not appear to work in the built in browser REPL Created: 14/Feb/15  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

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


 Description   

When creating a simple "Hello world" style project, requiring and aliasing the main namespace does not appear to work. Warnings are emitted about the namespace not existing. Sneaking suspicion that this is a code path issue for browser REPL - it's the oldest REPL outside of Rhino and has received the fewest updates.



 Comments   
Comment by David Nolen [ 21/Feb/15 10:11 AM ]

Cannot replicate this on master





[CLJS-998] Nashorn REPL does not support require special fn Created: 02/Feb/15  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

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


 Description   

Need to monkey patch goog.require as we have done elsewhere.



 Comments   
Comment by David Nolen [ 21/Feb/15 9:42 AM ]

fixed https://github.com/clojure/clojurescript/commit/3d8c29ebcc38768b2c1a837fafb09d5baead3078





[CLJS-1052] Cannot require ns from within the ns at the REPL for reloading purposes Created: 21/Feb/15  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

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


 Comments   
Comment by David Nolen [ 21/Feb/15 9:28 AM ]

fixed https://github.com/clojure/clojurescript/commit/3a856c83e8c1c4b1363f4c0ff0803646aec1385b





[CLJS-975] preserve :reload & :reload-all in ns macro sugar Created: 13/Jan/15  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

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


 Description   

:include-macros true and :refer-macros [...] are both sugar forms supported in :use and :require, they expand into :use-macros or :require-macros specs. We should make sure to preserve :reload and :reload-all when desugaring.



 Comments   
Comment by Thomas Heller [ 13/Jan/15 10:48 AM ]

Oops, didn't see there was an issue for this.

Sorry for the rant but please note my comment on:
https://github.com/clojure/clojurescript/commit/e408305344ef7b3658118377e530c78c2eb93cf3

I do not think REPL related "options" have anything to do in the parse function of the namespace. I outlined a different approach which I hope you'll consider. Of course all of this is motivated by shadow-build, so YMMV.

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

While shadow-build is an interesting piece of technology it's not relevant for tickets like this. Please keep threads clear of non-issues.

Comment by David Nolen [ 21/Feb/15 8:51 AM ]

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





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

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: 1
Labels: None


 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






[CLJS-785] :refer-macros in conjunction with :refer not working Created: 17/Mar/14  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

Type: Defect Priority: Minor
Reporter: Kevin Marolt Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None


 Description   

The :refer-macros directive isn't working when used in conjunction with the :refer directive. Compiling a ns-form like

(ns foo
  (:require
    [bar :refer [baz] :refer-macros [quux]]))

produces the compiler error

Each of :as and :refer options may only be specified once in :require / :require-macros; offending spec: (bar :refer [baz] :refer [quux])

The problem seems to be with analzyer.cljs/desugar-ns-specs. Invoking

(desugar-ns-specs '((:require [bar :refer [baz] :refer-macros [quux]])))

returns

'((:require-macros (bar :refer [baz] :refer [quux])) (:require (bar :refer [baz])))

instead of

'((:require-macros (bar :refer [quux])) (:require (bar :refer [baz])))

Furthermore, there seems to be a typo in the local remove-sugar function on line 1094 [1]: It should probably be

(let [[l r] (split-with ...)] ...) ;; no '&' before 'r'

instead of

(let [[l & r] (split-with ...)] ...)

Otherwise, something like

(desugar-ns-specs '((:require [bar :include-macros true :as b])))

becomes

((:require-macros (bar :as b)) (:require (bar)))

instead of

((:require-macros (bar :as b)) (:require (bar :as b)))

[1] https://github.com/clojure/clojurescript/blob/78d20eebbbad17d476fdce04f2afd7489a507df7/src/clj/cljs/analyzer.clj#L1094



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

Is this still a problem in master? Thanks.

Comment by David Nolen [ 21/Feb/15 7:50 AM ]

this problem no longer exists in master





[CLJS-579] Use leiningen to handle clojurescript dependency resolution && classpath string generation Created: 28/Aug/13  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 0001-let-lein-handle-classpath-string-generation-dependen.patch    

 Description   

After every bump in one of clojurescript's dependency, it is needed to manually run script/clean && script/bootstrap in order to use the new versions.

By leveraging `lein classpath`, we can automate this process removing the need for any manual intervention.



 Comments   
Comment by Nicola Mometto [ 28/Aug/13 7:45 PM ]

The following patch also removes script/bootstrap script/clean and script/test-compile since those files are no more relevant.

I haven't tested the batch version of the scripts since I don't have windows on my machine, it would be a good idea to have somebody test those before merging.

Comment by David Nolen [ 04/Sep/13 11:07 PM ]

Is there a revised minimal patch? Thanks!

Comment by Nicola Mometto [ 05/Sep/13 6:10 AM ]

http://dev.clojure.org/jira/browse/CLJS-578 only adds project.clj, is this what you're asking for?

Comment by David Nolen [ 21/Feb/15 7:45 AM ]

We have the project.clj for people that want to use it. The bootstrap approach while manual removes the requirement that you need Maven or Lein.





[CLJS-353] Use different primitives for array access and property/object access Created: 09/Aug/12  Updated: 21/Feb/15

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

Type: Defect Priority: Trivial
Reporter: Raphaël AMIARD Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-353-Use-different-primitives-for-array-access-a.patch     Text File 0001-CLJS-353-Use-different-primitives-for-array-access-a.patch    

 Description   

In javascript, array access (array[index]) and property access (object["property"]) have the same syntax, hence aget/aset are used for both in the core library.

But this isn't true for every target language. For example in Lua, there are no arrays, so if you want to have an array like container, array access will need to go through a function.

This patch proposes to add new builtins : oget and oset (for obj-get obj-set) and use them where appropriate. The generated code will be the same for javascript, but will enable alternate backend implementers to treat both differently



 Comments   
Comment by Raphaël AMIARD [ 09/Aug/12 7:24 AM ]

All test pass

Comment by David Nolen [ 09/Aug/12 5:26 PM ]

Not sure I like the names oget/set. How about obj-get obj-set?

Comment by Raphaël AMIARD [ 10/Aug/12 4:08 AM ]

fine by me, added a new patch !

Comment by David Nolen [ 10/Aug/12 11:07 AM ]

The patches are identical.





[CLJS-494] -lookup method call inside get macro and keyword invoke Created: 10/Apr/13  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

Type: Defect Priority: Minor
Reporter: Daniel Skarda Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

ClojureScript from githhub



 Description   

I noticed that -lookup method is called incorrectly at two places

There are two -lookup methods defined for ILookup:

cljs/cljs/core.cljs:198
(defprotocol ILookup
(-lookup [o k] [o k not-found]))

clj/cljs/core.clj:426
(defmacro get
([coll k]
`(-lookup ~coll ~k nil))
([coll k not-found]
`(-lookup ~coll ~k ~not-found)))

As you see, macro (get foo bar) never calls first method. In first case it supplies default argument (instead calling method with 2 arguments).

Same for IFn and keywords:

(deftype Keyword [k]
IFn
(invoke [_ coll]
(when-not (nil? coll)
(let [strobj (.-strobj coll)]
(if (nil? strobj)
(-lookup coll k nil)
(aget strobj k)))))



 Comments   
Comment by Jozef Wagner [ 12/Apr/13 3:37 AM ]

Note that 2 argument get guarantees to return nil if the key is not found. This differs from e.g. 2 argument nth, which should throw if key is not found (index is out of bounds). If 2 argument -lookup does not guarantee to return nil when key is not found, we should keep it as it is.

Comment by Michał Marczyk [ 12/Apr/13 5:13 AM ]

get insisting on nil is definitely by design, as explained by Jozef. So, there is no bug here.

As a matter of style it might be good to have get-the-function and get-the-macro do exactly the same thing; the question remains whether we should add an explicit nil to the -lookup call in get-the-function or remove the nil from the macro and just rely on -lookup to do the right thing. I'd probably go for the former to eliminate one unnecessary indirection (described below).

About -lookup "doing the right thing": it certainly seems to me that -lookup's contract is that of get, even if this is not explicitly documented; also, in almost all instances the binary -lookup delegates to ternary -lookup or ternary -nth (with nil supplied as the final argument), the exception being TransientHashMap's -lookup which basically has that call inlined.

Note that the binary -lookup is called is also called in some other places in core.cljs.

Comment by David Nolen [ 14/Apr/13 5:57 PM ]

Yes the main idea behind inlining get into -lookup was because they do essentially the same thing and we can remove a level of indirection. Similarly I'd prefer that we add the nil to -lookup to avoid the the indirection if not given a not-found value.

Comment by David Nolen [ 21/Feb/15 7:42 AM ]

This is not a bug.





[CLJS-452] clojure.browser.net: enable WebSockets? Created: 31/Dec/12  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Linus Ericsson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement
Environment:

clojurescript in browsers, wrapper for Google Closures WebSocket.


Attachments: Text File 0001-enabled-websockets.patch    
Patch: Code

 Description   

In https://github.com/clojure/clojurescript/blob/master/src/cljs/clojure/browser/net.cljs there's a nicely prepared support for WebSockets with the note

;; WebSocket is not supported in the 3/23/11 release of Google
;; Closure, but will be included in the next release.

is there anything preventing us from enable the support for WebSockets with the next release of ClojureScript?

patch 0001-enable-websockets.patch do just enable the functionality. No test-cases included.



 Comments   
Comment by David Nolen [ 20/Jan/13 12:51 AM ]

Have you verified that this doesn't break browser REPL? Thanks!

Comment by David Nolen [ 21/Feb/15 7:39 AM ]

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





[CLJS-450] (ns) within (do) inconsistent with Clojure behaviour Created: 27/Dec/12  Updated: 21/Feb/15

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

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


 Description   

An (ns) within a (do) doesn't quite work as expected at the REPL:

ClojureScript:cljs.user> (do (ns foo) (def x 42))   
nil
ClojureScript:foo> x
nil
ClojureScript:cljs.user> cljs.user/x
42

The Clojure equivalent:

user=> (do (ns foo) (def x 42))
#'foo/x
foo=> x
42


 Comments   
Comment by David Nolen [ 05/Jan/13 2:05 PM ]

Looks like we need to do something similar to what is done in Clojure with top level do - http://github.com/frenchy64/typed-clojure/pull/4





[CLJS-449] stack traces for ex-info Created: 23/Dec/12  Updated: 21/Feb/15  Resolved: 21/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Tom Jack Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File 0001-hack-ex-info-stacktraces.patch    

 Description   

Currently, ex-info exceptions have no stack traces, which makes them.. much less useful.

Attached patch (0001-hack-ex-info-stacktraces.patch) is one hackish way of getting stack traces back when Error.captureStackTrace is available (at least in Chrome, presumably in Node as well).

But this seems unacceptable — all the stuff emitted for the deftype (making the constructor print as a type) gets overwritten.

Is there a better way?

Will this maybe be made irrelevant by source maps?



 Comments   
Comment by David Nolen [ 23/Dec/12 11:51 AM ]

Isn't the stack trace recoverable from the original exception?

Comment by Tom Jack [ 23/Dec/12 4:01 PM ]

I hadn't considered that. If a cause is provided, that seems reasonable. The only trouble is that you still have to know where the ExceptionInfo is being thrown, so that you can catch it and throw the cause. Chrome's debugger would make it easy to at least find where the ExceptionInfo is thrown, but it's still more trouble than having the appropriate stack trace directly on the ExceptionInfo.

If no cause is provided, of course that won't work. But I suppose one could write (ex-info msg data (js/Error.)) instead of (ex-info msg data)? Personally, I don't expect to pass a cause very often — I expect the binary arity to be much more common in my cljs libraries.

If we rely on the cause for the stack trace, maybe we could have the cause default to a new Error if not supplied? It seems sort of conceivable that we could also wire up the ExceptionInfo to proxy .stack to that Error so that the stacktrace will come through, without needing to override the ExceptionInfo constructor.

Comment by David Nolen [ 03/Jan/15 6:12 PM ]

Sorry for letting this one languish, it appears stack traces just work under Node.js. However that doesn't seem to be the case in browsers. I would take a rebased patch, thanks.

Comment by David Nolen [ 21/Feb/15 7:11 AM ]

CLJS-985





[CLJS-1051] :modules validation, :output-to should be unique Created: 20/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

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


 Comments   
Comment by David Nolen [ 20/Feb/15 8:26 PM ]

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





[CLJS-1039] Under Emacs source directory watching triggers spurious recompilation Created: 14/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

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


 Description   

Emacs is noisy when it comes to the writing disk. We currently filter out only JS and CLJS files but we probably need to do a little more.



 Comments   
Comment by David Nolen [ 20/Feb/15 4:58 PM ]

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





[CLJS-1036] cljs.closure/build does not find upstream dependencies when called from worker thread Created: 13/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Major
Reporter: Michael Griffiths Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: cljs, closure, compiler

Attachments: Text File CLJS-1036--001.patch    

 Description   

Example stacktrace: https://www.refheap.com/97198 (context is using figwheel in a clojure REPL from CIDER)

Because cljs.closure/build calls the 0-arity form of get-upstream-deps, it is implicitly using the current thread's classloader to find the deps.cljs resources. It is then assoc'ing the result into opts, so the caller has no way of gathering the dependencies themselves and passing them in.



 Comments   
Comment by Michael Griffiths [ 13/Feb/15 1:40 PM ]

Patch CLJS-1036--001.patch, attached, uses merge instead of assoc to allow passing of the upstream dependency data via opts.

Comment by Michael Griffiths [ 13/Feb/15 1:45 PM ]

We could also allow passing of a :resources-classloader opt so callers don't have to gather the dependencies themselves, if you'd prefer a patch that does that.

Comment by Bruce Hauman [ 15/Feb/15 2:17 PM ]

This is a problem with calling cljs.closure/build inside nREPL in general when upstream deps are involved.

Comment by David Nolen [ 15/Feb/15 2:34 PM ]

I would like Chas Emerick to chime in on this thread before considering it.

Comment by Chas Emerick [ 16/Feb/15 9:02 AM ]

Could someone point me to a simple (sample?) project that uses upstream dependencies? I haven't used them yet, and don't have such a thing handy.

The proposed patch is confusing to me; if the problem is in resolving dependencies due to classloader state/configuration, how does it help to give opts the opportunity to clobber what get-upstream-deps returns? That is, doesn't this just move the problem downstream to who/whatever is calling build?

Comment by Michael Griffiths [ 16/Feb/15 10:10 AM ]

Chas: yes, the proposed patch simply moves the responsibility to the caller. I was under the assumption that changing how get-upstream-deps/build resolves the classloader by default would be too-breaking of a change, and had not even considered the fact that this might be a broader issue with nREPL itself.

Example usage of get-upstream-deps in the wild (that even mentions classloader issues): https://github.com/immutant/immutant-release/blob/ea538feb548bde86ebce20ec679da7a19b639259/integration-tests/apps/ring/basic-ring/src/basic_ring/core.clj#L51

Note that Weasel (at least) is currently gathering the upstream deps itself too: https://github.com/tomjakubowski/weasel/commit/5cd7009f584100f0d89fc7f00079458bb1f2c016

I'll set up an example project tonight if you like, but I think all you need is to add [cljsjs/react "0.12.2-5"] as a dependency to an existing cljs project and then require [cljsjs.react] in one of its namespaces.

Comment by Bruce Hauman [ 16/Feb/15 1:04 PM ]

Chas, David here is a very quick example:

If you do a

lein new om-mies hello-world
cd hello-world
lein repl
user> (require '[cljs.closure :as cc])
user> (cc/build "src" {})

Assuming lein repl started an nREPL repl. This will produce the error mentioned above. The problem is that om is utilizing the new deps.js facility to autmatically import cljsjs.react. get-upstream-deps depends on being run on the main thread.

https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/closure.clj#L1116

A solution that fixes this in get-upstream-deps* would be preferable.

Is this simple solution to use the system classloader when not on the main thread:

(if (not= (.getName (Thread/currentThread)) "main")
  (java.lang.ClassLoader/getSystemClassLoader)
  (. (Thread/currentThread) (getContextClassLoader)))

To naive? My experience with Java threads and classloaders is minimal.

Or should we always use the (java.lang.ClassLoader/getSystemClassLoader) in this case?

Comment by Chas Emerick [ 17/Feb/15 3:35 PM ]

Using the system classloader isn't necessary. get-upstream-deps is currently using .findResources, not .getResources; the difference is that the former looks only for matching resources in the target classloader, while the latter first delegates to the parent classloader, and only looks locally if nothing is found there. So, just using .getResources instead will make all current usage work as expected, I think. e.g.

;; `lein repl` nREPL here
user=> (enumeration-seq (.getResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
(#<URL jar:file:/home/chas/.m2/repository/cljsjs/react/0.12.2-5/react-0.12.2-5.jar!/deps.cljs>)
user=> (enumeration-seq (.findResources (. (Thread/currentThread) (getContextClassLoader))  "deps.cljs"))
nil

There are some classloader edge cases that might motivate making get-upstream-deps perform a more comprehensive search, explicitly pinging all classloaders for matching resources. This is sometimes necessary for these sorts of scanning scenarios if e.g. a deps.cljs file is available via a parent classloader, which will therefore "shadow" other deps.cljs files in child classloaders. In that case, you need something like cemerick.pomegranate/resources.

Hope that helps.

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

Chas's suggested fix has been committed to master. It works on the simple tests I've run but I'd like to hear more confirmation. Thanks everyone.

https://github.com/clojure/clojurescript/commit/79208f5bf15825a2ba2d0ce95aae6d2b71966494

Comment by David Nolen [ 20/Feb/15 4:36 PM ]

Closing this one. Chas's solution is the correct one.





[CLJS-1045] cljs.repl/repl should mirror the expectations and behavior of the standard Clojure REPL as closely as possible Created: 17/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

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


 Description   

All the differences simply creates trouble and needless indirection for other tooling.



 Comments   
Comment by David Nolen [ 17/Feb/15 4:20 PM ]

Worth investigating if we can't implement cljs REPLs on top the Clojure REPL as suggested by Chas Emerick.

Comment by David Nolen [ 17/Feb/15 8:31 PM ]

Not going to reuse clojure.main/repl it's more trouble than it's worth and we need more options and other contextual things. However the option set that clojure.main/repl accepts is simple, clean and well considered. Happy to make this set the meeting place for the default behavior and customizations needed by tools like nREPL.

Comment by David Nolen [ 20/Feb/15 4:36 PM ]

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





[CLJS-960] On carriage return REPLs should always show new REPL prompt Created: 04/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

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


 Comments   
Comment by Bruce Hauman [ 10/Feb/15 8:55 AM ]

You could delegate this to IJavaScriptEnv's. I solved this in the FigwheelEnv.

I have to wait for a connection which is a valid wait, and it should wait for as long as it takes for a connection to connect. This happens both on -evaluate and -setup.

But then there is the case where an -evaluate fails to respond. In this case I just have an 8 second timeout to return control to the REPL.

Comment by David Nolen [ 20/Feb/15 3:02 PM ]

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





[CLJS-989] ClojureScript REPL loops on EOF signal Created: 27/Jan/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None
Environment:

ClojureScript 2740


Attachments: Text File CLJS_989.patch    

 Description   

If you run the cljs.repl outside of clojure.main and you CTRL+D, the REPL will loop infinitely



 Comments   
Comment by Bruce Hauman [ 27/Jan/15 3:45 PM ]

Here's a simple patch that fixes it.

Comment by David Nolen [ 20/Feb/15 3:02 PM ]

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





[CLJS-1046] static vars do not respect user compile time metadata Created: 18/Feb/15  Updated: 20/Feb/15  Resolved: 20/Feb/15

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

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

Attachments: Text File 1046.patch     Text File 1046-upd0.patch    

 Description   

We should probably generalize the :test function value support.



 Comments   
Comment by Ivan Mikushin [ 19/Feb/15 7:12 AM ]

I've submitted a patch.

Comment by David Nolen [ 19/Feb/15 7:35 AM ]

Thanks! Have you submitted your CA?

The patch looks good, it doesn't address the case where a user may accidentally include function values but this can be addressed separately in an enhancement ticket.

Comment by Ivan Mikushin [ 19/Feb/15 7:47 AM ]

I've signed the CA before uploading the patch.

Comment by Ivan Mikushin [ 19/Feb/15 8:20 AM ]

I'll add the ticket later today. Can you give me a pointer where to look for the current :test function value support?

Comment by David Nolen [ 19/Feb/15 8:52 AM ]

It will need to be handled in def analyzer case as that's the environment function values must be interpreted in. See https://github.com/clojure/clojurescript/blob/6019e4908b6643cf1e9dd2c403f9bfdf15ab7495/src/clj/cljs/analyzer.clj#L764

Comment by Ivan Mikushin [ 20/Feb/15 6:07 AM ]

CLJS-1048

Comment by Ivan Mikushin [ 20/Feb/15 7:25 AM ]

rebase the patch and rm some noise

Comment by David Nolen [ 20/Feb/15 8:10 AM ]

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

Ivan, I did not use the supplied patch as is because it leaked internal analyzer and compiler metadata into user space. I did copy over the supplied tests.

Let me know if this works for you thanks.

Comment by Ivan Mikushin [ 20/Feb/15 1:21 PM ]

Awesome, thanks! It works great.

Comment by David Nolen [ 20/Feb/15 1:37 PM ]

Ivan glad to hear it! Thanks for the confirmation.





[CLJS-1050] Warn on code that will obviously prevent dead code elimination or cross module code motion Created: 20/Feb/15  Updated: 20/Feb/15

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

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


 Description   

Might make sense to support a module level flag like *warn-on-reflection*, perhaps *warn-on-deoptimization*? Simple cases of this are top-level data structures and and top-level side effects.






[CLJS-1048] support function values in static vars compile time metadata Created: 20/Feb/15  Updated: 20/Feb/15

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

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


 Description   

Function values are currently only supported for :test metadata key as a special case.






[CLJS-941] Warn when a symbol is defined multiple times in a file Created: 31/Dec/14  Updated: 19/Feb/15  Resolved: 19/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 941-fix2.patch     Text File 941-fix.patch     Text File 941.patch    

 Description   

The only reason Clojure allows vars to be re-def-ed is so you can fix bugs in running programs.
- Rich Hickey

(ns example.core)
(defn foo [] "hi")
(defn foo [] "hello")

;; WARNING: foo at line 2 is being replaced at line 3 src/example/core.cljs

;; The warning only applies to files, not REPLs.
;; Also ignoring the "cljs" namespace since many symbols are redefined there.



 Comments   
Comment by Shaun LeBron [ 31/Dec/14 11:53 AM ]

Included a short patch. Is ignoring the entire `cljs` namespace a good idea? I noticed a lot of redefs in there.

Comment by David Nolen [ 31/Dec/14 12:36 PM ]

What is the intent of the cljs-file check? Also the exclusion of core is not desirable. I looked at the warnings and most of them are legitimate problems in cljs.core with the exception of ITER_SYMBOL and imul. These two are problematic cases for this patch as they represent a real useful pattern I expect to appear in many ClojureScript codebases.

I think the warning should only be emitted if the defs are not embedded in a conditional expression.

Comment by Shaun LeBron [ 31/Dec/14 2:05 PM ]

updated patch. The cljs-file check was an attempt to prevent the warning when redefining symbols from a REPL. But I just changed it to check if it's "<cljs repl>".

I'll look into preventing the warning for defs in conditional expressions.

Should we correct the duplicated function definitions in cljs.core in a separate patch?

Comment by Shaun LeBron [ 31/Dec/14 2:09 PM ]

the updated patch also prevents this warning if a `declare` comes after a `def`. No harm there.

Comment by David Nolen [ 31/Dec/14 2:15 PM ]

I'd rather just have a single squashed patch. Agree that declare after def is harmless and this is in line with the behavior of Clojure.

Comment by Shaun LeBron [ 31/Dec/14 4:14 PM ]

updated patch:

  • removed the redefs for cljs.core [sequence rand rand-int] that triggered redef warnings
  • allow redefs inside an if-block for conditional def'ing (using a dynamic var allow-redef)
Comment by David Nolen [ 31/Dec/14 4:27 PM ]

fixed https://github.com/clojure/clojurescript/commit/378a75e4b812ebc5a3596bc94d1afd3f6fb1506f

Comment by David Nolen [ 31/Dec/14 4:42 PM ]

Shaun, reopening as I had to disable this because it still interacts badly with REPLs. To see the problem reenable the warning and follow the instructions here: http://swannodette.github.io/2014/12/31/the-old-way-the-new-way/

Comment by Shaun LeBron [ 31/Dec/14 6:18 PM ]

updated patch.

I saw the warnings for every symbol after starting the rhino REPL. It was compiling the same files from local maven and then ".repl/" for some reason, triggering the redef warnings.

To fix, I just added a check to trigger the warning only if we're overriding a symbol from the same file path, which is the only case we want to consider anyway.

Comment by David Nolen [ 31/Dec/14 6:48 PM ]

Thanks for the extra information, will look into why an extra compile is being triggered.

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

fixed https://github.com/clojure/clojurescript/commit/9a5454c68c6202a732a5b0220120bde63da9bd94

Comment by David Nolen [ 01/Jan/15 1:21 PM ]

I've had to disable this again in master. Running the Rhino REPL from a checkout still spills warnings. Before turning this back on it's going to need a lot more testing under the various REPLs and incremental build scenarios.

Comment by Shaun LeBron [ 01/Jan/15 6:25 PM ]

Was able to reproduce. repl-rhino is now binding relative file paths to cljs-file for some reason, causing the filename inequality check to fail.

A general solution is for the analyzer to uniquely identify a file "session", so redefs emit warnings within a session, but not across them to allow redefs from the REPL or file-reloads.

This patch binds a new cljs-file-session var to a timestamp alongside cljs-file. Seems to have worked for the following cases:

  • tested in repl.rhino
  • tested in repl.node
  • tested in figwheel
  • tested in weasel
Comment by Andy Fingerhut [ 02/Jan/15 10:46 AM ]

Eastwood only works for Clojure on the JVM right now, and includes a warning like this because they seemed unwilling to include such a thing in Clojure itself, particularly due to interactive reloading of files during many people's typical development workflows. ClojureScript might be enough different that it makes more sense to include it in the compiler, but the number of times it has been undone makes me wonder.

Comment by Shaun LeBron [ 02/Jan/15 12:05 PM ]

I think since the analyzer functions work in sessions (i.e. one file at a time, or form at time in REPL), we can get the benefit of this warning in the compiler itself by preventing redefs on a session basis. Just took me a few tries to realize that.

Would be nice as a default capability in the compiler since its expected behavior for most compilers to detect this. Thanks for insight on Clojure and Eastwood!

Comment by David Nolen [ 03/Jan/15 2:04 PM ]

Shaun the patch is interesting but I'm concerned about using System/currentTimeMillis for the identifier. One idea that's been rolling around my head for some time is parallel compilation. This is not going to work for that. Is there any reason not to use a proper sentinel?

Comment by Shaun LeBron [ 03/Jan/15 6:09 PM ]

good point, updated fix2 patch:

  • use a uuid instead of timestamp for the file session
  • for extra caution, don't warn redefs if file session is unbound (nil)
Comment by David Nolen [ 10/Feb/15 11:44 PM ]

Can we get a patch rebased on master and can we use a proper sentinel instead of UUID? i.e. (Object.). Thanks.

Comment by Shaun LeBron [ 11/Feb/15 1:35 AM ]

updated 941-fix2.patch to use a (Object.) for sentinel and rebased on master.

Comment by David Nolen [ 11/Feb/15 12:33 PM ]

Looks like this patch needs to be rebased to master again.

Comment by Shaun LeBron [ 11/Feb/15 2:01 PM ]

rebased patch.

I tested this rebased patch against a "mies" project, and it is failing after a second compile with an "Unreadable form" exception in the project's core.cljs. I will have to look at this tonight.

Comment by Shaun LeBron [ 12/Feb/15 1:26 AM ]

Problem: the analyzer was caching the session sentinels as edn. The cache couldn't be read back correctly since they were Object literals.

Solution: I updated the patch to dissoc the file session sentinels from the analysis before caching.

Details: We don't need to cache the file session ids because they don't need to exist after a session is complete. They simply need to be different from the current session id, so nil meets this requirement. If the current session is nil for some reason, then we ignore redefs.

Comment by David Nolen [ 12/Feb/15 8:01 AM ]

I don't understand why *file-session* ever made it into analysis information. Why isn't this being done just purely via dynamic binding? As far as I can tell there is no reason to capture this ever. i.e. what's wrong with an atom containing a set that you dynamically bind around the body of cljs.analyzer/analyze-file and cljs.compiler/compile-file that simply tracks ns defs?

Comment by Shaun LeBron [ 12/Feb/15 8:17 PM ]

Yeah, I agree; I kind of rube-goldberg'd this one. Will update the patch when I get a chance, hopefully tonight.

Comment by Shaun LeBron [ 13/Feb/15 5:11 PM ]

updated. using file-defs to hold the set atom, bound in the body for analyze-file and compile-file.

Comment by David Nolen [ 18/Feb/15 7:04 PM ]

Sorry can we get a rebased patch? The patch looks great and will apply right away this time.

Comment by Shaun LeBron [ 19/Feb/15 10:30 AM ]

np, rebased the patch and tested it

Comment by David Nolen [ 19/Feb/15 10:46 AM ]

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





[CLJS-1042] Google Closure Modules :source-map support Created: 15/Feb/15  Updated: 18/Feb/15  Resolved: 18/Feb/15

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

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


 Description   

Should be pretty straightforward but means we probably want to make cljs.closure/emit-optimized-source-map a bit more generic so that it may be reused for this ticket.



 Comments   
Comment by David Nolen [ 18/Feb/15 7:02 PM ]

fixed https://github.com/clojure/clojurescript/commit/4dbf8c8723f0702be46546a036f152dcaa0598f1





[CLJS-1041] Google Closure Modules :foreign-libs support Created: 15/Feb/15  Updated: 16/Feb/15  Resolved: 16/Feb/15

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

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


 Description   

We should probably just move :foreign-libs into :cljs-base. If a user wants to lock a foreign library to a module they can add it to that module's :entries. If multiple modules need to share some subset of :foreign-libs these should be lifted manually into their own module.



 Comments   
Comment by David Nolen [ 16/Feb/15 9:37 PM ]

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





[CLJS-1044] Investigate Closure CommonJS Support Created: 16/Feb/15  Updated: 16/Feb/15

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

Type: Task Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

It appears Google Closure has basic support for CommonJS modules. Apparently mixing CommonJS and Closure style libraries seems somewhat supported but this is not a necessary goal. For our purposes it may be satisfactory to simply allow CommonJS libs to be a part of the build, likely as a separate pass, in which require is compiled away.






[CLJS-1043] top-level locals issues Created: 16/Feb/15  Updated: 16/Feb/15

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

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


 Description   
(let [x #js {"Foo" #js {"Bar" (fn [])}}
      y (new x.Foo.Bar.)])

x will get renamed and not considered a local. This seems like a fundamental problem with whatever top level let special casing we may have in place.






[CLJS-968] Metadata on function literal inside of a let produces invalid Javascript Created: 07/Jan/15  Updated: 16/Feb/15

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: bug
Environment:

Originally found with [org.clojure/clojurescript "0.0-2496"]
Still reproducible with the latest cljsc (b5e9a5116259fc9f201bee4b9c6564f35306f9a5)



 Description   

Here is a minimal test case that produces the invalid Javascript:

(defn f []
  (let [a 0]
    ^{"meta" "data"}
    (fn [] true)))

The compiled Javascript includes the invalid token sequence "return return". (Per Chrome: Uncaught SyntaxError: Unexpected token return)

The problem does not occur if the metadata applies to a map literal instead of a function literal.
The problem only occurs when the function and metadata are inside of a let.



 Comments   
Comment by Bobby Eickhoff [ 07/Jan/15 9:45 PM ]

I forgot to try with-meta. Using with-meta does not produce this syntax error, so it's only a problem with the reader macro for metadata.

Comment by David Nolen [ 08/Jan/15 7:41 AM ]

Any quick thoughts about this one Nicola? Quite possibly a compiler issue on the CLJS side.

Comment by Nicola Mometto [ 08/Jan/15 8:07 AM ]

David, I understand why this happens but I don't know enough about how cljs's js emission to propose a fix.
The issue is that with this commit: https://github.com/clojure/clojurescript/commit/d54defd32d6c5ffcf6b0698072184fe8ccecc93a the following scenario is possible:

{:op :meta
 :env {:context :return}
 :expr {:op :fn
        :env {:context :expr}
        :methods [{:op :fn-method 
                   :env {:context :return} ..}]
        ..}
 ..}

i.e. analyze-wrap-meta changes the context of the :fn node to :expr but keeps the context of the :fn-methods to :return.

This causes both
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L575-L576
and
https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L488 (https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/compiler.clj#L233)

to be true and emit a "return".





[CLJS-848] Support Google Closure modules Created: 31/Aug/14  Updated: 15/Feb/15  Resolved: 15/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Stephan Behnke Assignee: David Nolen
Resolution: Completed Votes: 2
Labels: None


 Description   

The Google Closure Compiler is able to emit multiple files after compilation, instead of just one, by using the '--modules' flag (see example here: http://stackoverflow.com/a/7827251/213730). Ergo, I would love to see the ability to customize the ClojureScript output that way!

My use case would primarily be to separate the library code (core, core.async etc.) from application code. The reasoning is that the libraries rarely change, but the app code constantly does. Also, when my app gets bigger, I could separate areas of my application into according files; libs.js, app.core.js, app.public.js, app.admin.js etc.

Note: I was about to create an issue on the Leiningen plugin but found there is one already (https://github.com/emezeske/lein-cljsbuild/issues/148). In it the plugin author says this is a compiler issue. But I wasn't able to find an issue on this tracker. So I created this one.

Updated proposed syntax (subject to change):

{:optimizations :advanced
 :output-dir "./target/client/production/"
 :cache-analysis true
 :modules {
   :common 
     {:output-to "./resources/assets/js/common.js"  
      :entries '#{com.foo.common}}
   :landing 
     {:output-to "./resources/assets/js/landing.js" 
      :entries '#{com.foo.landing}
      :depends-on #{:common}}
   :editor 
     {:output-to "./resources/assets/js/editor.js"
      :entries '#{com.foo.editor}
      :depends-on #{:common}}}}


 Comments   
Comment by Thomas Heller [ 01/Sep/14 3:32 AM ]

I build https://github.com/thheller/shadow-build to get the module support.

Docs are a bit lacking at the moment but it should be pretty straightforward to use, happy to help if you have questions.

Comment by Stephan Behnke [ 01/Sep/14 5:31 AM ]

Is 'shadow-build' just overriding the final stage of the compiler and the rest stays up-to-date - or is it a fork?
Is there a reason you didn't try to get support for it in the main compiler (yet)?

Either way, it sounds very interesting! I'll dive into shadow-build next weekend, hopefully

Comment by David Nolen [ 01/Sep/14 3:49 PM ]

I'm actually now think it's a good idea to support this to allow breaking up large ClojureScript libraries that will be consumed by others as a plain JS dependency. I'd be happy to see GClosure modules support land in ClojureScript if someone is willing to start a design page with basic plan of the implementation strategy.

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

@Stephan: shadow-build is not a fork, it is a replacement to the cljs.closure namespace (specifically cljs.closure/build) since a single build function is not flexible enough to do what I needed. The compiler/analyzer is untouched and is as up-to-date as you want (you provide which version of cljs you want to use, up to HEAD). The main reason I didn't try to get it into CLJS core itself is time, at the time I wrote it I needed to "get it done". Since then it just has been working so there was no need to have it in core really. I know of a couple other people using it, so it is working for others too.

@David: I'd be happy to maybe use shadow-build as a sort of reference implementation for the whole thing. I'm quite happy with it, some API/naming cleanup aside. But we should go through a proper design process first, since not all choices I made may be ideal for everyone. That being said: GClosure modules are ONLY meant to separate ONE build into multiple files. They are not what javascript commonly calls modules (since we have those basically through namespaces). There are some caveats when trying to share a build with the JS world. I will try to write up why I did what I did in shadow-build and the features it provides. I think all browser targeted builds will want those features when the project reaches a given size.

Comment by David Nolen [ 02/Sep/14 7:43 AM ]

@Thomas thanks. I've created an empty design page here: http://dev.clojure.org/display/design/Google+Closure+Modules

Comment by Thomas Heller [ 02/Sep/14 10:25 AM ]

I didn't know where to start so I did a sort of brain dump of the whole thing. Hope it is enough to get things started, happy to go into more detail.

Comment by Thomas Heller [ 01/Jan/15 4:33 PM ]

It should be noted that Closure Modules aren't actually about splitting files to seperate rarely changing code from frequently changing code since files from one :advanced build cycle cannot be shared with files from another.

It is still desireable to split files for other reasons but this is a common misunderstanding people keep running into with shadow-build.

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

Thomas, this is understood. It's just about supporting a useful lfeature that Closure has that's missing from ClojureScript.

Comment by Thomas Heller [ 02/Jan/15 11:47 AM ]

Agreed.

I just forgot there was an open issue for this and re-reading the description I thought it might be confusing as to why one would want modules. My experience with shadow-build has shown that it is not obvious that files from one optimize cycle cannot the shared with files from another one. Doing so leads to strange errors, I just thought I should clarify that.

Comment by pengke [ 19/Jan/15 3:48 AM ]

hunger for this fix.
it's very painful for download one large js file at once

Comment by David Nolen [ 19/Jan/15 7:12 AM ]

Please do not comment your support for tickets it just generates more noise to sift through. Use the vote functionality.

Comment by David Nolen [ 15/Feb/15 1:03 PM ]

basic support for modules has landed as of https://github.com/clojure/clojurescript/commit/c5613a0ea7b4d187f8c5c25ac7cabb51d4549758. Smaller tickets should be created for remaining detail work around foreign libraries, preambles, and source maps.





[CLJS-1040] Source-mapped script stack frames for the Nashorn repl Created: 15/Feb/15  Updated: 15/Feb/15  Resolved: 15/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Pieter van Prooijen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Add the new source map support for stack frames to the Nashorn repl. It now also reports the actual javascript stackframes (when applicable) to exceptions which happen during repl evaluation.



 Comments   
Comment by David Nolen [ 15/Feb/15 9:31 AM ]

fixed https://github.com/clojure/clojurescript/commit/664c8d54599d4e93a598423777bcb7f579099430





[CLJS-1035] Built in REPLs and 3rd party REPLs should support :watch option Created: 13/Feb/15  Updated: 13/Feb/15  Resolved: 13/Feb/15

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

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


 Description   

We should try running cljs.closure/watch in a future and see what problems arise. How much coordination do we need if we are auto building and a user can (require ... :reload) at the same time?



 Comments   
Comment by Mike Fikes [ 13/Feb/15 9:59 AM ]

There would probably be interesting interactions of this feature with CLJS-901.

Comment by David Nolen [ 13/Feb/15 2:51 PM ]

fixed https://github.com/clojure/clojurescript/commit/606ccce775706b9f9f6f40ecdcc40d6947226824

Comment by Mike Fikes [ 13/Feb/15 3:44 PM ]

Confirmed fixed.





[CLJS-1037] cls.analyzer/ns-dependents fails for common cases Created: 13/Feb/15  Updated: 13/Feb/15  Resolved: 13/Feb/15

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

CLJS 2843


Attachments: Text File CLJS_1037.patch     Text File CLJS_1037-tests-and-fix.patch    

 Description   

Because of the order of execution in ns-dependents namespaces can be eliminated and not included in the final return set.



 Comments   
Comment by Bruce Hauman [ 13/Feb/15 2:52 PM ]

I attached a patch with the failing test. There are lots of fixes for this I'm going to submit one in just a few.

Comment by Bruce Hauman [ 13/Feb/15 3:19 PM ]

Attached a path with a fix, but would prefer to rewrite the function.

Comment by David Nolen [ 13/Feb/15 3:25 PM ]

fixed https://github.com/clojure/clojurescript/commit/1a67860fa81488ee4692b53dcd3a8de46afa21d3





[CLJS-1033] take and drop accept nil as n argument Created: 12/Feb/15  Updated: 12/Feb/15

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

Type: Defect Priority: Minor
Reporter: Sean Grove Assignee: Sean Grove
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript


Attachments: Text File require-integer-for-take-and-drop.patch    
Patch: Code and Test

 Description   

(take nil [1 2 3]) in Clojure raises an error
In ClojureScript, it's the same as (take 0 [1 2 3])

This patch checks that take and drop both check that n is an integer, so that it doesn't silently behave differently.






[CLJS-733] Implement printing & equality for the JSValue type Created: 25/Dec/13  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Roman Scherer Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: File js-value-print.diff     File js-value-print-only.diff    
Patch: Code and Test

 Description   

Using the JSValue type in Clojure tests is a little bit cumbersome at
the moment. The following test for example is not passing at the
moment, because equality is not defined on JSValue.

(is (= '(js/React.DOM.div #js {})
'(js/React.DOM.div #js {})))

It would be nice if the JSValue type implements at least equality and
tagged literal printing on the Clojure side as well. The attached
patch implements this functionality.



 Comments   
Comment by David Nolen [ 25/Dec/13 11:15 AM ]

The equality test doesn't match equality semantics in JavaScript. So while this is convenient, this is going to need a really strong argument. I'm inclined to just say no.

Printing for JSValue is OK with me.

Comment by Roman Scherer [ 25/Dec/13 12:40 PM ]

Ok, this solves half of my problem. My strong argument for the
equality test would be "but JSValue lives in Clojure land, and
consumers (analyzer, compiler, tests) of JSValue are better served
with the same equality semantics that Clojure already provides". My
use case for this is over here:

https://github.com/r0man/sablono/blob/js-literal/test/sablono/compiler_test.clj#L18

The sablono compiler generates forms that can contain JSValues. Those
forms I need to check for equality in my tests. Ok, I can define my
own equality operator that walks the forms and replaces JSValue with
something else, but that feels really strange. Any other idea?

Can you make an example why implementing equality this way would be
problematic? I think I didn't get your point.

If you are still against it, I'm happy to provide a patch for the
print functionality. That's the one I really need. Unfortunately this
one I could have provided myself, the equality thing not.

Comment by David Nolen [ 26/Dec/13 8:43 AM ]

Consider if we bootstrapped and JSValue disappeared and we could use the JS Array type directly instead. But arrays are not equal in Clojure(Script) because they are not values.

Comment by Roman Scherer [ 26/Dec/13 8:50 AM ]

Ok. Thanks for the explanation.

Comment by David Nolen [ 12/Feb/15 10:43 AM ]

Not going to do this one.





[CLJS-769] clojure.core/unsigned-bit-shift-right not excluded in cljs.core Created: 17/Feb/14  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Defect Priority: Trivial
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 6
Labels: None

Attachments: File CLJS-769.diff    
Patch: Code

 Description   

EOM



 Comments   
Comment by Kyle VanderBeek [ 17/May/14 8:34 PM ]

This defect does cause a warning with lein-cljsbuild 1.0.3 under Clojure 1.6.0 and ClojureScript 0.0-2202.

WARNING: unsigned-bit-shift-right already refers to: #'clojure.core/unsigned-bit-shift-right in namespace: cljs.core, being replaced by: #'cljs.core/unsigned-bit-shift-right
Comment by Francis Avila [ 01/Jul/14 10:57 PM ]

This is fixed in r2261 (the clojure 1.6 release).





[CLJS-887] cljs.repl.browser does not serve css by default Created: 17/Nov/14  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Defect Priority: Trivial
Reporter: Christopher Shea Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl
Environment:

*


Attachments: File cljs-browser-repl-serve-css.diff    
Patch: Code

 Description   

A browser repl's server serves js, cljs, map, and html files by default, but not css.



 Comments   
Comment by John Chijioke [ 10/Dec/14 2:05 AM ]

What is css needed for from the repl?

Comment by Christopher Shea [ 10/Dec/14 9:13 AM ]

cljs.repl.browser/send-static can already send css with the appropriate MIME type, so this just seems like a minor oversight.

If you follow the Using the browser as an Evaluation Environment section of the ClojureScript wiki and have a css file linked from your html page, it will not be served, which can make it more difficult to work on your project (you won't see the effects of changing an element's class, e.g.).

As a workaround, I've been doing this when setting up my repl:

(server/dispatch-on :get
  (fn [{:keys [path]} _ _] (.endsWith path ".css"))
  browser/send-static)

It's not that my interactions with the repl require the css, it's the browser that's connected to the repl that does.

Comment by David Nolen [ 12/Feb/15 10:41 AM ]

fixed https://github.com/clojure/clojurescript/commit/43d0bb1e99aa2f23f1e7cf5004d347b6e6a70ff0





[CLJS-933] Redef warning omiting namespace of original symbol Created: 29/Dec/14  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Defect Priority: Trivial
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 933.patch    

 Description   

;; The redef warning is slightly broken:

(ns example.core
(:require [example.foo :refer [a]]))

(def a 1)

;; actual
;; WARNING: a already refers to: /a being replaced by: example.core/a at line 4 src/example/core.cljs

;; expected
;; WARNING: a already refers to: example.foo/a being replaced by: example.core/a at line 4 src/example/core.cljs



 Comments   
Comment by Shaun LeBron [ 29/Dec/14 4:41 PM ]

attached a one-line patch. An `:ns` value wasn't being passed to the warning (see link below), so I'm just using `:ev` value to get the fully qualified name.

https://github.com/clojure/clojurescript/blob/0da30f5aa3b937d1a1c01891cb4601be8a3ea210/src/clj/cljs/analyzer.clj#L654

Comment by David Nolen [ 30/Dec/14 8:04 AM ]

Looks good but this is not a properly formatted patch: https://github.com/clojure/clojurescript/wiki/Patches

Could we get a new one? Thanks!

Comment by Shaun LeBron [ 30/Dec/14 11:23 AM ]

oops, attached proper patch. thanks

Comment by David Nolen [ 30/Dec/14 11:23 AM ]

Looks good have you sent in your CA? Thanks again.

Comment by Shaun LeBron [ 30/Dec/14 1:33 PM ]

Just completed the CA submission.

Comment by David Nolen [ 12/Feb/15 10:38 AM ]

Ah missed this patch and fixed this myself in master





[CLJS-948] Simplify macro usage Created: 02/Jan/15  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: File cljs-948.diff     File cljs-948-no-passes.diff    

 Description   

The usage of macros from CLJS is very explicit and users of any given namespace must remember whether it comes with macros and which vars are macros. This leads to rather verbose :require statements which are confusing and lead to unexpected results if incomplete (eg. missing :refer-macros).

(ns my-ns
  (:require [my-app.other-ns :as other :refer (something) :refer-macros (something) :include-macros true]))

(something) ;; macro (when :refer-macros, otherwise no macro)
(other/thing) ;; maybe macro (only when :include-macros, otherwise no macro)

I think the user should not need to know whether something is a macro or not, they should just be able to use it.

I implemented a proof-of-concept that

(ns my-ns
  (:require [my-app.other-ns :refer (something)])

will work just find and do the correct thing with regards to macros (assuming the namespace has a corresponding clj namespace, :require-macros is unaffected). No changes to any code are necessary as the implementation uses ana/passes, it is also fully backwards compatible.

Implementation points which should be discussed

  • The CLJS namespace with macros currently has to opt-in through a bit of metadata (eg. (ns my-ns-with-macros {:load-macros true})), that might not be necessary and maybe should default to true.
  • The implementation assumes compilation happens in dependency order. shadow-build always does this, I'm not too sure about CLJS though. Given ana/analyze-deps equals true that is guaranteed but I'm not sure that is always the case.

If there is any interest I can provide a patch, until then refer to the proof-of-concept [1].

[1] https://github.com/thheller/shadow-build/blob/f37cfa598f1e90dd66e333d1e45580ea25650025/src/clj/shadow/cljs/passes.clj#L30-L82



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

This is interesting. Will think about it.

Comment by Thomas Heller [ 02/Jan/15 1:15 PM ]

Is it ok if I leave implementation notes here?

Things that would need addressing should this be implemented:

  • cljs.analyzer/check-uses runs when parsing the ns form, since the macro passes run AFTER the parsing finished the check-uses will warn incorrectly if a :refer is not defined in CLJS but is a macro. Solution: check-uses should probably just become a pass itself which runs after the macro ones. Moving a side-effect out of the parse function is probably well worth it.
  • cljs.analyzer/load-macros would obsolete and the entire functionality it was toggling can be moved to the load-macros pass.
Comment by David Nolen [ 03/Jan/15 3:33 PM ]

I looked a bit at your code. It looks interesting. Definitely not going to do the namespace metadata bit. It seems to me this should work only if the required namespace loaded macros from a Clojure namespace of the same name.

Comment by Thomas Heller [ 03/Jan/15 3:36 PM ]

Yeah, probably. Also played with the idea of just looking for (io/resource (str (ns->path) ".clj")). If that is nil the namespace doesn't have macros, if its not null we require it.

But I like your approach of just using (ns my-ns (:require-macros [my-ns])) more.

Comment by David Nolen [ 03/Jan/15 3:40 PM ]

OK, let's try a patch for this. This is an interesting simplification that unifies Clojure & ClojureScript macro usage.

Comment by Thomas Heller [ 03/Jan/15 4:10 PM ]

Yay! Coming right up.

One issue though: I thought it would be a good idea to put the passes into a separate file (eg. cljs.analyzer.passes), that is currently not possible due to cljs.analyzer/analyze (it controls the passes defaults) [1]. cljs.analyzer cannot depend on cljs.analyzer.passes since it most likely depends on cljs.analyzer (circular dependency). Technically the passes currently only require the analyzer because of ::ana/namespaces but faking out a circular dependency is not a good idea.

If we remove the (or passes [infer-type]) and instead force opts to contain :passes (or bind passes beforehand) we should be fine.

Although I'm perfectly fine with putting the new passes into cljs.analyzer, just thought it would be cleaner not to.

Also, how should I address the invalid refer warning issue? Since the check-uses runs before the pass it can never find the macro. I was going to make check-uses a pass in itself if thats alright with you.

[1] https://github.com/clojure/clojurescript/blob/2d1e2167f5ab8bd76ac5c8bafd198990cc88d34a/src/clj/cljs/analyzer.clj#L1711

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

I cleaned up the reference in shadow-build [1]. This is how a cljs.analyer.passes could look, the default passes would then be

[cljs.analyzer.passes/load-macros
 cljs.analyzer.passes/infer-macro-require
 cljs.analyzer.passes/infer-macro-use
 cljs.analyzer.passes/check-uses
 cljs.analyzer/infer-type]

This would basically make ana/load-macros obsolete since this can now be controlled via passes. Also addresses CLJS-955.

[1] https://github.com/thheller/shadow-build/blob/220d3bb0bef2cdb5696487b639ca5aaa169c56f2/src/clj/shadow/cljs/passes.clj

Comment by David Nolen [ 03/Jan/15 5:40 PM ]

This looks OK to me at the moment.

Comment by Thomas Heller [ 03/Jan/15 5:40 PM ]

Should stop refering to specific hashes, please see [1].

There is a slight issue with cljs.core itself. There is a bit of code that makes cljs.core macros special. I'm not quite sure why this is, since it would not be required if it did.

(ns cljs.core
  (:require-macros [cljs.core]))

Getting an error [2] on the recently changed def to (defonce print-fn). Haven't exactly figured out why this is yet.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj
[2] https://www.refheap.com/61bc05e4b4a74a3b573797721

Comment by Thomas Heller [ 03/Jan/15 5:44 PM ]

Wait, are other namespaces allowed to use their own macros unqualified?

(ns my-ns
  (:require-macros [my-ns]))

(this-is-a-macro)
;; vs
(my-ns/this-is-a-macro)

I don't think so? The recent defonce seems to be an exception?

Comment by David Nolen [ 03/Jan/15 5:50 PM ]

yes cljs.core and core macros are an exception

Comment by Thomas Heller [ 03/Jan/15 5:54 PM ]

Yeah, but cljs.core calls all macros directly.

(defn ^boolean identical?
  "Tests if 2 arguments are the same object"
  [x y]
  (cljs.core/identical? x y))

The recently changed defonce vars are the exception here. Although even if I change them to cljs.core/defonce I still can't explain the error.

Comment by David Nolen [ 03/Jan/15 5:56 PM ]

The inlining macros are an exception, it's done that way to break circularity which would otherwise stack overflow the compiler.

Comment by Thomas Heller [ 03/Jan/15 7:09 PM ]

Alright, resolved that macro issue.

I updated [1] so the analyzer passes are initialized correctly and outside cljs.analyzer. The patch would also change the cljs.analyzer/parse 'def since that functionality is moved to cljs.analyzer.passes.

(when (and *analyze-deps* (seq @deps))
      (analyze-deps name @deps env opts))
    #_ (when (and *analyze-deps* (seq uses))
         (check-uses uses env))
    (set! *cljs-ns* name)
    #_ (when *load-macros*
         (load-core)
         (doseq [nsym (concat (vals require-macros) (vals use-macros))]
           (clojure.core/require nsym))
         (when (seq use-macros)
           (check-use-macros use-macros env)))

The question is where do we require cljs.analyzer.passes (must be required somewhere so the default is configured correctly)?

cljs.closure is probably the best bet.

Not a big fan of this alter-var-root but I'm not sure how we could do without. I doubt anyone but me currently uses cljs.analyzer/passes and therefore the default behavior of (or passes [infer-type]) always triggers. Since that would no longer work correctly we need to initialize to another default.

cljs.analyzer/parse-ns would also need to change again given the removal of load-macros. Same functionality can be achieved within the same binding though.

[1] https://github.com/thheller/shadow-build/blob/master/src/clj/shadow/cljs/passes.clj

Comment by Thomas Heller [ 03/Jan/15 8:16 PM ]

My head hurts, I need to get some sleep.

I cannot figure out why the attached patch does not work, but ./script/test produces errors in parse-ns.

Will get back to it tommorrow.

Exception in thread "main" java.lang.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol, compiling:(/Users/zilence/code/oss/clojurescript/bin/../bin/cljsc.clj:18:68)
	at clojure.lang.Compiler.load(Compiler.java:7142)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$script_opt.invoke(main.clj:336)
	at clojure.main$main.doInvoke(main.clj:420)
	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.IllegalArgumentException: Don't know how to create ISeq from: clojure.lang.Symbol
	at clojure.lang.RT.seqFrom(RT.java:505)
	at clojure.lang.RT.seq(RT.java:486)
	at clojure.core$seq.invoke(core.clj:133)
	at cljs.analyzer$parse_ns$fn__1660.invoke(analyzer.clj:1757)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1747)
	at cljs.analyzer$parse_ns.invoke(analyzer.clj:1741)
	at cljs.compiler$compile_root.invoke(compiler.clj:1069)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$eval3084$fn__3085.invoke(closure.clj:398)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$eval3071$fn__3072.invoke(closure.clj:412)
	at cljs.closure$eval3020$fn__3021$G__3011__3028.invoke(closure.clj:306)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at user$eval3279.invoke(cljsc.clj:21)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	... 9 more
Comment by Thomas Heller [ 04/Jan/15 5:40 AM ]

That is why you should probably not write code tired at 3am. Was using infer-tag as a pass instead of infer-type.

I decided to drop the cljs.analyzer.passes namespace and instead moved the passes to cljs.analyzer. The extra namespace introduced a weird circular dependency which cljs.analyzer/parse-ns couldn't quite handle.

The patch now applies cleanly, would probably be nice to have a test that actually tests that it does what its supposed to.

Comment by David Nolen [ 07/Jan/15 8:21 AM ]

The patch has not been updated with these changes.

Comment by Thomas Heller [ 07/Jan/15 8:37 AM ]

Sorry, my mistake. Fixed.

Comment by Thomas Heller [ 08/Jan/15 7:19 AM ]

One issue I found is related to caching. Depends on how/where cljs.analyzer/parse-ns is called.

If the new passes aren't executed and the result of that analysis are used to write out the cache it won't contain the new macros information. I believe cljs.analyzer/parse-ns does this.

On the next run if it was decided to use the cache, we are missing some information AND won't require the macros. This was true before the changes but since the dependant namespaces would require the macros on their own the issue was somewhat hidden. Now they assume that macros were already loaded, therefore the must require macros when reconstructing from cache.

How to do that best in CLJS I don't know since shadow-build does its own caching. The passes could be refactored so they can be called with the cached information as well.

Note that this issue was basically introduced by load-macros (CLJS-940), but since that flag is already gone it is now moved to the :load-macros option of parse-ns.

The simplest way to address this would be to always run the new passes, basically how it was before CLJS-940.

Comment by David Nolen [ 08/Jan/15 7:39 AM ]

Thomas to me this illustrates the pitfalls of trying to bring a multipass approach to an existing compiler that isn't already fundamentally multipass. It seems to me that an alternative approach is possible without bothering at all about passes. In the analyzer parse 'ns case for each required CLJS lib you could leverage parse-ns to see if it requires a macro file of the same name - if it does add an implicit :require-macros.

While I like the passes approach in theory it will just make it harder for contributors to understand the structure of the compiler, they have to look at two different approaches to determine where things happen.

Comment by Thomas Heller [ 08/Jan/15 7:56 AM ]

Well, yeah we could to away with the passes and do it all in (defmethod parse 'ns ...) but that doesn't change the issue as long as there is a way to toggle this at all (ie. :load-macros or load-macros). It is not really about whether it is in a pass or not.

I just liked using passes since it sort of allows cherry-picking what you want and don't want to run. infer-types doesn't need to run in parse-ns for example, but it always did before this.

But I'm not committed to passes, happy to move everything so it runs in the parse method.

Let me know, if I should rewrite the patch.

Comment by Thomas Heller [ 08/Jan/15 8:35 AM ]

Hmm, you are right. Will move it out of the passes.

The only reason it was in there in the first place is due to shadow-build. It was the only place I could put it to make it work without patching CLJS. But since we agree that this should be in core the passes option really doesn't make sense any more.

Going to deliver a new patch soon.

Comment by David Nolen [ 08/Jan/15 8:37 AM ]

The difference for inter-types is that is something that needs to be run for a large number of nodes - it's a real global pass.

Let's go with moving the logic in to parse 'ns. I still don't understand the *load-macros* issue you are describing. We absolutely need to be able to disable this and dependency analysis for legitimate uses of parse-ns.

Comment by Thomas Heller [ 08/Jan/15 9:42 AM ]

Attached the no-passes version. The does get a bit simpler since it doesn't have to manually patch the compiler env.

I'm not totally sure how the @reload logic works, maybe some adjustments are required.

Comment by Thomas Heller [ 08/Jan/15 9:53 AM ]

The *load-macros* issue manifests as follows:

(ns demo.ns1
  (:require-macros [demo.ns1 :as m]))

(ns demo.ns2
  (:require [demo.ns1 :as x :refer (a-macro)]))

Compile 1: Everything is fine, cache is written.
Compile 2:

  • ns1 not modified, parse-ns decides to use cached version
  • ns2 modified, compiler attempts to compile, fails with NullPointerException
Caused by: java.lang.NullPointerException
	at cljs.analyzer$get_expander.invoke(analyzer.clj:1532)
	at cljs.analyzer$macroexpand_1.invoke(analyzer.clj:1543)
	... 80 more

This is because the load from cache does not require the macro namespace. Before these changes demo.ns2 would have had a :require-macros or equivalent in the ns form which would then have require'd the macro ns. Now only demo.ns1 would trigger the require, but since it loads from cache it doesn't UNLESS *load-macros* is true. Since parse-ns does revive the cache (I think) it is not.

The require must be triggered somewhere before actual compilation happens. In shadow-build this is pretty simple but I'm not sure where to put this for CLJS.

Comment by Thomas Heller [ 09/Jan/15 10:25 AM ]

As per IRC the *load-macros* issues does not present in CLJS since it is true when it needs to be.

Comment by David Nolen [ 01/Feb/15 4:11 PM ]

fixed https://github.com/clojure/clojurescript/commit/28e6070691e89f5fe35ae8f8a196ae1048fbf07d

Comment by Thomas Heller [ 01/Feb/15 9:11 PM ]

Nice work, your solution looks much better.

Comment by Thomas Heller [ 04/Feb/15 5:48 AM ]

I take that back, I no longer like the solution since you added the call to parse-ns, which basically supersedes the *analyze-deps* option.

Since you cannot guarantee that files are processed in dependency order the extra parse-ns call will create an entry in the compiler env for the namespace, analyze-deps will then skip the analyze-file since parse-ns already created the entry (which is incomplete since it stopped at the ns)

In addition the sequence of calls is wrong and check-uses does not work correctly since it does not check for macros.

(ns macro-use.bug
  (:require [macro-use.core :as core :refer (a-macro)]))

Since check-uses now throws this will result in

clojure.lang.ExceptionInfo: Referred var macro-use.core/a-macro does not exist at line 1 src/macro_use/bug.cljs {:tag :cljs/analysis-error, :file "src/macro_use/bug.cljs", :line 1, :column 1}

Demo repo here:
https://github.com/thheller/macro-use-bug/blob/master/src/macro_use/bug.cljs

Comment by David Nolen [ 04/Feb/15 6:49 AM ]

parse-ns does not change the compiler environment by default nor is it recursive by default. All that it is used for in this context is to extract namespace information, nothing more or less.

Expecting :refer to work like that for macros was never under consideration for this issue. I should have been more clear about that. We can open a new minor enhancement issue for this addition.

Comment by Thomas Heller [ 04/Feb/15 7:05 AM ]

parse-ns calls analyze [1] which calls (defmethod parse 'ns ...) which does a swap! [2] and therefore touches the compiler environment?

[1] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1818
[2] https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1357

When opening the issue my goal was to make everything as close to Clojure as possible which included the transparent handling of :refer for macros. All my patches included that functionality so I'm slightly confused that this was not "under consideration". Especially since my example basically consisted of exactly that use-case. We can take this to another issue if we must ...

Comment by David Nolen [ 04/Feb/15 8:56 AM ]

Thomas the previous environment gets restored, https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/analyzer.clj#L1840.

Because parse-ns is not recursive by default there are no problems.

Like I said sorry I wasn't clear about what I wanted to see implemented. The :refer bit is just nice sugar and not fundamental. Please open a separate ticket.





[CLJS-1032] Node.js target should support :main Created: 12/Feb/15  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

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


 Comments   
Comment by David Nolen [ 12/Feb/15 7:48 AM ]

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





[CLJS-967] "java.net.ConnectException: Connection refused" when running node repl Created: 07/Jan/15  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Defect Priority: Major
Reporter: Brian Muhia Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, errormsgs
Environment:

Processor: 1.33GHz Intel Atom, Ubuntu 12.04, OpenJDK 1.7, Clojure 1.6.0, ClojureScript 0.0-2665, nodejs v0.10.26


Attachments: Text File backtrace.txt    

 Description   

I used trampoline like so:

rlwrap lein trampoline run -m clojure.main scripts/repl.clj

run the script repl.clj with contents:

(require
  '[cljs.repl :as repl]
  '[cljs.repl.node :as node])

(repl/repl* (node/repl-env)
  {:output-dir "out"
   :optimizations :none
   :cache-analysis true                
   :source-map true})

Instead of getting a repl, I got the exception: Exception in thread "main" java.net.ConnectException: Connection refused, compiling/home/brian/projects/essence-cljs-redux/scripts/repl.clj:3:30)

The full stack trace is attached in backtrace.txt.



 Comments   
Comment by David Nolen [ 08/Jan/15 7:21 AM ]

The issue is that we use a 300ms timeout to connect to the Node.js process, we need to instead redirect the process out and wait for the Node.js REPL server start string.

Comment by David Nolen [ 12/Feb/15 7:22 AM ]

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





[CLJS-1031] Download Closure over https: in ./script/bootstrap Created: 12/Feb/15  Updated: 12/Feb/15  Resolved: 12/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Chris Cowan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Line 69 of ./script/bootstrap downloads Google's Closure Compiler from http://dl.google.com/closure-compiler/compiler-$CLOSURE_RELEASE.zip, though the URL may be accessed securely over HTTPS too. Could the script be changed to do so?



 Comments   
Comment by David Nolen [ 12/Feb/15 5:58 AM ]

fixed https://github.com/clojure/clojurescript/commit/8b30ecb2327d7942aae1485865fa9540f2eab9e1





[CLJS-1029] Investigate how ns aliasing can be supported in ClojureScript macro files Created: 11/Feb/15  Updated: 11/Feb/15

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

Type: Task Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently we just require the file. Perhaps possible to control compilation of the macro file such that ClojureScript ns aliases are established. This may not bear fruit but worth thinking about.



 Comments   
Comment by David Nolen [ 11/Feb/15 4:05 PM ]

Any design ideas along this path needs to keep .cljc files in mind.





[CLJS-1018] Add support for cljs.core/*e Created: 07/Feb/15  Updated: 11/Feb/15  Resolved: 07/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS-1018-better-format.patch     Text File CLJS-1018.patch    

 Description   

ClojureScript supports *1, *2, *3 in the REPL just like Clojure REPLs. But, it is lacking support for *e, the last exception caught.



 Comments   
Comment by Mike Fikes [ 07/Feb/15 1:05 PM ]

In the Clojure REPL, here is an example showing that the exception is stored and printed as a non-readable:

user=> (first 1)

IllegalArgumentException Don't know how to create ISeq from: java.lang.Long  clojure.lang.RT.seqFrom (RT.java:505)
user=> *e
#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.lang.Long>

I experimented in the Ambly ClojureScript REPL with a hypothetical cljs.user/*e (wrong namespace) and found that a non-readable could be captured if the JS Error is placed in this dynamic var:

ClojureScript:cljs.user> (def ^:dynamic *e nil)
nil
ClojureScript:cljs.user> (first 1)
Error: 1 is not ISeqable
(... trace omitted)
nil
ClojureScript:cljs.user> *e
#<Error: 1 is not ISeqable>

I did this by revising the internals of the REPL impl near here to stash the exception in the var, with a bit of code.

self.jsContext[@"cljs"][@"user"][@"_STAR_e"] = self.jsContext.exception;

Of course, this isn't the right place at all to implement something like this, but it experimentally shows that such a thing would be feasible.

There is code that imperatively shifts down *1, *2, *3, in the JavaScript that is sent to the execution environment when a form is evaluated. Perhaps that code is a place that could be revised to catch exceptions, stash them in *e, and re-throw. (I don't know enough about JavaScript to know if that is feasible or performant.)

I suppose any solution that would work would simply need access to the actual JavaScript exception object so that it can assign it to *e.

Comment by Mike Fikes [ 07/Feb/15 3:00 PM ]

Adding a try-catch-throw is relatively simple and the attached patch works:

ClojureScript:cljs.user> (doc *e)
-------------------------
cljs.core/*e
()
  bound in a repl thread to the most recent exception caught by the repl
nil
ClojureScript:cljs.user> (first 1)
Error: 1 is not ISeqable
cljs.core/seq (.ambly_jsc_repl/cljs/core.cljs:707:4)
cljs.core/first (.ambly_jsc_repl/cljs/core.cljs:731:6)
nil
ClojureScript:cljs.user> *e
#<Error: 1 is not ISeqable>

This causes a try-catch-throw to be sent to the JavaScriptEngine:

cljs.core.pr_str.call(null,(function (){try{var ret__3518__auto__ = cljs.core.first.call(null,(1));
cljs.core._STAR_3 = cljs.core._STAR_2;

cljs.core._STAR_2 = cljs.core._STAR_1;

cljs.core._STAR_1 = ret__3518__auto__;

return ret__3518__auto__;
}catch (e4844){var e__3519__auto__ = e4844;
cljs.core._STAR_e = e__3519__auto__;

throw e__3519__auto__;
}})())
Comment by Mike Fikes [ 07/Feb/15 3:14 PM ]

The last attachment had poorly formatted code in it. This new attachment fixes that.

Comment by David Nolen [ 07/Feb/15 8:22 PM ]

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

Comment by Mike Fikes [ 11/Feb/15 11:24 AM ]

This "catch and rethrow" strategy is not completely transparent in the Node.js REPL (using Node v0.12.0). In particular, an indicator of the throw site appears in the description:

ClojureScript:cljs.user> (ffirst 1)
repl:12
throw e__3553__auto__;
      ^
Error: 1 is not ISeqable
    at Object.seq (.../cljs/core.cljs:727:20)
    at Object.first (.../cljs/core.cljs:736:16)
    at ffirst (.../cljs/core.cljs:1155:11)

I don't recall seeing this with the previous Node version.

The stackframes themselves are fortunately not modified, which this SO claims is the proper behavior; the problem described in this comment is limited only to the description.

Comment by David Nolen [ 11/Feb/15 12:16 PM ]

I think we can live this for now





[CLJS-1025] Source mapping could support clients more generally. Created: 10/Feb/15  Updated: 11/Feb/15  Resolved: 11/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently print-mapped-stacktrace takes a path from the root of the project.

(print-mapped-stacktrace
  [{:file "samples/hello/out/hello/core.js" <-- path from project root
    :function "first"
    :line 2
    :column 1}]))

Some clients are getting their code from a server and thus don't have project root relative paths.

In fact, the client side paths can be arbitrary in a way that is difficult and non trivial
for repl server code like IJavaScriptEnv's to deduce the correct project root path from.

For instance if a browser (figwheel) client is getting its code from localhost/arbtirary/path/to/example/core.js
but :output-dir is "out", how is the IJavaScriptEnv supposed to reliably calculate the
path from project root. :asset-path can help here but asset path is not a required build option.

Because all clients can easily calculate the output-dir relative path with js/goog.basePath:
output-dir_relative_path = path - ( js/goog.basePath - #"goog/$")

I think it may be more general to take an output-dir relative path from base like so:

(print-mapped-stacktrace
  [{:file "hello/core.js" <-- path from output dir
    :function "first"
    :line 2
    :column 1}]))

I'm solving this problem in figwheel by calculating the output-dir_relative_path client side
then having the IjavaScriptEnv prepend the :output-dir.

Since the web is our most common eval environment it would be nice not to force clients to process stack lines on both sides of the divide.



 Comments   
Comment by David Nolen [ 10/Feb/15 9:06 AM ]

Thanks I'm starting to see the problem and will look into it. Will definitely get this sorted out before the next release as I would like everyone to just reuse this work.

Comment by Bruce Hauman [ 10/Feb/15 10:55 AM ]

I'm happy to contribute a default stackline parser.

One more thing, it may be nice to allow for stack line parse failures, in order to allow for incredible pace at which these environments are evolving. Either have print-mapped-stacktrace accept a vector of maps(successful stack line parses) and strings(unsuccessful parses). Or accept an extra key in the stack-line map like :orig-stack-line.

print-mapped-stacktrace can then just print out the unsuccessful parses as they are.

Comment by David Nolen [ 10/Feb/15 11:34 AM ]

We're not going to do default stackline parser. Will look into the path stuff for sure.

Comment by David Nolen [ 10/Feb/15 11:41 PM ]

Ok so I looked into this some more, one problem is that IJavaScriptEnv methods beyond -setup do not take compiler opts. This makes it impossible for REPLs to correctly generate the :output-dir relative paths at the time the exception is detected without introducing some state to store opts. I think we should introduce a new protocol IMapStacktrace which REPLs may optionally implement. This protocol will consist of a single method, it will receive the original stacktrace string and compiler opts. It's implicit that REPLs do not return canonical stacktraces under any other conditions - only strings. Feedback welcome.

Comment by Bruce Hauman [ 11/Feb/15 8:14 AM ]

It seems to me this focuses on the difficulty of getting output-dir, but exacerbates the path parsing problem for browser based IJavaScriptEnvs'

This is a choice that would prevent the client side(browser) from parsing canonical traces and foist it all on IJavaScriptEnv. Why make that decision for implementors?

IJavaScriptEnv doesn't have js/goog.basePath and :asset-path isn't a required build option and it really shouldn't be for folks to get mapped stacktraces. To me this increases the difficulty of going from an arbitrary client-side path to a project root or output-dir relative path because we are forcing the computation to be made in an environment that doesn't have an very important piece of information.

In my proposal I was thinking that leveraging the client side knowledge of -basePath is absolutely key.

I much much prefer the code the way it is now. At least implementors have the flexibility to decide how and where to parse stack traces for given environments.

As far as making the :output-dir available to IJavaScriptEnv. I'm providing to the the FigwheelEnv record constructor. An IJavaScript can only be associated with one build anyway.

David, thanks for looking into this.

Comment by David Nolen [ 11/Feb/15 8:15 AM ]

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

Comment by Bruce Hauman [ 11/Feb/15 8:18 AM ]

I really didn't want that to sound combative. But reading it back it does, I'm sorry about that. I'm not really feeling that way. It's just early for me.

Comment by Mike Fikes [ 11/Feb/15 10:58 AM ]

Confirmed working with Node REPL, Nashhorn REPL (not yet participating, but still works), and the Ambly REPL.





[CLJS-1010] Printing hook for cljs-devtools Created: 05/Feb/15  Updated: 10/Feb/15  Resolved: 10/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print

Attachments: Text File cljs_1010_new_hope.patch     Text File cljs_1010.patch    
Patch: Code

 Description   

cljs-devtools[1] should provide pretty-printing support for ClojureScript values in javascript console under blink-based browsers.

We could just print dumb strings using pr-str, but we want to be smarter. Basically we need a way how hook into pr-writer to re-use ClojureScript printing machinery (including calls to IPrintWithWriter) when doing our pretty-printing. The main goal is to get this functionality "for free" and to be future-proof. Any future code which will call pr-writer, pr-sequential-writer, print-map or similar should be supported by cljs-devtools automatically. Any future or user code implementing IPrintWithWriter protocol should be supported as well, at least as string tokens. Please read the description in the patch[2].

I have a working implementation[3] of cljs-devtools which depends on this patch and results look good. My approach was to augment calls pr-writer:
1) catch calls for simple values I know how to render myself and fall back to original pr-writer for more complex ones
2) (optionally) stop recursion by printing some placeholder or abbreviated value
3) (optionally) post-process results returned from original pr-writer and wrap them in references to live objects (devtools will render disclosure triangles for further user-initiated expansion)

[1] https://github.com/binaryage/cljs-devtools
[2] https://github.com/darwin/clojurescript/commit/a3c96832df6d8893f2066afb4441fa9e9ee629ad
[3] https://github.com/binaryage/cljs-devtools/blob/6dd12324938d9efcb0a7245f5ff0e2db35d1bf56/src/devtools/format.cljs#L132-L147



 Comments   
Comment by Antonin Hildebrand [ 06/Feb/15 11:02 AM ]

Apply after CLJS-1016

Comment by Antonin Hildebrand [ 08/Feb/15 9:48 AM ]

rebased on top of latest CLJS-1016 patch

Comment by David Nolen [ 10/Feb/15 3:46 PM ]

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





[CLJS-1016] Make more marker during printing configurable Created: 06/Feb/15  Updated: 10/Feb/15  Resolved: 10/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs_1016_more_effective.patch     Text File cljs_1016_new_attempt.patch     Text File cljs_1016_no_unicode.patch     Text File cljs_1016.patch     Text File minimal_breaking_change.patch    
Patch: Code and Test

 Description   

print-length controls how many items will be printed by sequential printer. If there are more items available it is communicated by printing "..." the last item of a sequence. For example:

(binding [*print-length* 2] (str [1 2 3 4 5 6 7 8 9 0]))
=> "[1 2 ...]"

I want this "..." string (more-marker) to be configurable via printing opts. This is useful for cljs-devtools. For complex structures I call clojurescript printer. I can set more-marker to some less likely unicode value. And then use it for detection if all values were actually printed or not. This way I don't have to understand internal structure. I simply observe presence of my marker. And can render disclosure triangle for user to dig deeper into the structure.



 Comments   
Comment by David Nolen [ 06/Feb/15 6:09 PM ]

When I try to apply this patch many tests fail. The failures are pretty strange as they don't seem to have anything to do with printing.

Comment by Antonin Hildebrand [ 06/Feb/15 6:13 PM ]

Tests passed on my machine, but I have only spidermonkey engine available.

Maybe unicode characters in the patch could be an issue? My tests contain … which is a unicode char.

Comment by Antonin Hildebrand [ 06/Feb/15 6:14 PM ]

Also now when I look at that let binding second time, maybe I should just inline the :more-marker lookup in places where it is used. Now the lookup is done every time even when entering the method.

Comment by Antonin Hildebrand [ 06/Feb/15 6:17 PM ]

consider this one instead, lookup is done only when needed

Comment by David Nolen [ 06/Feb/15 6:19 PM ]

The new patch does not apply to master.

Comment by David Nolen [ 07/Feb/15 8:24 PM ]

Patch still does not apply. Let's drop the unicode.

Comment by Antonin Hildebrand [ 08/Feb/15 9:13 AM ]

FYI: I did a clean checkout of clojurescript repo. Followed https://github.com/clojure/clojurescript/wiki/Running-the-tests.

./scripts/test does not work for me. All tests pass or it spills random errors/failures (even without this patch applied). When I apply this patch. And try to run ./scripts/test the generated javascript in builds/out-adv does not contain my changes. Of course, I was looking for unique strings not function names.

./scripts/test-simple works as expected and reflect my code changes (for example when I break tests)

Comment by David Nolen [ 08/Feb/15 9:41 AM ]

I just tried a clean checkout of the repository and I cannot replicate the test failures.

Comment by Antonin Hildebrand [ 08/Feb/15 9:56 AM ]

FYI: with CLJS-1010 I have two clojurescript repos checked out. Because I was trying to do your workflow of applying my patches with "git am" just to test them. And I observed that even script/test-simple does not work in one of them. I deliberately broke the tests and script/test-simple didn't reflect my change. In other repo the same change worked as expected.

The broken repo is not totally clean. It was my original one where I was editing files with unicode characters. Maybe there is something bad there. But I wonder what, because since then I did "git reset --hard your-master".

Anyways. I will try to download it from scratch and test it again.

Comment by David Nolen [ 09/Feb/15 7:34 AM ]

The latest patch applies but I still get test failures:

Testing cljs.core-test
builds/out-adv/core-advanced-test.js:605: Error: No matching clause: :fn
],0)));default:throw Error([mb("No matching clause: "),mb(Nr(D,F))].join(""));
                                                                    ^
Error: No matching clause: :fn
    at Error (native)
    at builds/out-adv/core-advanced-test.js:605:194
    at builds/out-adv/core-advanced-test.js:605:206
    at Er (builds/out-adv/core-advanced-test.js:596:78)
    at builds/out-adv/core-advanced-test.js:3739:1
    at builds/out-adv/core-advanced-test.js:3927:3
Comment by Antonin Hildebrand [ 09/Feb/15 10:17 AM ]

I tried to find minimal case to trigger the problems with core-advanced-test.js. The change to core.cljs can be as small as replacing the first one of those "..." in pr-sequential-writer with '(get opts :x "...")' (no changes to test files).

When trying different versions by rewriting my code, errors reported by script/test were changing. I tried to make some sense of them by debugging them in chrome devtools, but I didn't find any common traits. They seem to me to be at random places. Advanced compilation unfortunately prevents me to reason about them effectively.

I've got one new observation: I added :static-fns true in scripts/test and problems went away. I didn't find any documentation about static-fns tough.

Comment by David Nolen [ 09/Feb/15 5:25 PM ]

For debugging advanced builds enable :pseudonames true, :pretty-print true compiler options. This should clarify where things are going wrong.

Comment by Antonin Hildebrand [ 10/Feb/15 2:27 PM ]

This test suite failures are not related to my patch. This is some issue in the script/test or closure compiler itself and I was unlucky to trigger it.

Also I assume there is some caching issue, because my results are not stable. Clean checkout without code
modifications script/test passes without errors. Then I change core.cljs with a trivial change. And script/test spills bunch of unrelated errors. I revert the code back. Run script/test and I get the same errors. This is really puzzling.

When I set :static-fns true in script/test all problems go away. I'm afraid this is not something I could solve on my side.

Comment by Antonin Hildebrand [ 10/Feb/15 2:58 PM ]

Tested with current tip 2711d029:
https://github.com/clojure/clojurescript/commit/2711d02948c103e33ab4fd1431881edcd65095ef

Full protocol with clean checkout:
https://gist.github.com/darwin/8e9d75a4e810d6f2d9e3

btw. In this case I was unable to reproduce unstable results. When I revert back. The script/test reflect it and pass all tests.

Comment by David Nolen [ 10/Feb/15 3:44 PM ]

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





[CLJS-1028] Roll back "single-segmented namespace" warning Created: 10/Feb/15  Updated: 10/Feb/15  Resolved: 10/Feb/15

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

Type: Defect Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

This is about changed merged from CLJS-1004

I don't see why single-segment namespace is bad. Something like (ns rum) can have collision with <div id=rum>, but either can (ns rum.id) or (ns rum.lang) or ns rum.style.display).

If our goal is really robust semantic for namespaces, maybe it will be better to provide some warning when (require) instruction references non-ns object (I assume we can identify at runtime if JS object is ns or not?).

Anyway, I think CLJS-1004 patch should be rolled back



 Comments   
Comment by David Nolen [ 10/Feb/15 12:47 PM ]

We're not rolling this back. Single segment namespaces have many potential problems.

Comment by Nikita Prokopov [ 10/Feb/15 1:18 PM ]

Not for the sake of argue, but can you name a few? I really want to understand the subject

Comment by David Nolen [ 10/Feb/15 1:26 PM ]

Clashes with global objects & classpath problems. By using two or more segments you are exponentially decreasing the likelihood of either surfacing in your program.

This will not be rolled back. Disable the warning with the compiler flag if you want to suppress the warning. For third party libraries it's simply too bad. This anti-pattern has never been encouraged in Clojure ever.





[CLJS-1026] CLJS Repl doesn't respond well to clients refreshing. Created: 10/Feb/15  Updated: 10/Feb/15  Resolved: 10/Feb/15

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

Type: Defect Priority: Minor
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

CLJS 2815



 Description   

It's not uncommon for CLJS browser clients to be refreshed. This blows away the cljs.user data on the client side. And this results in the REPL erroring whenever the repl user tries to define something in the cljs.user ns in the REPL.

I solved this in figwheel repl client code by ensuring that there is at least an empty #js {} in cljs.user before evaling code sent from the REPL.

I also require cljs.repl in my figwheel repl client code to ensure its existence through reload.



 Comments   
Comment by David Nolen [ 10/Feb/15 9:36 AM ]

Is there any reason you can't resend the init client state form, (ns cljs.user ...), when the client connects again? The existing REPL infrastructure sends it once for you but having to resend on reconnect is kind of a issue specific to certain kinds of REPLs semantics.

Comment by Bruce Hauman [ 10/Feb/15 10:44 AM ]

Yeah, Say I'm in IJavaScript/-evaluate because a connection was lost and I'm waiting for a new one, after connect I'll have to call cljs.repl/evaluate-form? It just doesn't seem like the best place to do it. It seems like strange behavior could result in certain circumstances.

Does my solution above not work?

Comment by David Nolen [ 10/Feb/15 11:34 AM ]

This one is out of scope.





[CLJS-1027] server side source map helper for mapping canonicalized stack traces under :none & concatenating builds Created: 10/Feb/15  Updated: 10/Feb/15

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

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





[CLJS-1024] Double analysis warnings for dependencies in JARs Created: 10/Feb/15  Updated: 10/Feb/15

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

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


 Description   

If there is a problem with a CLJS source in a JAR, warnings will be emitted twice - once when analyzing the source in the JAR and again when the file is compiled to the output directory. Because the path has changed we don't realize we have already seen it. This probably just needs minor tweaks so that we drop warnings from the second pass.






[CLJS-1001] metadata leakage Created: 03/Feb/15  Updated: 10/Feb/15  Resolved: 10/Feb/15

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

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


 Description   

Using this template https://github.com/hellofunk/hellofunk-lein-template/blob/master/resources/leiningen/new/hellofunk/project.clj results in producing metadata leakage on maps at a minimum.



 Comments   
Comment by Andrew S [ 03/Feb/15 1:39 PM ]

Not sure this is relevant but if the :source-map line is removed, the issue remains.

Older versions of clojurescript do not have this "leakage" of file paths in the compiled JS.

Here is an example of the leakage from that template:

return (new weirdpathissue.core.t11112(owner,app,main,new cljs.core.PersistentArrayMap(null, 5, [new cljs.core.Keyword(null,"end-column","end-column",1425389514),33,new cljs.core.Keyword(null,"end-line","end-line",1837326455),14,new cljs.core.Keyword(null,"column","column",2078222095),7,new cljs.core.Keyword(null,"line","line",212345235),11,new cljs.core.Keyword(null,"file","file",-1269645878),"/Users/andrew/REPOS/THROWAWAYS/weirdpathissue/src/cljs/weirdpathissue/core.cljs"], null)));

Comment by Immo Heikkinen [ 04/Feb/15 1:18 AM ]

Seems to be related to reify. Here's minimal repro case:

(defprotocol Foo (foo [_]))

(defn new-foo []
  (reify Foo (foo [_])))

(new-foo)
Comment by David Nolen [ 10/Feb/15 6:30 AM ]

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





[CLJS-939] Quit noderepljs with EOF Created: 31/Dec/14  Updated: 09/Feb/15  Resolved: 09/Feb/15

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

Type: Defect Priority: Minor
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

As is common in every other REPL out there, Ctrl+D on a new line should quit the REPL



 Comments   
Comment by David Nolen [ 09/Feb/15 6:30 PM ]

CLJS-989





[CLJS-1023] macro-autoload-ns? and ns-dependents need to throw on cyclic dependencies Created: 09/Feb/15  Updated: 09/Feb/15  Resolved: 09/Feb/15

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

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


 Description   

Currently both will result in stack overflow as they run independently of the existing cyclic check in the parse ns portion of the analyzer.



 Comments   
Comment by David Nolen [ 09/Feb/15 6:29 PM ]

fixed https://github.com/clojure/clojurescript/commit/014524d9f04f6262dce7af13cb47c0c29db07908





[CLJS-1022] Concatenate foreign dependencies safely Created: 08/Feb/15  Updated: 08/Feb/15  Resolved: 08/Feb/15

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

Type: Defect Priority: Major
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 1022.patch    

 Description   

If there is a comment on the last line of a foreign JS dependency, it will result in the first line of the concatenated code to be commented out.

Including a patch to join foreign JS dependencies with newlines between them.



 Comments   
Comment by David Nolen [ 08/Feb/15 9:45 AM ]

fixed https://github.com/clojure/clojurescript/commit/0657b0a3ab2a606bb114620d0f66265158a7bc39





[CLJS-1011] Unified REPL stacktrace reporting Created: 05/Feb/15  Updated: 07/Feb/15  Resolved: 07/Feb/15

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

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


 Description   

Every REPL should take the original error stack string from the JavaScript target and parse into a canonical vector representation containing function name, file, and line, column. Then the core REPL architecture can provide source mapping functionality universally.



 Comments   
Comment by David Nolen [ 07/Feb/15 10:06 PM ]

fixed in master https://github.com/clojure/clojurescript/commit/ae62c6cfca71832e9ec829f073503ff892a6edd8





[CLJS-1008] Browser repl doesn't find upstream dependencies. Created: 05/Feb/15  Updated: 07/Feb/15  Resolved: 07/Feb/15

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

Type: Defect Priority: Minor
Reporter: Brian Jenkins Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl
Environment:

MacOS 10.9.5
Running a repl through cider & emacs.

project.clj attached.


Attachments: Text File fix-upstream-deps-from-repl.patch     File project.clj    
Patch: Code

 Description   

I found that when running piggieback from the repl like this:

```clojure
(defn piggieback-repl []
(cemerick.piggieback/cljs-repl :repl-env (cljs.repl.browser/repl-env :port 9000)) )
```

get-upstream-deps doesn't find anything.

This prevents me from being able to use piggieback with Om's new cljsjs dependency on React.

Comment on get-upstream-deps says

```
Should be run in the main thread. If
not, pass (java.lang.ClassLoader/getSystemClassLoader) to use the
system classloader.
```

I guess the repl isn't the main thread?



 Comments   
Comment by David Nolen [ 07/Feb/15 10:05 PM ]

fix https://github.com/clojure/clojurescript/commit/f311ebd121bf2385efcfba6f7bfb07251cf812f1





[CLJS-988] Support async testing in cljs.test Created: 27/Jan/15  Updated: 07/Feb/15  Resolved: 07/Feb/15

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None

Attachments: File cljs-988.diff     File cljs-988.diff    

 Description   

Async tests should take the form of:

(deftest foo
  (cljs.test/async done
    ...))

This should desugar into:

(deftest foo
  (reify
    cljs.test/IAsyncTest
    IFn
    (-invoke [_ done]
      ...)))

If test running code encounters an async test it should invoke the async test with the next step as the done parameter - it's simply a thunk for the continuation of testing.



 Comments   
Comment by Thomas Heller [ 28/Jan/15 4:40 AM ]

What are the benefits of using an empty marker protocol combined with IFn?

(defprotocol IAsyncTest
  (start-test [_ done]))

Seems like it would do the trick just fine since we will probably never have another arity?

Comment by David Nolen [ 28/Jan/15 6:08 AM ]

It really doesn't matter that much but it does mean the test runner code can be written in the usual functional continuation passing style.

(let [ret (test ...)]
  (if (async? ret)
    (ret k)
    ...))

vs.

(let [ret (test ...)]
  (if (async? ret)
    (start-test k)
    ...))

The former permits simple further functional combinations. The later does not. In this light it may be better to use function metadata to detect async test functions instead of a protocol.

(deftest foo
  (with-meta
    (fn [done]
      ...)
    {:async-test true}))
Comment by Leon Grapenthin [ 03/Feb/15 3:15 AM ]

I don't see the benefit of letting the test return the async test? Why not have a (defasynctest done ...) that associates the IAsyncTest obj directly into :test?
Also, wouldn't the chaining of test after test blow the call stack at some time? If I skimmed over Chas clojurescript.test code correctly, it uses setInterval or setTimeout to detect completion of asynchronous tests in mutable state, assumedly for that reason.

Comment by Leon Grapenthin [ 05/Feb/15 2:22 PM ]

Hi David,
I began working on a patch yesterday. Unless someone else is working on this, I'd like to finish it. Several difficulties and options presented themselves - it would be great if you could comment.

1. Asynchronous execution

To avoid blowing the stack, it seems that the canonical way is to use (js/setTimeout f 0). This is what I came up with so far.

(defn- schedule-async-fns
"Schedules invocation of first fn, passing it a callback and
optionally args. callback must be invoked to schedule the second fn,
again optionally with args..."
[fns & args]
(when-first [f fns]
(js/setTimeout #(apply f
(partial schedule-async-fns (rest fns))
args)
0)))

An alternative would obviously be to use core.async - js/setTimeout doesn't seem present in most browserless execution environments - is there an alternative way or workaround?

2. API of runner

By allowing tests to take over runner execution via callback, the runner API needs to change.
a. Most macros could have added arities to invoke a final callback with what they currently return. However, API compatibility can't always be preserved. Here is a list:

  • test-ns: Can have a new arity, but won't return the result anymore
  • run-tests: Can't have a new arity due to variadic overload, won't return result anymore.
    Unless API compatibility with clj is a non-goal, this certainly isn't the best idea...
    b. The current API could skip async tests, so that it can return synchronously with a meaningful result. The final test-env could include a function which could be invoked with a callback function to trigger the async-tests and receive the final test-env.
    c. As an alternative to the idea in b., it could expect a function to be present in the test-env which it would call with the final result.
    d. As an alternative to b and c, there could be an async test runner. In contrast to b and c, this test runner would run all tests in declared order.
    e. ...

It would be great if you could point in some direction.

3. API of async tests

Should tests really be able to return async tests? The only advantage I can see is that tests can run both as sync and async test. But why? Unless this is a desired usecase, I'd propose a defasynctest which would associate the IAsnycTest obj directly into test. Even simpler, it could just flag the var with :async? true. The latter would certainly make the test-runner code more straight forward - What do you think?

Comment by David Nolen [ 05/Feb/15 2:45 PM ]

Leon

1. if the tests are actually async there's no need to worry about blowing the stack.

2. people use cljs.test to run a side-effect - test reporting. That you can't return values in async isn't a real problem.

3. there will be no changes to the design I've outlined above.

Comment by Leon Grapenthin [ 06/Feb/15 6:32 AM ]

Thanks David, I think I can work with this and will get back to it.

A point I forgot to mention is fixtures. Tear down code as described in the ns docstring would have to work asynchronously as well:

The straightforward way seems to optionally call the passed function with a tear-down-fn callback.

Comment by David Nolen [ 06/Feb/15 5:15 PM ]

Yes fixtures optionally be a map of :before and :after. If we have an async test and the fixture we have in hand isn't a map I think we should abort testing and throw.

Comment by Leon Grapenthin [ 07/Feb/15 9:40 PM ]

Patch notes:

New:

  • fixtures can be specified as maps - see docstring of cljs.test
  • asnyc macro as described in CLJS-988
  • for all runner fns there are -block versions which return composable blocks for users who want to pick their own execution paths, e.g. in a test-ns-hook. Also see run-block, block.

Minor API changes:

  • all runner fns and macros return before any asynchronous testing is finished.
  • test-ns doesn't return the result anymore
  • run-test doesn't return the result anymore
Comment by Leon Grapenthin [ 07/Feb/15 9:46 PM ]

minor fix

Comment by David Nolen [ 07/Feb/15 10:03 PM ]

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





[CLJS-1020] REPL source map support off by one Created: 07/Feb/15  Updated: 07/Feb/15  Resolved: 07/Feb/15

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

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


 Comments   
Comment by David Nolen [ 07/Feb/15 8:18 PM ]

fixed https://github.com/clojure/clojurescript/commit/57277cff5ccde7cac6fe3d2784db2275ab409542





[CLJS-1021] REPL source map support needs to treat JS files correctly Created: 07/Feb/15  Updated: 07/Feb/15  Resolved: 07/Feb/15

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

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


 Comments   
Comment by David Nolen [ 07/Feb/15 8:17 PM ]

fixed https://github.com/clojure/clojurescript/commit/57277cff5ccde7cac6fe3d2784db2275ab409542





[CLJS-1019] REPL source map caching support Created: 07/Feb/15  Updated: 07/Feb/15

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

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


 Description   

Need smarter caching for better performance. Probably something based on last modified times.






[CLJS-1017] Support :main for :advanced builds Created: 06/Feb/15  Updated: 06/Feb/15

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

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


 Description   

This would use the supplied entry point to compute the build instead of looking at everything in the source directory.






[CLJS-999] JS file concatanation in preamble should place a newline between each file Created: 03/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

Type: Defect Priority: Minor
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None


 Description   

Some JavaScript files one might wish to include in a preamble are not well formed, in the sense that they end without a semicolon and without a newline.

These files work fine as long as they are in their own <script/> tag, but will cause invalid JavaScript when concatenated with any other file, as they are in a ClojureScript :preamble.

It should be trivially possible for ClojureScript to be defensive about this type of error, by inserting a newline character between concatenated files, in the process of building the preamble.



 Comments   
Comment by David Nolen [ 03/Feb/15 10:35 AM ]

We already do this.

Comment by David Nolen [ 06/Feb/15 5:38 PM ]

We already do this. Need an example of something that doesn't work though I suspect this is JS parsing issue.





[CLJS-1000] Source mapping helpers Created: 03/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

In some debugging scenarios it would be nice to provide a Clojure REPL api for mapping a JS line to the original ClojureScript line and vice versa.



 Comments   
Comment by David Nolen [ 06/Feb/15 5:09 PM ]

fixed https://github.com/clojure/clojurescript/commit/751a3eb3d5c8fcf90fe97ae6395105640cb585a3





[CLJS-1012] When *print-length* set to 0 the behaviour is inconsistent with Clojure Created: 06/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

Type: Defect Priority: Minor
Reporter: Antonin Hildebrand Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print

Attachments: Text File cljs_1012.patch    
Patch: Code and Test

 Description   

https://github.com/darwin/clojurescript/commit/fc7c91ca009528ac54cd020509b49acea4eecca9

I don't care that much about Clojure. I need this to work for cljs-devtools:
https://github.com/binaryage/cljs-devtools/commit/dcbfe1ba1aef78f300ef8fbf58c8fd1e77dd2450



 Comments   
Comment by David Nolen [ 06/Feb/15 8:48 AM ]

Patch welcome for this.

Comment by Antonin Hildebrand [ 06/Feb/15 8:53 AM ]

The patch is here:
https://github.com/darwin/clojurescript/commit/fc7c91ca009528ac54cd020509b49acea4eecca9

based on CLJS-1010 because it uses :more-text from opts. Do you want me to sign the CLA?

Comment by Antonin Hildebrand [ 06/Feb/15 8:56 AM ]

But I can fork current tip and make it independent. And then rebase CLJS-1010 on top of this.

Comment by David Nolen [ 06/Feb/15 9:07 AM ]

Patches need to be added to tickets. And yes re: CLA.

Comment by Antonin Hildebrand [ 06/Feb/15 9:44 AM ]

Got it. Patch rebased on top of master and added. I will submit :more-text change in a separate CLJS.

Comment by David Nolen [ 06/Feb/15 5:08 PM ]

fixed https://github.com/clojure/clojurescript/commit/20778cd528167910031bffcc77624628c02c9e8f





[CLJS-1015] Closure libraries using custom annotation generate warnings during compilation Created: 06/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

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

Attachments: File CLJS-1015.diff    

 Description   

Closure libraries defining their own JSDoc annotation generate warnings during compilation. This forces library consumers to rely on {:closure-warnings {:non-standard-jsdoc :off}} to hide them.

I can see 2 options to improve this:

See more context here.

What do you think?



 Comments   
Comment by David Nolen [ 06/Feb/15 9:57 AM ]

Patch for option 1 welcome.

Comment by Julien Eluard [ 06/Feb/15 4:57 PM ]

I just attached a patch for option #1.

Comment by David Nolen [ 06/Feb/15 5:06 PM ]

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





[CLJS-1013] track goog closure defines Created: 06/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

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


 Description   

Currently if you write a conditional a closure define it will be wrapped in cljs.core.truth_ call. This is unintuitive. The compiler should track all defines and emit the correct code.



 Comments   
Comment by David Nolen [ 06/Feb/15 9:45 AM ]

closure defines need not be boolean





[CLJS-1007] Add :recompile-dependents knob Created: 05/Feb/15  Updated: 06/Feb/15  Resolved: 06/Feb/15

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

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


 Description   

This is to support disabling the default. This is useful in live coding scenarios.



 Comments   
Comment by David Nolen [ 06/Feb/15 9:08 AM ]

fixed https://github.com/clojure/clojurescript/commit/65a8973e2db8a9bd48aac55d310e536ebc0dcd75





[CLJS-1003] Cannot pass custom env to run-tests Created: 04/Feb/15  Updated: 05/Feb/15  Resolved: 04/Feb/15

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

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


 Description   

Calling (cljs.test/run-tests (cljs.test/empty-env :my-reporter)) does not actually pass the environment with a custom reporter key onwards. I think the problem lies here: https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/test.clj#L257

We should pass ~env-or-ns argument to cljs.test/test-ns call, because now the single-arg version internally creates a new empty environment (see: https://github.com/clojure/clojurescript/blob/master/src/clj/cljs/test.clj#L309).



 Comments   
Comment by Kimmo Koskinen [ 04/Feb/15 9:01 AM ]

Seems that adding ~env-or-ns to cljs.test/test-ns call in test-ns isn't enough. Summary is still reported with the :cljs.test/default reporter.

Comment by David Nolen [ 04/Feb/15 2:35 PM ]

Kimmo do you have a minimal example of something that you think should work so that we can use it as a test? Thanks.

Comment by Kimmo Koskinen [ 04/Feb/15 2:49 PM ]

Could this be of help? https://gist.github.com/viesti/bba5c88abbb2f6ecb914

The call to (test/run-tests (test/empty-env :karma)) should print a message "Testing with Karma myapp.test" instead of "Testing myapp.test".

Comment by David Nolen [ 04/Feb/15 6:38 PM ]

fixed https://github.com/clojure/clojurescript/commit/3bbd9756d7984ecb7d530d31f3b867c0a58ae35a

Comment by Kimmo Koskinen [ 05/Feb/15 2:01 AM ]

Thanks for that fast reply! I tried the patch quickly, and report method is called on the :begin-test-ns phase correctly. However, on the :summary phase, my custom report method isn't called.

I updated the gist: https://gist.github.com/viesti/bba5c88abbb2f6ecb914. It should print "All done" at end of the test, instead of "Ran 1 tests containing 1 assertions.
0 failures, 0 errors.".

Comment by David Nolen [ 05/Feb/15 6:53 AM ]

fixed in master https://github.com/clojure/clojurescript/commit/647f775f01fad563a9c7b481a7e15273a8cff884

Comment by Kimmo Koskinen [ 05/Feb/15 7:22 AM ]

Thanks!





[CLJS-1006] Implicit dependency of clojure.browser.repl on cljs.repl Created: 05/Feb/15  Updated: 05/Feb/15  Resolved: 05/Feb/15

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

Type: Defect Priority: Minor
Reporter: Murphy McMahon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl

Attachments: Text File 1006.patch    

 Description   

The clojure.browser.repl/connect function monkey patches goog.require with :optimizations :none. The substituted version uses goog.basePath-relative URLs to retrieve assets, one of which is cljs.repl (the repl-connection callback returns "goog.require('cljs.repl')"). Therefore clojure.browser.repl has an implicit dependency on cljs.repl, which will not be able to be located if the :output-dir of application compilation (eg cljsbuild) and that passed to cljs.repl/repl* are not one in the same.



 Comments   
Comment by David Nolen [ 05/Feb/15 7:13 AM ]

fixed https://github.com/clojure/clojurescript/commit/576fd1c70933fc91180c769cb639c262b7c79071





[CLJS-1005] Browser REPL creates "out" directory no matter what Created: 05/Feb/15  Updated: 05/Feb/15  Resolved: 05/Feb/15

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

Type: Defect Priority: Minor
Reporter: Murphy McMahon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl

Attachments: Text File 1005.patch    

 Description   

Steps to reproduce:

$ lein new mies test-brepl
$ cd test-brepl
$ sed -i 's/:output-dir "out"/:output-dir "not-out"/' scripts/brepl.clj
$ ./scripts/brepl # <Ctrl-c> <Ctrl-c>
$ ls -la

Expected: Only "not-out" and ".repl-0.0-XXXX" directories should be created for compiled javascript.

Actual: "out" directory is also created.



 Comments   
Comment by David Nolen [ 05/Feb/15 7:13 AM ]

fixed https://github.com/clojure/clojurescript/commit/4bd3e6563d36871b1c5e603c35fc7bb4a9b68eb2





[CLJS-1004] Single segment namespaces are not properly loaded Created: 04/Feb/15  Updated: 05/Feb/15  Resolved: 05/Feb/15

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

Type: Defect Priority: Minor
Reporter: Martin Klepsch Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File 1004-single-segment-ns-warning.diff    

 Description   

When having a namespace app and supplying it to the compilers :main option it is not properly loaded by the resulting Javascript file (output-to).

A test case is in this repository: https://github.com/martinklepsch/single-segment-cljs-ns

I'm not sure what the error here is exactly. The files that get loaded look fine and I don't see an issue with how the dependency is registered:

{{goog.addDependency("../app.js", ['app'], ['cljs.core', 'om.dom', 'om.core']);}}

Still when later goog.require("app"); is called it returns undefined and has no effect on the DOM.

Happy to look into this if someone can provide me with some pointers.



 Comments   
Comment by Martin Klepsch [ 05/Feb/15 5:46 AM ]
goog.isProvided_ = function(name) {
    return !goog.implicitNamespaces_[name] &&
        goog.isDefAndNotNull(goog.getObjectByName(name));
  };

When it's checked if a namespace is already provided goog.getObjectByName is called.
When there is a DOM node with the same id that is passed to goog.getObjectByName
it returns the DOM node and assumes everything that has been required is present.
Therefore the dependency resolution process stops early.

Comment by Martin Klepsch [ 05/Feb/15 5:49 AM ]

This patch adds a warning when using namespaces with only a single segment (i.e. no dot).

WARNING: Using single segment namespaces (app) is discouraged. at line 1 ssrc/app.cljs

Comment by David Nolen [ 05/Feb/15 7:08 AM ]

Didn't notice the patch! Fixed in master.

https://github.com/clojure/clojurescript/commit/dc47f1fa1483871641c34f765034efedb7238f28





[CLJS-995] deps.js contains references to js files not found in out/ Created: 01/Feb/15  Updated: 04/Feb/15  Resolved: 04/Feb/15

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

Type: Defect Priority: Minor
Reporter: Mike Thompson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Version 2665 was okay. Version 2719 shows the problem.



 Description   

When using ":optimisations :none" ...

In 2665 and prior, the list of goog.Dependency entries in deps.js appears to have been filtered/trimmed in some way, and it matched the js contents of out/. In effect, deps.js did not contain references to any js file which didn't exist in out/

As of 2719 release, I'm seeing a really large deps.js file which contains goog.Dependency entries referencing js files which are not present in out/



 Comments   
Comment by Mike Thompson [ 01/Feb/15 8:04 PM ]

< I can't seem to edit my ticket description. Puzzling>

I should stress that this will NOT cause a problem generally. If all you do is goog.require() a root namespace, then the dependencies are still correct, and everything works.

I only noticed this issue because, as you know, I am doing this:

for(var namespace in goog.dependencies_.nameToPath)
goog.require(namespace);

And, whereas that worked fine in 2665 (and prior), in 2719 (and later) it results in a lot of js files being imported which do not exist (in out/). The non-existence of js files referenced in <script>s causes copious console errors, which is not a showstopper, but it is untidy.

So, my "hack" still works, albeit now with copious warnings, but it just doesn't seem right that deps.js contains code that references non-existent js files.

Comment by David Nolen [ 01/Feb/15 10:47 PM ]

The previous behavior caused other problems - deps.js would not get updated consistently. I would take a patch that is guaranteed to keep deps.js minimal and up to date.

Comment by Thomas Heller [ 02/Feb/15 3:12 AM ]

@Mike: Instead of doing a require ALL you could just requires the namespaces that require cljs.test (or the cemerick variant).

{{goog.dependencies_.nameToPath["cljs.test"]}} gives you the filename {{goog.dependencies_.requires[name]}} gives you everything that requires it. If you goog.require those it will bring in all other dependencies. That might work arround the problem with deps.js.

Comment by Mike Thompson [ 02/Feb/15 3:15 AM ]

I've gone through all the checkins between the two releases, and it looks as if this is the commit which changed the behaviour:
https://github.com/clojure/clojurescript/commit/e726c22fcc77bfd38cc17a2f0d8349c43c83add5
made when addressing CLJS-963

I've had a look at http://dev.clojure.org/jira/browse/CLJS-963 but details about the problem to be avoided are scant.

I'll keep looking.

Comment by Mike Thompson [ 02/Feb/15 3:16 AM ]

Thanks Thomas! Will experiment.

Comment by Stuart Mitchell [ 04/Feb/15 5:16 PM ]

Replace the hack

for(var namespace in goog.dependencies_.nameToPath)
   goog.require(namespace);

with this

//find out what requires cljs.core
 for(var key in goog.dependencies_.requires) {
     if (goog.dependencies_.requires.hasOwnProperty(key)) {
         if (goog.dependencies_.requires[key]["cljs.core"]) {
             //as key is a path find its namespace
             for (var namespace in goog.dependencies_.pathToNames[key])
                 goog.require(namespace);
         }
     }
 }
Comment by David Nolen [ 04/Feb/15 6:06 PM ]

Not changing this for now.

Comment by Mike Thompson [ 04/Feb/15 8:30 PM ]

Hack in use has been modified as per Thomas' and Stu's suggestion. Thanks!





[CLJS-901] Protocolize compiler access to the file system Created: 03/Dec/14  Updated: 04/Feb/15

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

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


 Description   

Currently builds are based on files on disk. It is desirable to be able to instead get in memory builds, WebDAV based builds, S3 based builds, etc. Many of these alternative strategies are not in scope for the ClojureScript compiler but this does not mean we should not supply the needed hooks for users to control the behavior.



 Comments   
Comment by Thomas Heller [ 03/Dec/14 9:31 AM ]

This and some other issues opened recently (CLJS-900, CLJS-851, CLJS-899, ...) have some overlap with what I have done in shadow-build [1]. Memory based builds are actually already possible cause it will only touch the disk when asked to, although the API could use some cleanup.

Anyways, might be worthwhile to coordinate these efforts to make CLJS more accessible for everyone.

[1] https://github.com/thheller/shadow-build

Comment by Alan Dipert [ 04/Feb/15 11:36 AM ]

I too think it would be totally awesome to have builds based on sources from disparate places.

One alternative in this spirit I have been thinking about is a "SourceSet" approach. The idea is, instead of teaching CLJS how to consume various place-types directly via protocols, provide an API for building a "SourceSet" value and also a build function that takes the SourceSet as input. I imagine the SourceSet in its simplest form as a map of namespaces to string sources.

With a value to represent sources that is place-agnostic and immutable, 3rd party tools can consume/emit/transform these values before invoking a compile without knowledge or interest in CLJS internals. Conversely CLJS need not be concerned with how SourceSets are constructed.

This whole idea is inspired by boot's FileSets, which work basically the same but can't have the "it fits in memory" assumption.





[CLJS-1002] `cljs.closure/get-upstream-deps`: Allow caller to specify classloader to use Created: 03/Feb/15  Updated: 03/Feb/15  Resolved: 03/Feb/15

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

Type: Enhancement Priority: Minor
Reporter: Moritz Ulrich Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch
Environment:

Clojurescript, commit d124f71


Attachments: Text File 0001-cljs.closure-get-upstream-deps-Add-optional-CLASSLOA.patch     Text File 0001-cljs.closure-get-upstream-deps-Add-optional-CLASSLOA.patch     Text File 0002-cljs.closure-get-upstream-deps-Correct-arity-order-r.patch    
Patch: Code

 Description   

Some users of cljs.closure/get-upstream-deps don't run it from the main thread. Because it uses the current thread's classloder, it just returns an empty list.

This patch allows callers to specify the classloader to use, for example (java.lang.ClassLoader/getSystemClassLoader) (which fixes the function for weasel)



 Comments   
Comment by Moritz Ulrich [ 03/Feb/15 4:54 PM ]

Combined patch, should apply to d124f71b4fa7dd3ceecb064eaf5a2f7392e738e1

Comment by David Nolen [ 03/Feb/15 4:58 PM ]

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





[CLJS-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 03/Feb/15

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    

 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.)





[CLJS-847] TypeError in cljs.core/str using Safari 6.0.5 Created: 29/Aug/14  Updated: 03/Feb/15  Resolved: 31/Aug/14

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

Type: Defect Priority: Major
Reporter: Kevin Neaton Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-847.patch    

 Description   

In some versions of Safari 6 (at least 6.0.4 and 6.0.5) calls to cljs.core/str may throw a very cryptic Type Error: type error. This error has occurred repeatedly in our production cljs app over the last ~3 months but I have not been able to reproduce it locally, or boil it down to a simple test case. This appears to be due to the nature of the bug itself. I was however, able to workaround the issue by patching cljs.core/str to use the '' + x form of coercion instead of calling x.toString directly.

Other js projects have encountered this issue and adopted the same solution:

This shouldn't hurt performance and might actually improve it in some browsers:

I'll submit the patch we are using shortly.



 Comments   
Comment by David Nolen [ 31/Aug/14 9:41 AM ]

thanks for the report this is now fixed in master https://github.com/clojure/clojurescript/commit/08b4b1585cf0ef739e903985ee4c6b7fc6c47642.

Comment by Kevin Neaton [ 31/Aug/14 1:30 PM ]

My pleasure. Thanks for the quick turnaround!

Comment by Nikita Beloglazov [ 23/Nov/14 5:30 PM ]

Seems like using concatenation breaks some cases when valueOf and toString methods are overriden. Example:

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

So ClojureScript str function will return '42' for that object, but in fact it should return 'hello'. How about using String() to convert object to strings? I have no idea whether it breaks in Safari or not, may be Safari breaks only on toString() and not String().

Comment by Kevin Neaton [ 23/Nov/14 11:09 PM ]

Since this issue is closed, you might consider creating a new one. That said, I suspect `String` would work as well but I have no way to check, since the original error is so difficult to reproduce.

Comment by David Nolen [ 24/Nov/14 7:27 AM ]

Please open a new issue, thanks.

Comment by Nikita Beloglazov [ 24/Nov/14 9:52 AM ]

Sorry, should have thought about that myself. Done: http://dev.clojure.org/jira/browse/CLJS-890

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

Kevin, we can't accept your fix because ''+x is not the same as x.toString because the former is like x.valueOf().toString(). We're investigating alternatives but we cannot reproduce your failure. See the end of CLJS-890 for details. If you have a way of reproducing or testing things in Safari 6.0.x your help would be appreciated. A simple thing you could do is open/run the following urls on browsers which gave you trouble:

  1. This file is a minimal reproducible case from this jsbinfrom someone with the same problem outside cljs. It should fail with TypeError: type error on Safari 6.0.x. Can you see if it does? (It doesn't for me on Lion Safari 6.0.5 via BrowserStack.)
  2. This jsperf. The str-tostr should fail on Safari 6.0.5 but I can't get it to fail. Let me know if any of the alternatives work.
  3. Finally, this page is my attempt at a minimal reproducible. I expected at least one of the cases to fail, but again, I can't get any to fail.

Thanks in advance.

Comment by Kevin Neaton [ 11/Jan/15 7:28 PM ]

I have not been able to reproduce this error myself since I do not have access to a computer with Safari 6.0.x. However, this issue has been reproduced (outside of cljs) for other projects experiencing this issue. Based on this and this it seems that the browser console actually needs to be closed for the error to occur. Hopefully, that helps.

Comment by Francis Avila [ 18/Jan/15 9:52 PM ]

We're aware of these other reports of problems (and that the console must be closed because it is JIT-related) but none of them actually provide a test case to reproduce. With some more digging I was able to find this issue which provided this test case (which is the first link in my previous post), but in fact I could not get it to fail on Safari 6.0.5.

How were you able to determine this failed on Safari 6.0.5 and that your fix actually fixed it? Was it all second hand guesswork through these github issues or did you actually see the problem and its resolution? Is there any way for you to confirm that some other patch (other than ''+x) solves this error for you? As it stands we cannot accept your patch but we also cannot reproduce and thus cannot write an acceptable alternative patch.

Comment by Kevin Neaton [ 18/Jan/15 10:31 PM ]

How were you able to determine this failed on Safari 6.0.5 and that your fix actually fixed it?

We noticed the error regularly in production logs for an app running Clojurescript 0.0.2202. The stack trace indicated that the error occurred in native code upon calling toString. The browser version logged along with the errors was always Safari 6.0.5 or 6.0.4. (I can provide you with a sampling of user agent strings that produced the error if that would help). We added the patch above an the error never occurred again.

Is there any way for you to confirm that some other patch (other than ''+x) solves this error for you?

The app that produced the error is now stable and we do not have any updates planned at the moment. I can talk to my boss about deploying an alternate patch to a subset of our users, but it might be a tough sell. Is that what you had in mind?

Comment by Francis Avila [ 19/Jan/15 9:18 AM ]

I can talk to my boss about deploying an alternate patch to a subset of our users, but it might be a tough sell. Is that what you had in mind?

We really need to see this bug in a controlled environment. Deploying a test on the open internet and waiting for stacktraces is not a definitive confirmation of a fix. (How many Safari 6.0.x users do you get anyway? They should be very rare.)

If there is a url in your app that you know raises TypeError, this would be the best way to go (and would not require changing your production code):

  1. With app built with unpatched cljs 0.0.2202: hit page with Safari 6.0.5, confirm TypeError.
  2. With your production code (patched cljs 0.0.2202): hit page with Safari 6.0.5, confirm no TypeError.
  3. With some other patch: confirm no TypeError.

BrowserStack is the only browser testing platform I could find which has Safari 6.0.5, and as I said I could not reproduce the error using it.

One more question: what were your cljs compilation settings?

Comment by Kevin Neaton [ 19/Jan/15 5:05 PM ]

Here are the compiler settings from the build that produced the error:

{:output-to "target/main.js"
 :output-dir "target/main"
 :optimizations :advanced
 :output-wrapper true
 :pretty-print false
 :preamble ["react/react_with_addons.min.js"]
 :externs ["react/react_with_addons.js"]
 :closure-warnings {:externs-validation :off
                    :non-standard-jsdoc :off}}

I had no luck reproducing the issue previously, but I'll take another stab at it later this week and report back here.

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

There is a new candidate patch in CLJS-890 now.

Comment by Kevin Neaton [ 03/Feb/15 9:29 AM ]

I have had no luck reproducing this bug in Safari 6.0.5 on BrowserStack. Others have reported elsewhere that simply logging a message to the console may mask the bug. That said, I am not confident that it is possible to reproduce this bug using BrowserStack.

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

I'm not 100% sure either that BrowserStack's code injection magic doesn't somehow mitigate this bug. However, based on the jsperfs I have been running inside it I am sure that it does not disable the higher code optimization levels where this bug would manifest. (This is why console.log calls and opening the developer console mask the bug: both disable code optimization, and this is a bug that only manifests at higher optimization levels.)

So it is at least possible that BrowserStack's Safari 6.0.5 would be able to trigger this bug, or rather I have not seen anything about it yet that absolutely precludes triggering the bug.





[CLJS-997] Nashorn repl bindings Created: 02/Feb/15  Updated: 02/Feb/15  Resolved: 02/Feb/15

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

Type: Enhancement Priority: Major
Reporter: Pieter van Prooijen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Jdk 8 or higher


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

 Description   

Hello,

I've created a repl binding to the Nashorn script engine, similar to the Rhino and node.js repls. Although Nashorn is slow loading javascript code, it still has a reasonable startup time (about 10s with fast trampolines, cache-analysis enabled and AOT compiled libraries on a four year old laptop).

The code could be useful for people wanting to minimize the number of external dependencies in their project and could also offer a starting point when trying out with server-side Clojurescript using Nashorn.
The file contains comments on how to setup the repl under leiningen and nrepl / piggieback.

Kind regards,

Pieter van Prooijen

(Patch follows shortly)



 Comments   
Comment by David Nolen [ 02/Feb/15 12:50 PM ]

Pieter, thanks. In order for the patch to be applied you must sign the Contributor Agreement - http://clojure.org/contributing.

Comment by Pieter van Prooijen [ 02/Feb/15 12:56 PM ]

Patch against 2761

Comment by Pieter van Prooijen [ 02/Feb/15 1:03 PM ]

David, I've signed the agreement.

Comment by David Nolen [ 02/Feb/15 2:57 PM ]

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





[CLJS-996] .indexOf method(in javascript) returns wrong result when called with keyword array Created: 02/Feb/15  Updated: 02/Feb/15  Resolved: 02/Feb/15

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

Type: Defect Priority: Major
Reporter: sorrow17 Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, cljs
Environment:

Windows, clojurescript-2755



 Description   

The following expression should return "1":

(.indexOf (to-array [:a :b :c]) :b)

but actually the result is -1.
Note that this method call behaves normally with other type of array:

>(.indexOf (to-array ["a" "b" "c"]) "b")
>1

So I guess there might something wrong with the type Keyword.
Anyone have a look at this?



 Comments   
Comment by Francis Avila [ 02/Feb/15 8:18 AM ]

Array.prototype.indexOf uses strict equality (js === operator) to compare search item with array items. Keywords are Objects, and the same keyword-equal object may not be the same identical object. Non identical objects never compare strictly equal in js and there is nothing that can be done to change this.

Use something other than indexOf to find your item. You need to compare items with the cljs = function or with keyword-equal?

Comment by David Nolen [ 02/Feb/15 9:40 AM ]

This is not a bug.





[CLJS-957] Parallel compilation Created: 03/Jan/15  Updated: 02/Feb/15

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

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


 Description   

For non-interacting branches of the dependency graph we could compile in parallel.



 Comments   
Comment by Thomas Heller [ 03/Jan/15 3:06 PM ]

Just a heads-up: I attempted this in shadow-build.

The problem with this is macros, specifically their require. Granted you said non-interacting but suppose two namespaces require cljs.core.async.macros, they might trigger the clojure.core/require at the same time (parallel after all).

This resulted in a very weird exception: https://www.refheap.com/91731

I'm not sure how thread-safe clojure.core/require is supposed to be but it was as close as I was able to get tracking this weird error down. Removing reducers fixed everything. FWIW the gain was minimal to begin with.

Comment by David Nolen [ 03/Jan/15 3:09 PM ]

Making this really work I think largely depends on how much time you are willing to spend on the problem. Fork/join work stealing seems ideal for this not sure if you tried that. RE: macros, seems like you could use information collected from ns analysis to serialize calls to require.

Comment by Thomas Heller [ 03/Jan/15 3:28 PM ]

Lets throw some reducers at it was about as much thought I put into it.

Just beware of macros when you attempt to do something is all I wanted to say. Treating analysis seperate from cljs.compiler/emit might be a good idea though.

Comment by Dusan Maliarik [ 06/Jan/15 7:20 AM ]

I always thought parallel compilation belongs to the build tool (process), and should be parallelized per source file. Would there be any gain in using pmap in here ? It's used by lein-cljsbuild I suppose. Or parallelize this part?

Comment by David Nolen [ 06/Jan/15 7:54 AM ]

pmap won't work because of dependencies. A work queue approach is probably going to be most fruitful. Let's keep this thread clean unless you are actually going to work on this ticket and have a clear picture how to do it. Thanks.

Comment by John Chijioke [ 02/Feb/15 8:14 AM ]

David, Where exactly should the parallelization happen?





[CLJS-927] :recompile-dependents build option Created: 24/Dec/14  Updated: 01/Feb/15  Resolved: 01/Feb/15

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Completed Votes: 5
Labels: None


 Description   

It should be possible to optionally recompile dependent namespaces. This would make disk based caching less prone to strange errors because the user didn't do this manually.



 Comments   
Comment by David Nolen [ 25/Dec/14 12:55 AM ]

cljs.closure/add-dependencies looks like the right place to do this. For this to work we would need to always sort files into dependency order before compilation. While this is done for cljs.closure/add-dependencies it is not done for files encountered in the source directory as a result of the implementation of cljs.compiler/compile-root.

If files were always guaranteed to be compiled in dependency order we could then easily pass the dependency information along and use this in cljs.compiler/requires-compilation? - a file would then additionally require compilation if any of the namespaces it depends on was recompiled.

Comment by David Nolen [ 01/Feb/15 2:02 PM ]

fixed https://github.com/clojure/clojurescript/commit/05d0209c388bb1955af7170d573afae4d26695e1





[CLJS-994] print a warning when :externs file paths can't be found. Created: 30/Jan/15  Updated: 01/Feb/15

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

Type: Enhancement Priority: Trivial
Reporter: Crispin Wellington Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)


Attachments: Text File clojurescript-extern-missing-warning.patch    
Patch: Code

 Description   

clojurescript silently ignores missing externs files possibly leading a developer to chase their tail.

Presently it can be very confusing using advanced compilation if you have made a mistake in the path name of one of your :externs files. This patch makes the compiler print a warning on stderr so you can quickly determine the cause of the broken advanced compilation output.

As a side effect, when doing a basic lein-cljsbuild a warning is always printed:

```
WARNING: js resource path closure-js/externs does not exist
```

This is because lein-cljsbuild quietly adds this extra path to your :externs listing without you knowing.



 Comments   
Comment by David Nolen [ 31/Jan/15 1:59 PM ]

You need to bind *out* to *err*, or just print to it directly a la cljs.util/debug-prn.

Comment by Crispin Wellington [ 31/Jan/15 7:30 PM ]

I did bind out to err. Check the patch.

Comment by David Nolen [ 01/Feb/15 12:30 PM ]

Crispin, oops sorry you are correct. Thanks.





[CLJS-913] Error when trying to convert javascript object to clojure (js->clj obj) under :advanced compilation Created: 18/Dec/14  Updated: 31/Jan/15  Resolved: 31/Jan/15

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

Type: Defect Priority: Major
Reporter: Erik Wickstrom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None