<< Back to previous view

[CLJS-992] satisfies? does not work with locally bound symbols Created: 28/Jan/15  Updated: 28/Jan/15

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

Type: Defect Priority: Minor
Reporter: Richard Davies Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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



 Description   

(ns sample)
(defprotocol ICustomer (doit [_]))
(defrecord Customer [fname])
(extend-type Customer
ICustomer
(doit [_] nil))
(let [t ICustomer]
(js/alert (satisfies? t (Customer. nil))))

;satisfies? returns false but similar code in returns true.






[CLJS-987] Introduce Special :main marker value to help with unit testing. Created: 26/Jan/15  Updated: 28/Jan/15

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

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


 Description   

This is a follow on enhancement building on http://dev.clojure.org/jira/browse/CLJS-851 (simplify :none script inclusion if :main supplied)

I'd like to improve the unit test story for :optimizations :none

Explanation: https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L69-L93

I propose that if ":main" has a value of "*" then instead of single require, this loop would be put in:
https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L110-L111



 Comments   
Comment by David Nolen [ 27/Jan/15 12:20 PM ]

We're not going to do this. I would be up for allowing a sequence of namespaces.

Comment by Mike Thompson [ 27/Jan/15 3:23 PM ]

Instead of a sequence, how about namespace wildcarding? Eg: :main "some-ns.test.*"

Why? Well, I really, really don't want to manually maintain a sequence. It will be a guaranteed pit of failure. If I have 3 developers working on a project, each adding and deleting unit-test "cljs" files in a certain directory, manual maintenance of that sequence will be error prone, verbose and horrible. tests will be accidentally omitted, some might, wastefully be included twice, etc. Uggh.

I guess I'm saying that manual maintenance of a sequence does not scale up, in a unittest context.

All the good unit test frameworks I've used over the years (eg Nose under python) have a "a runner" ... you point it at a directory, and it recursively "finds" all the unittests in that directory. You never have to list the tests. Instead there is a set of rules around identifying tests:

  • must be in a (sub) directory called "test"
  • must start with "test_" ;; equivalent to wildcarding etc

These "rules" are pretty much just wildcards (which is what I'm proposing) ... discovery by pattern.

Comment by David Nolen [ 28/Jan/15 8:13 AM ]

Mike, we're just not going to do it sorry. Sugar like this is best handled by third party tools like lein-cljsbuild, figwheel, etc.

Also perhaps you're not aware of cljs.test/run-all-tests? It takes an optional regex.

Comment by Mike Thompson [ 28/Jan/15 7:36 PM ]

David, that you are suggesting "run-all-tests" as a solution means I've failed to explain the problem.

RUNNING all the tests is not hard in any of the unittest runners. That is not the issue.

The problem is that of LOADING (goog.require) the namespaces of the unittests WHEN you use `optimisations :none`. In that particular case, there is no root namespace you can `goog.require` which triggers all the unittests cljs into the browser (or Nodejs).

Note: if you use `:optimisation :simple` the problem disappears because all the code comes in one big blob. All the unittests are concatenated into the one file. Easy then to run them, when they are all loaded.

It is `:optimisations :none` which causes the problem, and only when combined with an unknown, flat set of unit-test namespaces (a directory of unittests in cljs files)

I can't do better than this explanation: https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L69-L93

And lein-cljsbuild can't help. Anyway, I've got a feeling you've given your answer on this. Maybe you'll revisit this subject when you get around to getting cljs.test to work with `:optimisations :none`. In the meantime, we'll continue to use the hack.





[CLJS-991] Wrong inference - inconsistent behavior? Created: 28/Jan/15  Updated: 28/Jan/15

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

Type: Defect Priority: Minor
Reporter: Christian Fortin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The following function will cause a warning to appear when compiling:

(defn test-fn [a]
(let [b (when a (aget js/document "documentElement" "clientHeight"))]
(- 10 b 2)))

WARNING: cljs.core/-, all arguments must be numbers, got number #tok0-block-tok instead. at line 12 src/hello_world/core.cljs

This one will compile without any problem:

(defn test-fn2 []
(let [b (rand-nth [nil (aget js/document "documentElement" "clientHeight")])]
(- 10 b 2)))






[CLJS-988] Support async testing in cljs.test Created: 27/Jan/15  Updated: 28/Jan/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   

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}))




[CLJS-990] Clojurescript records do not have same equality semantics as Clojure records Created: 27/Jan/15  Updated: 27/Jan/15

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

Type: Defect Priority: Major
Reporter: Richard Davies Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

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



 Description   

(defrecord Pot [a])

(= (Pot. 1) (Pot. 1)) ; returns false

This arose when I was trying to get some code to cross-compile between Clojure and ClojureScript and the Clojure code was using records as map keys.

Workaround:

(extend-type Pot
IEquiv
(-equiv [this that] (and (instance? Pot that) (= (into {} this) (into {} that)))))

Can this behaviour be baked into ClojureScript records by default?



 Comments   
Comment by Richard Davies [ 27/Jan/15 11:27 PM ]

I tried with the latest version of ClojureScript and this works (in isolation). However, when I compile it along with the rest of my code, the equality test still fails without extend-type. I will try to isolate the root case.





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

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

Type: Defect Priority: Major
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Unresolved Votes: 1
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.





[CLJS-969] Node REPL fails with TypeError under Windows Created: 08/Jan/15  Updated: 27/Jan/15  Resolved: 27/Jan/15

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

Type: Defect Priority: Major
Reporter: Adrian Sampaleanu Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Windows 8.1, Node.js 0.10.35



 Description   

With latest source and following the steps outlined in The Node.js REPL of My Dreams, the following error is emitted when noderepljs is invoked:

$ ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 58343
"Error evaluating:" (do (.require js/goog "cljs.core") (set! *print-fn* (.-print (js/require "util")))) :as "goog.require(\"cljs.core\");\r\n\r\ncljs.core._STAR_print_fn_STAR_ = require(\"
util\").print;\r\n"
TypeError: Cannot read property 'nameToPath' of undefined
    at Object.goog.require (repl:2:49)
    at repl:1:6
    at Socket.<anonymous> ([stdin]:30:80)
    at Socket.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:765:14)
    at Socket.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:427:10)
    at emitReadable (_stream_readable.js:423:5)
    at readableAddChunk (_stream_readable.js:166:9)
    at Socket.Readable.push (_stream_readable.js:128:10)
ClojureScript:cljs.user>

Though the correct prompt appears, the REPL is non functional.



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

Thanks for the report. Sadly there are no cycles to work on Windows issues ourselves. Patches from Windows users are alway welcome.

Comment by David Nolen [ 27/Jan/15 12:23 PM ]

Should be fixed in master





[CLJS-851] simplify :none script inclusion if :main supplied Created: 05/Sep/14  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

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

Attachments: Text File 0001-Emit-require-for-main-namespace-do-deps-file.patch     Text File 0002-Emit-goog.base-configuration-constants.patch     Text File 0003-Emit-goog.base-into-deps-file.patch     Text File 0004-Emit-all-deps-into-a-single-file.patch     Text File 0005-Update-samples-to-new-none-opt-usage.patch    
Patch: Code

 Description   

If :main namespace supplied - under :none optimization settings :output-to file should goog.require it. This would also allow script inclusion to be unified regardless of the level of optimization supplied, i.e.:

<script type="text/javascript" src="foo.js"></script>

Instead of

<script type="text/javascript" src="out/goog/base.js"></script>
<script type="text/javascript" src="foo.js"></script>
<script type="text/javascript">goog.require("foo.core");</script>


 Comments   
Comment by David Nolen [ 05/Sep/14 5:10 PM ]

This does mean concatenating the contents of goog.base into the deps file.

Comment by Thomas Heller [ 06/Sep/14 8:31 AM ]

Just a quick Note: Not sure what your plan is for :main but it in my experience it is important to make it a collection. A larger app quickly gains multiple entry points.

Comment by David Nolen [ 06/Sep/14 9:38 AM ]

I don't see the point of making it a collection, users can simply load other entry points themselves via the "main" namespace.

Comment by Andrew Rosa [ 14/Oct/14 5:28 PM ]

Is someone already working on this issue? I will familiarize with the compiler code to try tackle this feature.

Comment by Andrew Rosa [ 15/Oct/14 10:17 PM ]

@dnolen I made the changes to compiler for auto require an specified :main namespace (but still optional). After that I started to experiment adding the goog.base code directly into the generated js, just to see what happens.

What I found is that we will need to disable the Closure default deps.js file. So we need to copy the goog.base code and after that include all dependencies, including the Closure ones. This work is need to avoid two issues:

  • We need to explicit set the base path for loading, since the auto-discovery method used by Closure searches for a <script str="base.js">.
  • By default, Closure automatically includes a <script> containing the goog/deps.js file to include itself. This causes race conditions during our require('main-ns'), since only half of our dependencies was added to dependency graph. Explicitly requiring everything will avoid this bug.

I could change the compiler to accommodate all this changes, but first like to communicate you the deep effects on our current behavior. What do you think?

Comment by Thomas Heller [ 16/Oct/14 2:24 AM ]

I have implemented this feature in shadow-build [1] and use the following strategy:

Assuming our output file is named "app.js" for an optimized build, it will look basically like this when compiling with :none.

1) imul.js fix
2) closure global settings

I found that these are required

var CLOSURE_NO_DEPS = true;
var CLOSURE_BASE_PATH = '/assets/js/src/';

The CLOSURE_BASE_PATH is required for the include scripts to find the javascripts for each namespace.

3) After the settings the goog/base.js is emitted into the file.
4) After that the contents of dependency settings are emitted (basically goog/deps.js) plus all of our namespaces. They look something like this

goog.addDependency("clojure/set.js",['clojure.set'],['cljs.core']);

When closure attempts to load a given namespace it will simply emit a <script src="CLOSURE_BASE_PATH + path/to/ns.js">, I found that using an absolute path is the best way to ensure that files are loaded correctly since the "path detection" feature of the goog/base.js file fails since we do not include a goog/base.js

5) Then finally you can just add the

goog.require('main-ns');

Hope that helps.

[1] https://github.com/thheller/shadow-build

Comment by Andrew Rosa [ 16/Oct/14 7:35 AM ]

Thank you Thomas for your very nice writeup.

In my tests I found that we could leave CLOSURE_BASE_PATH alone IF we made everything relative to our output file. Do you think forcing it to some path is more reliable?

Comment by Thomas Heller [ 16/Oct/14 1:08 PM ]

Yes definitly. The location of the javascript files usually is "fixed" once the app is configured. Either you put it locally at something like "/js" or something like a CDN, so you configure it once and forget about it. But thats my own personal opinion, I have no idea how other people are handling it. I had some problems with relative paths but can't remember exactly what they were only that CLOSURE_BASE_PATH fixed it.

Comment by Andrew Rosa [ 24/Oct/14 4:52 PM ]

Just to give an update here: I already have a patch for this issue, but depends on the resolution of CLJS-874.

Comment by Andrew Rosa [ 08/Nov/14 7:59 PM ]

Here are the patches to fix this issue. I made smaller patches for each "step" to light the burden of code review.

  • 0001: this issue creates a new compiler option :main, which receives a symbol with the "main namespace" similar to Clojure. If supplied, will emit a require at the end of our custom deps file.
  • 0002: here I put the Closure configuration constants to make the loading works. I set the goog/base.js dir as a relative path so the change will be less intrusive to cljs compiler.
  • 0003: Emit whole goog.base
  • 0004: combine all deps into our single file, to avoid async dependency loading between google closure libs and our deps.
  • 0005: Update all sample documentation and remove the old "*-dev.html" files.

Thomas: thank you for the help and the examples. In the end I removed the need to know absolute paths by just considering we can aways reach the goog/base directory. This leads to less code involved to make the feature available.

Please let me know if there is any problem with those patches, so I could fix them. This is my first contribution to Clojure(Script), so I still need to grasp the workflow here.

Comment by Martin Klepsch [ 27/Nov/14 10:55 AM ]

@Andrew, I just applied your patches and got the Twitterbuzz example running.
You've done great work, lot's of people will appreciate this change.

One issue I encountered doing that:

In the Twitterbuzz Readme there's a statement as follows:

(def opts {:main 'twitterbuzz.core :output-to "samples/twitterbuzz/twitterbuzz.js" :output-dir "samples/twitterbuzz/out"})
[...]
The reason we set the `:output-dir` is because the `index.html` script tag is specifically pointing to that directory.

When compiling with these options index.html tries to load deps from "[...]clojurescript/samples/twitterbuzz/samples/twitterbuzz/out/twitterbuzz/dom_helpers.js" which is not correct.

After that I just compile with

(def opts {:main 'twitterbuzz.core :output-to "samples/twitterbuzz/twitterbuzz.js" :output-dir "out"})

This places an "out" dir into the clojurescript project directory. Now, when opening index.html, twitterbuzz.js looks for dependencies like this: "[...]clojurescript/samples/twitterbuzz/out/twitterbuzz/dom_helpers.js". This correctly loads the earlier compiled js and the application works. I'm not sure how this is supposed to work as I've mostly just used lein-cljsbuild to date but you probably have an idea

Some minor things

  • The referenced patch CLJS-874 doesn't apply cleanly anymore. I'm too unfamiliar working with patches to help with this unfortunately.
  • Is there any reason to specify :main in advanced compilation mode? (It has been added to the advanced mode section in the twitterbuzz example.)

Really looking forward to see this merged!

Comment by Andrew Rosa [ 29/Nov/14 9:28 PM ]

Thanks @Martin for testing the patches, I kindly appreciate that.

I need to take some time to investigate the issue with this specific example. I'm not confident with the behavior of some examples, like the hello-js one which looks like broken (even on master). Probably I will provide a patch to review and possibly update the examples in another ticket, so we can have a more understandable environment to test this patches.

About the patch CLJS-874, it was made prior the last rework I've made here and probably will not be needed anymore. I need to confess that, if I knew at the time how to do multiple patches I neither need to created it in the first place.

At the last, the :main option isn't really used in advanced compilation. I just put it there to have some sort of "consistency" across all the examples - the only thing that change between them will be the optimization level. Do you think that this generates more confusion than good?

Comment by Mike Thompson [ 30/Nov/14 6:47 PM ]

I love this idea. Ironing out these sorts of stumbling blocks is so important to achieving wider cljs adoption. When I was starting with CLJS at the beginning of the year, I accumulated a healthy set of paper cuts from this sort of stuff. WAY too much time was wasted.

But ... two things:

1. I'd be concerned that the concatenation of base.js and deps.js might slow things down. The lovely thing about `:none` currently is that it delivers sub-second compile times. That's so important to a good productive workflow. But perhaps I'm being paranoid and adding in a base.js pre-pend won't add much time.

2. You really don't need to know what :main is. You can take a sledgehammer to the process by appending the following to the generated file:

```
for(var namespace in goog.dependencies_.nameToPath)
goog.require(namespace);
```

This requires everything in. Everything. (Which is the same outcome as using :whitespace or :simple) And that turns out to be essential in a unit-testing scenario where there isn't a single root namespace. Further explanation:

https://github.com/mike-thompson-day8/cljsbuild-none-test-seed/blob/master/test.html#L110-L111

Aside: I worry that this might not be backwards compatible and that it should be achieved by introducing a new `:optimization` target of `:debug` (and deprecating :none ??). Just a thought.

Comment by Thomas Heller [ 01/Dec/14 7:51 AM ]

@Mike Don't worry, prepending base+deps.js is not noticeable performance wise (less than a millisecond).

I tried defining what a "main" is for quite some time in shadow-build but the best I came up with: It's a namespace that contains functions called from outside your CLJS Code (eg. via HTML, aka API functions). It is a very valueable thing to have since its the only way to tell the CLJS compiler (and runtime) which namespaces are actually required. If you have a "main" namespace its deps (and the deps of the deps, ...) can be identified and namespaces that aren't actually needed can be skipped. The Closure dead-code elimination does this for :advanced as well, we just need a little help for :none.

IMHO every app should have at least one "main" ns, requiring everything is a hack not a solution.

As for your clojurescript.test example, that is a shortcoming of the tools.

I'm happy to provide help with shadow-build if anyone is interested, should be pretty simple to add a :test target. To be honest adding more features/options to cljs.closure/build (which lein-cljsbuild uses) is a recipe for disaster as it is already way too complex.

Comment by Andrew Rosa [ 01/Dec/14 10:01 AM ]

I'm also against introducing new options. I really want is eventually deprecate some of the unneeded ones.

I think that the :main namespace is clean, and could avoid issues like having some code evaluated in a namespace that is removed by Closure compiler in production. Not sure how common that could be in ClojureScript, but a valid buggy scenario in terms of global mutable state.

Anyway, if we follow the alternative of requiring everything, I think that could be done without dabbling with goog.base internals. We already have all app namespaces to be able to emit the deps.js

@Thomas your experience with shadow-build has shown you any need of "multiple entry-points" or only a single :main ns is enough? I have a feeling that the need to include all test namespaces is somewhat necessary because how clojurescript.test works, but I'm not familiar how the Clojure version works.

Comment by Thomas Heller [ 01/Dec/14 12:50 PM ]

@Andrew: See my first comment on this issue, in my experience an app will have more than one entry point. "clojurescript.test" would be one example why.

Comment by Martin Klepsch [ 01/Dec/14 3:14 PM ]

@thomas could you further explain the behaviour you'd expect from passing multiple namespaces to :main?

With my current understanding I don't see a need for multiple arguments to the :main option as Andre proposed it.
If you want a build that has another entry point (which also loads tests or similar) why not just make it a different build?

Comment by Thomas Heller [ 01/Dec/14 4:10 PM ]

It's always rather difficult to explain something without a good real world example, but explaining what I do in my app is too app specific. Anyways, my web app has a couple of different features. Lots of different pages and some use one or more features provided by CLJS code. It is not a single-page Application, rather an "old" web app with just some dynamic components sprinkled in.

Let me try an example:

(ns my-app.feature1)
(defn ^:export fancy-component [])

(ns my-app.feature2)
(defn ^:export fancy-component [])

(ns my-app.feature3)
(defn ^:export fancy-component [])

These would all be "main" namespaces by my definition.

I do not want a (ns my-app (:require [my-app [feature1 feature2 feature3]])) since that would couple all components together. One feature provided by shadow-build is that it is able to "split" the produced javascript into multiple files (modules) so you may load them optionally. So I can even split each feature into a seperate .js file loadable on demand, AFTER advanced compilation that is.

I do this in my web app and instead of feeding everyone a hefty 800kb javascript file, everyone gets 90kb and loads the rest when needed. This greatly improved page load times.

The problem with multiple builds is that EVERY build will contain cljs.core, which is quite heavy. You'd end up with

build1: cljs.core, my-app.feature1
build2: cljs.core, my-app.feature2
build3: cljs.core, my-app.feature3

instead of

module1: cljs.core (+ code shared by feature1,2,3)
module2: my-app.feature1
module3: my-app.feature2
module4: my-app.feature3

If you have experience with jQuery, multiple builds would be the equivalent to including jQuery with every jQuery plugin. But I'm getting off topic, thats about modules not about :main.

Hope I made some sense in regards to multiple mains, its just something I use alot but for me it is coupled with modules so YMMV.

Comment by Mike Thompson [ 01/Dec/14 4:15 PM ]

First, I'm a bit surprised. I thought there would be joy at the thought of NOT having to nominate a :main.

At its core, CLJS-851 appears to be about harmonizing `:none` with other options. The initial comment above explains how the HTML files for :none have to be different from the other options :simple etc. We don't like that. BUT the proposal to add :main into project.clj seems set to reintroduce a difference again between :none and the other options, but just in a different place. And there seems to be no need for that difference. :simple and :whitespace bring in all the code. So does :optimized except it is first tree-shaken down by Google Closure. So these 3 options do not need a main. Why should :none have a :main when the other options don't. I can see no downside to pulling in everything and making :none harmonious with the other.

@Andrew I agree that we don't need to use the internals of goog to require everything in. The necessary list of namespaces to require are present at the time of writing deps, so a list of goog.requires could be added, one for each namespace compiled.

Second, @Thomas could you explain more why you think cemerick's framework is wrong to be rootless? In my experience when unittesting you actively do not want a root. You want a directory full of test clj files, with that set of files growing and shrinking over time. Different members of the team might be adding and removing them at each step. You want your tests to be "discovered" by your "test runner" and then executed. I don't want to have to be manually maintaining a central list of tests. In my experience, tools like "Nose" under Python come with a "runner" which discovers all the tests by walking the directory of your project, but that approach isn't an option in the CLJS world. Instead, the compiler has to scoop up all the tests from the directory, compile them and then they have to be "required in", ONLY at that point can the tests be "discovered" in a CLJS world. At least that's my impression.

Proposal: if `:main` isn't there, then bring it all it as per my suggestion. If :main is there then only it gets required.

Comment by Thomas Heller [ 01/Dec/14 4:49 PM ]

@Mike: Without getting too much into the specifics of shadow-build: The feature of discovering tests and running them for you would be trivial, even without making a single change in shadow-build itself.

"... that approach isn't an option in the CLJS world" is simply not true. It is true for cljs.closure/build since its a function combining multiple very complex tasks into one simple map of options. lein-cljsbuild thereby suffers the same fate.

I haven't done much work documenting or promoting shadow-build and to be honest I'd much rather see tools like lein-cljsbuild built on top of shadow-build rather than trying to turn it into a lein plugin itself. Quite frankly to me it is perfect the way it is. I can step into every stage of the CLJS compilation/optimization and do stuff other tools simply cannot do, but that comes at the price of not being very beginner friendly. Documentation might help but I'm really bad at writing that stuff.

Anyways, I would inclue :main for EVERY build, ESPECIALLY :advanced!

I have a rather large codebase (way over 10k LOC CLJS) with several distinct builds. If I had to compile every CLJS file for every build everything would build forever. Some smaller builds just require a couple files (say 10), why feed 300+ into the Closure Compiler to eliminate what a simple :main could have done (and even skipped the CLJS->JS compilation).

I have this stuff in production for about 2 years now, but with custom tools for about as long. I cannot speak for the lein-cljsbuild world whether :main would be a good idea there or not. I don't even care since shadow-build is not affected by this change at all, but given my experience with my own app/codebase I would recommend making :main accept a collection of namespaces.

Comment by Mike Thompson [ 01/Dec/14 5:29 PM ]

@Thomas I'm looking at this through the cljsbuild point of view. I appreciate that shadow build might be able to do test discovery, but unfortunately I don't have the option of using it. So I'm explaining this from the perspective of a cljsbuild user, who is seeking a better outcome.

Comment by Andrew Rosa [ 01/Dec/14 7:33 PM ]

Nice to see this conversation going forward @Mike and @Thomas.

Anyone here can confirm that the advanced compilation behaves like a concatenation of all user defined namespaces? If so, we could change our strategy to mimic the behavior on :none - consistency is a gold IMO, and I like the idea of transparency in optimization levels. Thanks @Mike to point it out.

@Thomas, I guess that the way you suggest to modularize the builds is not possible even for :advanced compilations. I prefer to have a separate issue to discuss and address that feature (which looks interesting for use cases like you presented). And sorry to make you answer the same question again.

The conditional :main is worth the cost of added complexity? How it will affect :advanced compilation?

Comment by Thomas Heller [ 02/Dec/14 4:39 AM ]

Advanced Optimization does not behave like a concatenation, Closure handles the optimization like this (greatly simplified):

1) You provide a list of inputs (Javascript files) and externs
2) Closure parses these into AST inputs
3) basic analysis to extract goog.provide/require information
4) build dependency graph, topological sort
5) magic (if :advanced)
6) generate output

shadow-build does something similar

1) scan all source paths for "resources" (.js, .cljs)
2) extract goog.provide/require from .js
3) read first form of .cljs files (expects it to be 'ns), get provide/require from that
4) build dependency graph, topological sort
5) compile CLJS->JS (if reachable by any :main)
6) feed into Closure Compiler
7) output

Basically cljs.clojure/build does the same, except for #3 which results in CLJS compiling in a somewhat random order (eg. it compiles one 'ns and all its deps until everything is compiled). Without a :main everything will be compiled even if not used anywhere in your project. Closure Advanced Compilation can still remove dead-code since it will analyze the AST and realize that parts (or everything) of a given namespace are never used, :none can not do that without a :main.

If you have 300 source files (CLJS+JS) the load time of a web page (in dev) will be rather high (we are talking seconds here, remember worrying about a couple ms), especially with open dev tools since that will also load an additional 300 source maps. Loading files you don't need therefore is a huge waste of time especially if you reload (or even phantomjs clojurescript.test) often. I cannot stress enough how important :main is for this stuff, but it largely depends on the size of your codebase and the structure of your app. Small example: the Admin "App" of my application will do 714 requests on page load (JS+source maps) and transfer 3.3mb, page load is 3,54s on my rather beefy MacPro (in production this cuts down 13 requests, 343kb and page load in 700ms). Without the :main every page in my app would do 714 requests (or even more) on page load. Not a great development experience. Again I'm using my own tooling but do things like lein-figwheel not trigger full page reloads? REPL might provide a different experience but I found it impossible to properly build/test complex input forms when using a REPL, much rather edit/reload.

@Andrew: Agreed that the module discussion has no place here, just wanted to point out that the :main option becomes mandatory then. To be honest this whole discussion only stresses the problem of cljs.closure/build: trying to combine everyone's expectation of how a build should look into a single function is tough. So in the long run something that lets you split these parts will be easier (hint: shadow-build) but the good part is that ClojureScript itself does not even need to deal with all this stuff. A "simple" CLJS->JS compiler would do, the Closure bits are optional after all (even more so for non-browser targeted builds). But discussing what should be in CLJS and what shouldn't is another issue as well.

Comment by Martin Klepsch [ 02/Dec/14 1:21 PM ]

Again I'm using my own tooling but do things like lein-figwheel not trigger full page reloads?

No. Figwheel only reloads changed files. So the slowdown should be a lot smaller once the page is loaded.

Besides that: @thomas thanks for all your very thoughtful messages here, just reading them already taught me a ton about Clojurescript compilation!

Comment by Andrew Rosa [ 02/Dec/14 6:22 PM ]

Thank you @Thomas for all this explanation, quite insightful. You convinced me that the way to go is requiring :main anyway. If we fell the necessity, we could add the "require all" behavior later. I also like the idea of having a dependency graph to compile only required stuff, but is something for another discussion

Just a final tweak for us to think about: the :main option could receive a list of namespaces or just a single one? Conceptually, I biased for the later. A single namespace could allow us to eventually add something like a -main function, useful for node apps for example - just an idea, of course.

I just updated the patches to fix a single issue with :main not being mungled before require, which caused problems with namespaces including dashes (like foo-bar)

Comment by Mike Thompson [ 02/Dec/14 6:35 PM ]

Okay, my final observations:

  • naming
    Given :main is to be used to indicate a namespace, should its name be changed to make that clearer?
    After all this is about automating: 'goog.require(something)' but the name :main seems unrelated.
    To me :main carries connotations of main functions, not root namespaces.
    I would suggest that ":auto-require" would be a better name and I believe names (cumulatively) matter.
  • Special Marker
    No matter the name chosen, to handle my unittest use-case, would it be possible to have a
    special marker value for :main like, say, "*" which triggers the require everything approach.
    As I've indicated, that would be a big win from where I'm standing.

Thanks to everyone.

Comment by Andrew Rosa [ 02/Dec/14 6:41 PM ]

Good call @Mike. I guess :main was chosen to provide some "parity" with Clojure - that's why I suggest the auto-calling -main feature.

For the unit test case, do you think that it could be done as one enhancement on clojurescript.test? It seems like the test runner should be capable of finding the right namespaces... not sure if it does not do that for limitations of the platform.

Comment by Thomas Heller [ 02/Dec/14 7:16 PM ]

Closure calls them Entry Points, I got used to :main.

Can't really say much about clojurescript.test, only that testing generally is rather complex hiding this behind a "require all" marker will probably not be enough. I know I don't always want to run every single test, usually only those affected by the file I just changed is enough. But getting off-topic again, proper Tooling is hard.

Again for :main, my advice would be a collection but just offer a fallback and turn a single symbol into a collection of one. That covers all cases 0,1,more ... No :main just does not emit any requires which is what we have now.

:main my-app.feature1
:main [my-app.feature1]
:main [my-app.feature1 my-app.feature2]
Comment by Andrew Rosa [ 02/Dec/14 7:27 PM ]

@Thomas no defined :main will still will include base.js and deps.js, but not requiring anything. Right? At least is that what I would expect.

Yeah, tooling is very hard.

Comment by Thomas Heller [ 03/Dec/14 4:20 AM ]

No :main might as well behave like it does now, this way the feature is completely optional and will not break any existing code. Thats usually worth something, don't know how much because as I said, :main is mandatory in shadow-build.

ClojureScript itself should probably be more conservative than that. I imagine there are alot of projects that still use /out/goog/base.js + other in their HTML, that won't change over night.

Comment by David Nolen [ 24/Jan/15 12:25 PM ]

The approaches taken thus far are unnecessarily complex. All that needs to happen is for the generated :none script to document.write the needed scripts and goog.require the :main namespace. I will take a patch that does this.

Comment by Andrew Rosa [ 24/Jan/15 2:24 PM ]

Thanks @dnolen for the feedback. I will work on this patch.

Comment by David Nolen [ 24/Jan/15 2:43 PM ]

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

Comment by David Nolen [ 24/Jan/15 2:44 PM ]

Andrew, I went ahead and tackled this one.





[CLJS-874] Add closure/path-relative-to support to relative paths based on directories Created: 19/Oct/14  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Andrew Rosa Assignee: Andrew Rosa
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 874.patch    
Patch: Code and Test

 Description   

The referred function could not handle directories as base paths, since it only calculate the relative paths via string manipulation. I propose using some checking on file system (through {File#isDirectory()}) to be able to differentiate directories from files.

The current behavior:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "core.js"

After the patch we will have:

(path-relative-to (io/file ".") {:url (deps/to-url (io/file "cljs/core.js"))})
;; => "cljs/core.js"

This behavior is needed for my patch for CLJS-851. If there is some better alternative to address that, I'm open to make the appropriate changes.



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

Can we get a rebased patch? Thanks.

Comment by Andrew Rosa [ 02/Dec/14 5:24 AM ]

Of course I can provide it @David. But the current CLJS-851 implementation this behavior is not needed anymore. Do you still want this patch or prefer to wait until CLJS-851 resolution?

Comment by David Nolen [ 24/Jan/15 12:20 PM ]

No longer relevant





[CLJS-983] Make ExceptionInfo printable Created: 20/Jan/15  Updated: 24/Jan/15  Resolved: 24/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-983-printable-ex-info-2.patch    

 Description   

Pretty simple enhancement — so we can see what's inside



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

This patch needs a rebased to master.

Comment by Nikita Prokopov [ 24/Jan/15 2:56 AM ]

Things got a little messier after CLJS-985 and prototype manipulations

Comment by Nikita Prokopov [ 24/Jan/15 2:57 AM ]

Uploaded updated patch, rebased to master

Comment by David Nolen [ 24/Jan/15 10:29 AM ]

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





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

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

Type: Defect Priority: Major
Reporter: Bobby Eickhoff Assignee: Nicola Mometto
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-966] :foreign-libs should not go through Closure Compiler Created: 07/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/15

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

Currently the files listed in foreign libs will go through Closure Compiler when using :advanced, they should instead be in the preamble.

https://github.com/thheller/cljs-foreign-bug

lein cljsbuild once release

will create a foreign.min.js [1] which includes the advanced optimized source of js/sample.js (grep for I WONT SURVIVE). In this case the source actually keeps working since the source is quite simple. But we don't even need to include the js/sample.js in the HTML file since the source won't access Foreign.sayHi since it got renamed and uses the optimized code.

[1] https://github.com/thheller/cljs-foreign-bug/blob/master/foreign.min.js



 Comments   
Comment by David Nolen [ 23/Jan/15 6:21 PM ]

fixed in master





[CLJS-985] ex-info loses stack information Created: 20/Jan/15  Updated: 23/Jan/15  Resolved: 23/Jan/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: Completed Votes: 1
Labels: None

Attachments: Text File cljs-985-ex-info-stack.patch    

 Description   

Native js Error keeps stacktrace:

(js/console.log (.-stack (js/Error. "message")))

Error: message
    at eval (/Users/prokopov/Dropbox/ws/datascript/test/test/datascript.cljs[eval5]:10:14)
    at eval (native)
    at SocketNamespace.<anonymous> (http://localhost:65000/socket.io/lighttable/ws.js:118:26)
    at SocketNamespace.EventEmitter.emit [as $emit] (http://localhost:65000/socket.io/socket.io.js:633:15)
    at SocketNamespace.onPacket (http://localhost:65000/socket.io/socket.io.js:2248:20)
    at Socket.onPacket (http://localhost:65000/socket.io/socket.io.js:1930:30)
    at Transport.onPacket (http://localhost:65000/socket.io/socket.io.js:1332:17)
    at Transport.onData (http://localhost:65000/socket.io/socket.io.js:1303:16)
    at WebSocket.websocket.onmessage (http://localhost:65000/socket.io/socket.io.js:2378:12)

But ex-info does not:

(js/console.log (.-stack (ex-info "message")))

Error
    at file:///Users/prokopov/Dropbox/ws/datascript/web/target-cljs/cljs/core.js:32066:38

Problem is that ex-info inherits stack property from prototype which is instantiated at script load time here:

(deftype ExceptionInfo [message data cause])

(set! (.-prototype ExceptionInfo) (js/Error.))
(set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)

The possible solution is to create new instance of js/Error at (ex-info) and manually copy stack property to ExceptionInfo object. Related SO: http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript

Problem is that Chrome has setter on stack property, and it only allows for this property to be set inside a constructor functions.

Proposed fix creates new Error each time ex-info is called and sets ExceptionInfo.prototype to newly created error. This way new ExceptionInfo instance will inherit stack from newly created Error with correct stack.

This patch has been tested in Chrome 39 Mac, Safari 8 Mac, Firefox 35 Mac and IE 10 Win. Here's test code I used:

(defn -ex-info
  ([msg data]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data nil))
  ([msg data cause]
    (set! (.-prototype ExceptionInfo) (js/Error msg))
    (set! (.. ExceptionInfo -prototype -name) "ExceptionInfo")
    (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
    (ExceptionInfo. msg data cause)))

(try
  (throw (ex-info "[ -- Current ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Current ex-info::" (.-stack e))))

(try
  (throw (js/Error "[ -- Native message -- ]"))
  (catch js/Error e
    (js/console.log "Native error::" (.-stack e))))

(try
  (throw (-ex-info "[ -- Patched ex-info message -- ]" 123))
  (catch ExceptionInfo e
    (js/console.log "Patched ex-info::" (.-stack e))))

Test results:

Chrome, Firefox, IE, Safari

Note that current implementation reports line number and overall stacktrace from cljs.core file where Error prototype is created in current implementation.
Note that patched version reports correct line number (it should be close to native error stack), stack, message and exception name.
Also note that IE is fine even without patch — that's because in IE stack is capturead at throw place, not at new Error() call site.



 Comments   
Comment by Nikita Prokopov [ 20/Jan/15 2:48 PM ]

Ok, this is crazy, but this seems to solve the issue:

(defn ex-info [msg data cause]
  (set! (.-prototype ExceptionInfo) (js/Error msg))
  (set! (.. ExceptionInfo -prototype -name) "cljs.core.ExceptionInfo")
  (set! (.. ExceptionInfo -prototype -constructor) ExceptionInfo)
  (ExceptionInfo. msg data cause))

Basically we change prototype before creating each object.

(taken from http://stackoverflow.com/questions/783818/how-do-i-create-a-custom-error-in-javascript#answer-12030032)

I guess high performance is not needed from ex-info, so this solution is somewhat okay-ish? Should I make a patch from it?

Comment by David Nolen [ 20/Jan/15 2:55 PM ]

It would be nice to get confirmation from others that this works under the major browser - Safari, Firefox, Chrome, and modern IE.

Comment by Nikita Prokopov [ 20/Jan/15 3:12 PM ]

I can confirm Firefox 34, Firefox 35, Safari 8.0.2 and Chrome 39 (all Mac) for now

Comment by Nikita Prokopov [ 22/Jan/15 3:50 AM ]

David, I updated issue, added patch, test code and test results (including IE). There’s no unit test on this because stack traces are very engine-specific. Please take a look

Comment by David Nolen [ 22/Jan/15 2:24 PM ]

Nikita, thanks for the update will check it out.

Comment by David Nolen [ 23/Jan/15 6:13 PM ]

fixed https://github.com/clojure/clojurescript/commit/93dce672e1af8f698cfc2a61e293cb48aeeddc2c





[CLJS-986] Add :target to the list of build options that should trigger recompilation Created: 21/Jan/15  Updated: 21/Jan/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





[CLJS-27] Conditional compilation (or reading) Created: 22/Jul/11  Updated: 21/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: None

Attachments: File cljs-27.diff     File cljs-27-v2.diff     File cljs-27-v3.diff     File cljs-27-v4.diff     File cljs-27-v5.diff     File cljs-27-v6.diff     File conditional-compilation-clojure.diff     File conditional-compilation-clojurescript.diff    
Patch: Code and Test
Approval: Vetted

 Description   

As people start trying to write libs that are portable between Clojure and ClojureScript they might need to have a bit of branching on target. N.B. supporting this means a change to Clojure, although it has general utility there as well.

Consider CL #+ #- reader macros - http://www.lispworks.com/documentation/lw50/CLHS/Body/02_dhq.htm

Patch: cljs-27-v6.diff

Related: CLJ-1424, TRDR-14



 Comments   
Comment by Roman Scherer [ 19/Jul/12 8:52 AM ]

The following patches include an implementation of Common Lisp's #+
and #- reader macros to allow conditional compilation/reading for
Clojure and ClojureScript.

The patches add a dynamic variable called features to the
clojure.core and cljs.core namespaces, that should contain the
supported features of the platform in question as keywords.

Unlike in Common Lisp, the variable is a Clojure set and not a list.
In Clojure the set contains at the moment the :clojure keyword, and in
ClojureScript the :clojurescript keyword.

I would like to get feedback on the names that are added to this
variable. Are those ok? Is :jvm for Clojure and :js for ClojureScript
better? Should ClojureScript add something like :rhino, :v8 or
:browser as well?

To run the ClojureScript tests, drop a JAR named "clojure.jar" that
has the Clojure patch applied into ClojureScript's lib directory.

Comment by David Nolen [ 19/Jul/12 12:18 PM ]

This is an enhancement so it probably requires a design page and extended discussion before it will go anywhere. Until that happens I'm marking this is as low priority.

Comment by Roman Scherer [ 19/Jul/12 1:45 PM ]

Ok. If someone could give me write access to the Clojure Dev Wiki I would be happy to start such a design page.

Comment by David Nolen [ 19/Jul/12 5:50 PM ]

If you've sent in your CA request permissions on the clojure-dev mailing list.

Comment by Roman Scherer [ 21/Jul/12 5:45 AM ]

I started a design page for this ticket in the Clojure Dev wiki:
http://dev.clojure.org/display/design/Feature+Expressions

Comment by Stuart Halloway [ 27/Jul/12 1:48 PM ]

Posting my comments over on the design page...

Comment by Alex Miller [ 06/Aug/14 7:42 AM ]

Latest patch updates into current ClojureScript and use of tools.reader/tools.analyzer etc. The reader changes are all in the accompanying tools.reader patch in TRDR-14. This patch adds support to allow a new option "features" which is expected to be a set of keywords. build will inject :cljs into this set. The feature set is maintained in clojure.tools.reader/*features*. set! is enhanced to special-case an update to *features* in the code (presumably a rarely-used feature).

Because tools.reader needs the supporting patch, I left in several changes that pull in a new version of tools.reader - I don't actually expect those to be the correct versions but they are there as a reminder to update all of the proper places.

Comment by Alex Miller [ 11/Sep/14 9:04 AM ]

cljs-27-v2.diff adds ability to load .clj files as well as .cljs files when compiling.

Comment by Alex Miller [ 07/Nov/14 10:55 AM ]

Fresh patch, switch to take .cljc files.

Comment by Alex Miller [ 06/Jan/15 3:04 PM ]

Freshened patch for current CLJS.

Comment by David Nolen [ 06/Jan/15 9:55 PM ]

Alex the freshened patch includes modifications to the compiler version dynamic var and compiler version fn. Can we remove these? Thanks!

Comment by Alex Miller [ 07/Jan/15 12:28 PM ]

Yep, updated to -v5 patch.

Comment by Alex Miller [ 21/Jan/15 4:36 PM ]

Updated to use new tools.reader patch and to remove the ability to dynamically set the features set. The active set of features can be set on startup and are held in the features var in the analyzer.





[CLJS-984] Update Node.js REPL support to use public API Created: 20/Jan/15  Updated: 20/Jan/15  Resolved: 20/Jan/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   

https://github.com/google/closure-library/commit/d909e5aa4b71923d72c15305f70d01a976c9947f



 Comments   
Comment by David Nolen [ 20/Jan/15 2:56 PM ]

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





[CLJS-890] Incorrect behaviour of (str obj) when obj has valueOf method Created: 24/Nov/14  Updated: 20/Jan/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.





[CLJS-847] TypeError in cljs.core/str using Safari 6.0.5 Created: 29/Aug/14  Updated: 19/Jan/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.





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

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

Type: Enhancement Priority: Major
Reporter: Stephan Behnke Assignee: David Nolen
Resolution: Unresolved 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
 :output-modules {
  {:id :common
   :out "./resources/assets/js/common.js"  
   :entries '#{com.foo.common}}
  {:id :landing
   :out "./resources/assets/js/landing.js" 
   :entries '#{com.foo.landing}
   :deps #{:common}
  {:id :editor
   :out "./resources/assets/js/editor.js"
   :entries '#{com.foo.editor}
   :deps #{: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.





[CLJS-982] Var derefing should respect Clojure semantics Created: 18/Jan/15  Updated: 18/Jan/15  Resolved: 18/Jan/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   

In every compilation mode other than advanced possible to preserve via a thunk



 Comments   
Comment by David Nolen [ 18/Jan/15 10:00 PM ]

fixed https://github.com/clojure/clojurescript/commit/522fbdc04e5e35171e6818ce80b3db4811cc3284





[CLJS-981] Better benchmarking infrastructure Created: 17/Jan/15  Updated: 17/Jan/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   

We should use ProcessBuilder to run the various benchmark scripts and control which benchmarks we test and which engines we run. Benchmarks should produce EDN data that can be written to a file, loaded into Incanter, etc.






[CLJS-980] ClojureScript REPL stacktraces overrun prompt in many cases Created: 17/Jan/15  Updated: 17/Jan/15  Resolved: 17/Jan/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:

ClojureScript 2687


Attachments: Text File CLJS-980.patch    

 Description   

ClojureScript repl stacktraces overrun prompt in many cases

If you do
(doc 'clojure.string/join)
The failure stacktrace hides the prompt, creating the illusion the REPL has hung.



 Comments   
Comment by Bruce Hauman [ 17/Jan/15 11:10 AM ]

Provided patch that fixes this.

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

fixed https://github.com/clojure/clojurescript/commit/359ea4eec7aad5902abbaeee319df44d78183469





[CLJS-979] ClojureScript REPL needs error handling for the special functions Created: 16/Jan/15  Updated: 16/Jan/15  Resolved: 16/Jan/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:

ClojureScript 2688


Attachments: Text File CLJS-979.patch    

 Description   

Currently the the special functions are not run int a try catch block. This creates a brittle repl that will fail is someone trys to require a namespace that doesn't exist or execute any of the special functions with error causing arguments.

The following fail and terminate the repl loop.
(require 'ex) where ex doesn't exist
(in-ns '5) causes an infinite loop.



 Comments   
Comment by Bruce Hauman [ 16/Jan/15 4:32 PM ]

Here is a patch that fixes this. A general handler ensures that injected special-fns are handled as well.

Comment by David Nolen [ 16/Jan/15 5:22 PM ]

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





[CLJS-978] Analysis caching doesn't account for constants table Created: 15/Jan/15  Updated: 16/Jan/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   

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.





[CLJS-977] Subvec missing IKVReduce implementation Created: 15/Jan/15  Updated: 15/Jan/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





[CLJS-973] cljs.test ignores some deftest names Created: 12/Jan/15  Updated: 15/Jan/15  Resolved: 15/Jan/15

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

Type: Defect Priority: Minor
Reporter: Denis Johnson Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: cljs, test
Environment:

Linux Ubuntu 14.04
Java 1.7.0_17 Java HotSpot(TM) 64-Bit Server VM
leiningen 2.5.1

:dependencies
[[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2665"]
[reagent "0.5.0-alpha"]
[alandipert/storage-atom "1.2.3"]
[com.andrewmcveigh/cljs-time "0.3.0"]]
:plugins [[lein-cljsbuild "1.0.3"]]



 Description   

examples:

  • ignores: (deftest test:change-step-contiguous-forward> ....)
  • works: (deftest test:change-current-step ...) ignores: (deftest test-change-current-step ...)


 Comments   
Comment by David Nolen [ 15/Jan/15 1:26 AM ]
(deftest test:change-current-step: ...)
(deftest test:change-current-step@ ...)

Removed these two from ticket description these are not valid Clojure symbols see http://clojure.org/reader

Comment by David Nolen [ 15/Jan/15 1:37 AM ]

I tested the remaining names could not reproduce. Please only reopen this ticket if a full minimal case is provided.





[CLJS-934] In the REPL return vars after defs Created: 30/Dec/14  Updated: 14/Jan/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   

We don't want to emit these in during normal compilation. However it's nice to unify the REPL experience with Clojure's. Currently we just display the value of the def. REPLs could set a :repl build flag which is checked by the emit* :def case. For this to work the analyzer should compile the var AST and include it in the def AST so the compiler can optionally use it.



 Comments   
Comment by Mike Fikes [ 14/Jan/15 4:03 PM ]

A change was recently made to introduce new behavior if a :repl-verbose flag is set.

Perhaps all flags that control REPL behavior could be prefixed with :repl-, with the flag controlling this ticket controlled by :repl-def-vars or somesuch.





[CLJS-976] Node REPL breaks from uncaught exceptions Created: 14/Jan/15  Updated: 14/Jan/15

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

Type: Defect Priority: Major
Reporter: Joel Holdbrooks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Certain errors raised by Node will cause a ClojureScript Node REPL to fail.

Steps to reproduce:

cljs.user> (require '[cljs.nodejs :as node])
cljs.user> (def fs (node/require "fs"))
cljs.user> (.createReadStream fs "fail")
;; => #<[object Object]>
cljs.user> (+ 1 1)
;; => java.lang.IllegalArgumentException: Value out of range for char: -1

A vanilla Node REPL will also fail under the same condition and terminate the session.

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: ENOENT, open 'fail'

The CLJS REPL should probably be a bit more durable and, instead of terminating, catch and report the error.

A gist of the above example can be found here: https://gist.github.com/noprompt/3d0e5cbfca7883727fcf.






[CLJS-975] preserve :reload & :reload-all in ns macro sugar Created: 13/Jan/15  Updated: 13/Jan/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   

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





[CLJS-971] at REPL :reload should work for require-macros special fn Created: 12/Jan/15  Updated: 13/Jan/15  Resolved: 13/Jan/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 [ 13/Jan/15 8:03 AM ]

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





[CLJS-974] Clean compile/build fails to resolve namespaces for cljs.test Created: 12/Jan/15  Updated: 13/Jan/15  Resolved: 13/Jan/15

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

Type: Defect Priority: Major
Reporter: Denis Johnson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cljs, test
Environment:

Linux Ubuntu 14.04
Java 1.7.0_17 Java HotSpot(TM) 64-Bit Server VM
leiningen 2.5.1
Intellij 13.1.6 with latest cursive 0.1.43 plugin

:dependencies
[[org.clojure/clojure "1.6.0"]
[org.clojure/clojurescript "0.0-2665"]
[reagent "0.5.0-alpha"]
[alandipert/storage-atom "1.2.3"]
[com.andrewmcveigh/cljs-time "0.3.0"]]
:plugins [[lein-cljsbuild "1.0.3"]]


Attachments: File test_suite.cljs    

 Description   

Clean compile gives the following error for a valid namespace for runner/suite (see attachment).
Same namespace works fine using cemerick/clojurescript.test
NOTE 1: if compiled using "cljsbuild auto test" after giving failure, if you delete the test/test-suite.cljs the compile completes successfully, if then you revert the file it also recompiles successfully and tests run as expected.
NOTE 2: Happens all the time on my machine but other devs on Windows platform don't seem to get this issue. The only difference I can see is I use leiningen checkouts feature to point at our other local repos and these are project dependencies. I have tested with removing the checkouts folder with change in behaviour so I doubt it is related.

Deleting files generated by lein-cljsbuild.
Compiling ClojureScript.
Compiling "compiled/test.js" from ["src" "test"]...
Compiling "compiled/test.js" failed.
clojure.lang.ExceptionInfo: failed compiling file:test/test_suite.cljs
core.clj:4403 clojure.core/ex-info
compiler.clj:1039 cljs.compiler/compile-file
compiler.clj:1069 cljs.compiler/compile-root
closure.clj:358 cljs.closure/compile-dir
closure.clj:398 cljs.closure/eval3160[fn]
closure.clj:306 cljs.closure/eval3096[fn]
closure.clj:412 cljs.closure/eval3147[fn]
closure.clj:306 cljs.closure/eval3096[fn]
compiler.clj:44 cljsbuild.compiler.SourcePaths/fn
core.clj:2557 clojure.core/map[fn]
LazySeq.java:40 clojure.lang.LazySeq.sval
LazySeq.java:49 clojure.lang.LazySeq.seq
RT.java:484 clojure.lang.RT.seq
core.clj:133 clojure.core/seq
core.clj:624 clojure.core/apply
core.clj:2586 clojure.core/mapcat
RestFn.java:423 clojure.lang.RestFn.invoke
compiler.clj:44 cljsbuild.compiler/cljsbuild.compiler.SourcePaths
closure.clj:1018 cljs.closure/build
closure.clj:972 cljs.closure/build
compiler.clj:58 cljsbuild.compiler/compile-cljs[fn]
compiler.clj:57 cljsbuild.compiler/compile-cljs
compiler.clj:159 cljsbuild.compiler/run-compiler
form-init1706317005734214457.clj:1 user/eval3512[fn]
form-init1706317005734214457.clj:1 user/eval3512[fn]
LazySeq.java:40 clojure.lang.LazySeq.sval
LazySeq.java:49 clojure.lang.LazySeq.seq
RT.java:484 clojure.lang.RT.seq
core.clj:133 clojure.core/seq
core.clj:2855 clojure.core/dorun
core.clj:2871 clojure.core/doall
form-init1706317005734214457.clj:1 user/eval3512
Compiler.java:6703 clojure.lang.Compiler.eval
Compiler.java:6693 clojure.lang.Compiler.eval
Compiler.java:7130 clojure.lang.Compiler.load
Compiler.java:7086 clojure.lang.Compiler.loadFile
main.clj:274 clojure.main/load-script
main.clj:279 clojure.main/init-opt
main.clj:307 clojure.main/initialize
main.clj:342 clojure.main/null-opt
main.clj:420 clojure.main/main
RestFn.java:421 clojure.lang.RestFn.invoke
Var.java:383 clojure.lang.Var.invoke
AFn.java:156 clojure.lang.AFn.applyToHelper
Var.java:700 clojure.lang.Var.applyTo
main.java:37 clojure.main.main
Caused by: clojure.lang.ExceptionInfo: No such namespace: test.model.dimension-glossary-test at line 1 test/test_suite.cljs
core.clj:4403 clojure.core/ex-info
analyzer.clj:297 cljs.analyzer/error
analyzer.clj:294 cljs.analyzer/error
analyzer.clj:1095 cljs.analyzer/analyze-deps
analyzer.clj:1280 cljs.analyzer/eval1561[fn]
MultiFn.java:249 clojure.lang.MultiFn.invoke
analyzer.clj:1609 cljs.analyzer/analyze-seq
analyzer.clj:1696 cljs.analyzer/analyze[fn]
analyzer.clj:1689 cljs.analyzer/analyze
compiler.clj:948 cljs.compiler/compile-file*[fn]
compiler.clj:906 cljs.compiler/with-core-cljs
compiler.clj:927 cljs.compiler/compile-file*
compiler.clj:1033 cljs.compiler/compile-file



 Comments   
Comment by David Nolen [ 13/Jan/15 7:31 AM ]

Please report cljsbuild bugs with the cljsbuild project thanks.





[CLJS-972] Node.js REPL eats errors in required ns when using require Created: 12/Jan/15  Updated: 12/Jan/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





[CLJS-970] assert - generate Error string when compiling Created: 10/Jan/15  Updated: 12/Jan/15

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

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

Attachments: File cljs-generate-assert-message.diff    
Patch: Code

 Description   

Currently when using assert in CLJS the assert macro will generate the assert error message through CLJS. We can do that in CLJ which reduces the amount of code generated.

(let [bar 1]
  (assert (string? bar)))

Currently generates

if(typeof bar_11332 === 'string'){
} else {
throw (new Error([cljs.core.str("Assert failed: "),cljs.core.str(cljs.core.pr_str.call(null,cljs.core.list(new cljs.core.Symbol(null,"string?","string?",-1129175764,null),new cljs.core.Symbol(null,"bar","bar",254284943,null))))].join('')));
}

But could generate

if(typeof bar_23183 === 'string'){
} else {
throw (new Error("Assert failed: (string? bar)"));
}

Attached patch does that.



 Comments   
Comment by Thomas Heller [ 12/Jan/15 9:16 AM ]

Forgot to account for the fact that

(assert (string? something) "this might not be a string"))

New patch only generates the string of the form when compiling.





[CLJS-101] Node fails with :whitespace, works with :simple Created: 17/Nov/11  Updated: 10/Jan/15  Resolved: 19/Apr/14

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

Type: Defect Priority: Minor
Reporter: Ulrik Sandberg Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

NodeJS 0.6.1
MacOSX Lion



 Description   

Running the example from http://mmcgrana.github.com/2011/09/clojurescript-nodejs.html. Node fails if using :whitespace optimizations, but works when using :simple. The problem seems to be missing object literals.

The source is:

$ cat src/testing-cljs-node/hello.cljs 
(ns testing-cljs-node.hello)

(defn start [& _]
  (println "Hello World!"))

(set! *main-cli-fn* start)

Here is the compilation that results in the broken code:

$ cljsc src/testing-cljs-node/hello.cljs '{:optimizations :whitespace :pretty-print true :target :nodejs}' > out/hello.js
$ node out/hello.js

node.js:201
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
TypeError: Cannot set property 'Unicode' of undefined
    at Object.<anonymous> (/Users/ulrik/Source/testing-cljs-node/out/hello.js:487:21)
    at Module._compile (module.js:432:26)
    at Object..js (module.js:450:10)
    at Module.load (module.js:351:31)
    at Function._load (module.js:310:12)
    at Array.0 (module.js:470:10)
    at EventEmitter._tickCallback (node.js:192:40)

When changing the optimizations to :simple, however, it works:

$ cljsc src/testing-cljs-node/hello.cljs '{:optimizations :simple :pretty-print true :target :nodejs}' > out/hello.js
$ node out/hello.js
The "sys" module is now called "util". It should have a similar interface.
Hello World!

The offending line 487 in the :whitespace code:

goog.provide("goog.string");
goog.provide("goog.string.Unicode");
goog.string.Unicode = {NBSP:"\u00a0"};  // line 487

In the working :simple code, the corresponding lines looks like this:

goog.string = {};
goog.string.Unicode = {NBSP:"\u00a0"};  // line 367

If I fix that by adding a line that creates the object, I get another error on line 901:

goog.provide("goog.userAgent.jscript");
goog.require("goog.string");
goog.userAgent.jscript.ASSUME_NO_JSCRIPT = false;  // line 901

In the working code, it looks like this:

goog.userAgent = {};
goog.userAgent.jscript = {};
goog.userAgent.jscript.ASSUME_NO_JSCRIPT = !1;  // line 683

And if I fix that, I get another error on line 975:

goog.provide("goog.debug.Error");
goog.debug.Error = function(opt_msg) {  // line 975

The working code:

goog.debug = {};
goog.debug.Error = function(a) {  // line 730

etc etc



 Comments   
Comment by David Nolen [ 19/Apr/12 11:49 PM ]

I'm not sure how to fix this since the implementation of goog.provide has no meaning in Node.js. As a compromise perhaps we should complain if you try to target node with :whitespace optimizations?

Comment by Ulrik Sandberg [ 20/Apr/12 5:50 AM ]

Sure, that would probably be sufficient to point the user to what the problem really is.

Perhaps it's obvious to everyone else that :whitespace doesn't make sense for Node, but it wasn't to me. But I can now deduce that :simple maybe resolves all provide/require into a single file, whereas :whitespace is an intermediate format that is not applicable for Node.

Comment by David Nolen [ 20/Apr/12 11:29 AM ]

I don't think it's obvious at all and many people have encountered this issue.

Comment by Tom Jack [ 16/Aug/12 2:54 AM ]

Something that seems to work is

#!/usr/bin/nodejs
var _cljsGlobal_ = {};
with (_cljsGlobal_) {
var COMPILED = false;
_cljsGlobal_.goog = {};
goog.global = _cljsGlobal_;
// ... rest of source
}

Does this cause any problems other than forcing e.g. ;module.exports = _cljsGlobal_.mori; in whitespace mode?

Comment by David Nolen [ 16/Aug/12 8:30 AM ]

Using with is probably not an acceptable solution.

Comment by David Nolen [ 28/Sep/12 10:08 PM ]

nclosure actually sounds kind of promising! Do we need to do anything more then recommend that people install nclosure if they want to work :whitespace or :none for the optimization level? If the nclosure approach is simple enough we could perhaps duplicate the strategy?

Comment by Paul deGrandis [ 29/Sep/12 5:27 PM ]

Haha I deleted the comment after I started digging into the code. I'll try to capture my adventure here.

CLJS's node compat layer is built in the opposite direction as nclojure - that is, we provide an externs file for node, and assume interop will happen that way.
nclosure's approach is to hijack the base interactions with goog, and decide to dispatch accordingly to Node or to the Closure lib, but it expects you're writing node-based code. Everything is pushed into shared object off of the node `globals`.

My hypothesis was: I can use the nclosure stuff, but allow ALL dispatching to go to closure (since we have externs file), which very well may call node stuff in the end. The small shim would allow for skirting around the issues described here.

I hacked together the js files and copied the result in to my CLJS-compiled-JS file. All looked good except - the nclosure JS code needs to be inserted into the file before compilation happens. The nclosure stuff redefines core goog interactions, which is why it works. Technically, we could do a preamble hook when we're gathering all the the files together (much like how main-cli-fn is at the tail of all files).

That said, when inserted into the right spot, it prevents the errors.

Comment by David Nolen [ 19/Apr/14 10:14 AM ]

fixed https://github.com/clojure/clojurescript/commit/0c7b31ada01237de33cef77b817ccef3f2b3576d

Comment by Quiz Dr [ 04/May/14 3:43 AM ]

FWIW glad I found this page; I am getting the exact same "type error" error on this line:

goog.string.Unicode = {NBSP:"\u00a0"};

When I use the "whitespace" optimization and run my compiled JS inside Qt 5.2 which now offers Javascript as a native language for building device GUIs (it's pretty neat, and using ClojureScript for this is even neater if I can get it to work).

The type error went away when I switched to "simple" optimization. I am using CS 2202 with cljsbuild 1.0.3.

Comment by Stephan Behnke [ 10/Jan/15 5:05 AM ]

I'm also seeing the issue Quiz Dr describes.

goog.string.Unicode = {NBSP:"\u00a0"};
TypeError: Cannot set property 'Unicode' of undefined

The other issues on this tracker suggest that this has been fixed. But it still happens with version "0.0-2665".
Is there a workaround I haven't seen? Compiling with :simple is noticeably slower than :whitespace ...

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

I saw this problem while working on shadow-build and tracked it down to a faulty google closure library release (which was fixed in CLJS-826). The issue was that a basically empty goog/base.js was overriding the default goog/base.js and causing all sorts of weird issues.

Can you confirm that the correct goog/base.js is used? Either by looking at the build output or

lein repl

(require '[clojure.java.io :as io])
(println (slurp (io/resource "goog/base.js")))

If thats just a comment something is wrong. Should be correct with 0.0-2665 though.





[CLJS-955] Move incorrect *analyze-deps* check Created: 03/Jan/15  Updated: 09/Jan/15  Resolved: 09/Jan/15

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

Type: Defect Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File move-incorrect-check.diff    
Patch: Code

 Description   

A new analyze-deps check was added recently [1] which is at the wrong location. When running with analzye-deps false (which shadow-build always does) it will also disable all macro loading. The attached patch moves this check to the "correct" position.

[1] https://github.com/clojure/clojurescript/commit/08805fd519a46ca80aa95b9a50af8c8293df0d3f



 Comments   
Comment by Thomas Heller [ 03/Jan/15 6:20 AM ]

Wait ... this new behavior completely disables all warnings for shadow-build since it runs with analyze-deps false.

I don't think this flag should affect the warnings at all. Either a new flag should be introduced to suppress the warnings or the warning code should be moved to a analyzer pass which can be toggled on demand. Moving more side effects out of parsing is probably a good idea anyways (see CLJS-948).

Willing to provide a patch for either case.

Comment by David Nolen [ 03/Jan/15 10:56 AM ]

I need wrap up some things around REPL support before this can be addressed. Happy to look at it when things have settled down.

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

Ok, I'll keep an eye of HEAD and provide a new patch if required.

The issue is pretty straightforward though. If cljs.analyzer/parse-ns should not load macros it should bind cljs.analyzer/load-macros to false, currently analzyze-deps sort of hijacks that flag because of (and analyze-deps load-macros).

Comment by David Nolen [ 03/Jan/15 3:24 PM ]

I don't see a problem for your use case other than the API defaults have changed. Why can't you just set analyze-deps to true and load-macros to false?

Comment by Thomas Heller [ 03/Jan/15 3:31 PM ]

because (and true false) equals false.

Comment by David Nolen [ 03/Jan/15 3:36 PM ]

Sorry not understanding your point. You say you want to analyze deps but you don't want to analyze macros. Then false is the right thing in your case. Not wanting to analyze deps but wanting to load macros doesn't make sense to me.

Comment by Thomas Heller [ 03/Jan/15 3:53 PM ]

I use a completely different approach to compiling in dependency order than CLJS does. Basically I do one discovery pass (extracts goog.provide/require from JS files, parse NS of CLJS file) then sort everything and compile top-down (dependency order). analyze with analyze-deps is recursive and compiles bottom-up. Since we were talking about parallel compile, bottom-up doesn't work parallel.

Also since I control the loop that does the file compilation I can compile in memory without touching files (analyze-deps ALWAYS uses anaylze-file).

Given that I can obviously run with analyze-deps on the second pass through since I compile in dep order the deps are already compiled. I just don't want it to ever enter analyze-deps because it doesn't need to.

Also your change didn't change API defaults it broke the API.

load-macros was meant to to be able to skip the require of macros (since I don't need them on the first pass)
analyze-deps used to control if dependencies should be compiled, now it also controls warnings?

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

Well there's not a true API broken here since none of this stuff is officially anything we support.

analyze-deps never controlled whether dependencies should be compiled, it only performs analysis in dependency order so that warning & optimization information can be properly propagated. It falls out of not necessarily compiling files in dependency order. And I believe the current bottom-up behavior still has utility for analysis tools that aren't involved in building.

Still I see the value of keeping these two bits separate in your case now.

Comment by Thomas Heller [ 03/Jan/15 5:26 PM ]

Sorry mixed up compile with analyze. Of cource analyze-file doesn't compile, still it touches files and does the whole caching bits.

Still analyze-deps didn't use to toggle the warnings, but CLJS-948 will probably address all this anyways.

Side Note: I used the two pass approach in shadow-build since I wanted to discover closure-compliant JS files that aren't mentioned in deps.js. I have a few plain JS dependencies in my app which I converted to be closure-aware when I rewrote my project from JS->CLJS. cljs.closure (or lein-cljsbuild) still don't recognize these unless you configure it very explicitly. I think there is an open issue for this as well.

Comment by David Nolen [ 03/Jan/15 5:37 PM ]

I've committed the following to master https://github.com/clojure/clojurescript/commit/7b683ed43c9316d8163c2f764cc6e9b2710c3f46

Hopefully this clears up how parse-ns is actually used by the compiler, which might have been
a source of confusion. I split apart the two flags, however cljs.analyzer/parse-ns has defaults which differ
from the root bindings of *analyze-deps* and *load-macros* which must be
explicitly over-ridden. It's intention was always as a helper for build tools not for running any actual
analysis or loading any macros.

If this is satisfactory then I'll close this ticket.

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

Fixed in master

Comment by Thomas Heller [ 09/Jan/15 10:27 AM ]

CLJS-948 sort of reintroduces the problem.

It is because we combine *analyze-deps* to mean analyze deps and warn about deps. I don't want shadow-build to analyze deps but I want the warnings. Would you be ok with introducing a *check-deps* or should I find another way to address this in shadow-build?

We should resolve CLJS-948 first, I'll come back to this once we have.





[CLJS-948] Simplify macro usage Created: 02/Jan/15  Updated: 09/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved 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.





[CLJS-965] Make :foreign-libs more useful Created: 07/Jan/15  Updated: 08/Jan/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: 2
Labels: None


 Description   

:foreign-libs almost does what we need to make working with JavaScript dependencies painless. They need to additionally specify :requires. :foreign-libs should automatically be preambled in advanced compilation, dependency order can be solved since they provide :requires. Coupled with the deps.clj support using JS deps from JARs would be as simple and as painless as CLJS JARs.

The following conditions must be met:

A) do not break existing usage of deps.clj - means we need to check :foreign-lib true in deps.clj
B) Support optimizations :none, means we must emit goog.addDependency in deps.js
C) Support optimizations :advanced, means we should sort in dependency order and place foreign between :preamble and Closure advanced build
D) users should be able to specify a JS file to use under :none to support debugging



 Comments   
Comment by Yehonathan Sharvit [ 08/Jan/15 1:46 PM ]

Is it currently possible to include a js file inside a cljs lib, without requiring from the library clients to download manually the js file?

Comment by David Nolen [ 08/Jan/15 1:51 PM ]

It is not possible and such a feature is out of scope.

Comment by Francis Avila [ 08/Jan/15 2:45 PM ]

Isn't this done all the time, even with foreign-libs? Or do I misunderstand the question? Yehonathan, what specific scenario are you thinking of? Serving a foreign-lib from a cdn?





[CLJS-698] ^:export on a deftype/record method should goog.exportProperty Created: 25/Nov/13  Updated: 08/Jan/15  Resolved: 08/Jan/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: Declined Votes: 1
Labels: None


 Description   

See http://developers.google.com/closure/compiler/docs/api-tutorial3



 Comments   
Comment by Eduard Bondarenko [ 27/Nov/13 7:37 AM ]
(deftype ^:export SceneMain []
  Object
  (handleShow [_]
    (display-categories)))

I used exportSymbol:

(goog/exportSymbol "SceneMain" SceneMain)
(goog/exportSymbol "SceneMain.prototype.handleShow" SceneMain.prototype.handleShow)

It works even with advanced optimizations:

ca("SceneMain",mg);
ca("SceneMain.prototype.handleShow",SceneMain.prototype.Cb);
Comment by David Nolen [ 28/Nov/13 8:03 AM ]

It would be nice if the following worked:

(deftype ^:export SceneMain []
  Object
  (^:export handleShow [_]
    (display-categories)))
Comment by David Nolen [ 08/Jan/15 2:38 PM ]

goog/exportSymbol works fine.





[CLJS-967] "java.net.ConnectException: Connection refused" when running node repl Created: 07/Jan/15  Updated: 08/Jan/15

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

Type: Defect Priority: Major
Reporter: Brian Muhia Assignee: Unassigned
Resolution: Unresolved 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.





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

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

Type: Defect Priority: Minor
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-962] Inconsistent hashing of empty collections Created: 05/Jan/15  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Major
Reporter: Arthur Ulfeldt Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None
Environment:

clojurescript 0.0-2496"
clojure 1.6.0


Attachments: Text File 0001-CLJS-962-fix-inconsistent-hashing-of-empty-collectio.patch    

 Description   

I get different results when hashing a literal empty vector than when hashing one from read-string, while all the other collections produce the same hash code:

``` clojure
homepage.core> [(hash [2]) (hash (cljs.reader/read-string "[2]"))]
[-1917711765 -1917711765]
homepage.core> [(hash [0]) (hash (cljs.reader/read-string "[0]"))]
[965192274 965192274]
homepage.core> [(hash []) (hash (cljs.reader/read-string "[]"))]
[0 -2017569654] ;; <--- this one looks suspicious.
homepage.core> [(hash {}) (hash (cljs.reader/read-string "{}"))]
[-15128758 -15128758]
homepage.core> (hash #{}) (hash (cljs.reader/read-string "#{}"))
[0 0]
homepage.core> clojurescript-version
"0.0-2496"
```



 Comments   
Comment by David Nolen [ 05/Jan/15 11:24 AM ]

Seems to be a bug that effects all empty collections, the hash values should match Clojure's.

Comment by Michał Marczyk [ 05/Jan/15 6:34 PM ]

Patch with test.

Comment by David Nolen [ 06/Jan/15 7:57 AM ]

Michal, wow thanks for the quick fix!

Comment by David Nolen [ 06/Jan/15 8:00 AM ]

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

Comment by Michał Marczyk [ 06/Jan/15 2:55 PM ]

Thanks for the quick merge!





[CLJS-910] JavaScriptCore 0xbbadbeef EXC_BAD_ACCESS when evaluating (list 0 1 ... 18) Created: 16/Dec/14  Updated: 06/Jan/15

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

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

Embedded JavaScriptCore on iOS simulator
Connected via Weasel / simple-brepl
:whitespace optimization


Attachments: PNG File memory.png     Text File stacktrace.txt    

 Description   

If I evaluate

(list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)

JavaScriptCore exhibits a 0xbbadbeef EXC_BAD_ACCESS, with a fairly deep stacktrace:

(lldb) bt

  • thread #1: tid = 0x3f7e, 0x0111e583 JavaScriptCore`WTFCrash + 67, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x0111e583 JavaScriptCore`WTFCrash + 67
    frame #1: 0x011395a9 JavaScriptCore`WTF::fastMalloc(unsigned long) + 1929
    frame #2: 0x00c9cb56 JavaScriptCore`WTF::Vector<JSC::UnlinkedInstruction, 0ul, WTF::UnsafeVectorOverflow>::expandCapacity(unsigned long) + 86
    frame #3: 0x00c90f27 JavaScriptCore`JSC::BytecodeGenerator::emitGetById(JSC::RegisterID*, JSC::RegisterID*, JSC::Identifier const&) + 311
    frame #4: 0x00fd4617 JavaScriptCore`JSC::DotAccessorNode::emitBytecode(JSC::BytecodeGenerator&, JSC::RegisterID*) + 551
    ...

(Full stack trace attached as stacktrace.txt)

This only occurs with :whitespace optimization and does not under :advanced.

If you evaluate (list 0), it works, and so does (list 0 1), all the way up to 17. Interestingly, it gets progressively slower as you evaluate longer lists.



 Comments   
Comment by Mike Fikes [ 16/Dec/14 12:35 PM ]

While the EXC_BAD_ACCESS is arguably a bug in JavaScriptCore, it is likely provoked by excessive memory usage of the (list 0 1 ...) form. The attached memory.png shows what appears to be 2^n memory usage for evaluating a list of size n. This graph was produced while REPLd into an iOS device, monitoring memory use from Xcode.

Comment by Mike Fikes [ 18/Dec/14 11:28 AM ]

The construct

(list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)

works in r2014 (released Nov 6, 2013), but fails in r2024 (released Nov 8, 2013).

In r2014, the emitted JavaScript is:

cljs.core.list.call(null, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18)

while in r2024 the emitted JavaScript is:

cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null,
cljs.core.List.EMPTY, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1), 0)

This particular commit, between r2014 and r2014 on Nov 7 2013, is likely when the emitted JavaScript changed (I haven't pulled down that specific revision):

https://github.com/clojure/clojurescript/commit/5bcbc4745f599e352c51e01b210755a88aa4bc5f#diff-b64165608bed8fb21a132890b4e2fca2R1279

Comment by Mike Fikes [ 18/Dec/14 12:07 PM ]

Knowing this, it is trivial to reproduce this in desktop Safari (and also see that it works fine in Chrome and Firefox).

If you go to clojurescript.net, or himera.herokuapp.com, and define a function that returns a list constructed with (list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18), you can see that those websites are built using ClojureScript r2014 or earlier, as cljs.core.list.call(null, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18) appears in the reader literal for the returned function.

But, with either of those sites, if you evaluate the following, you will cause a severe performance problem in Safari:

(conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj '() 21) 20) 19) 18) 17) 16) 15) 14) 13) 12) 11) 10) 9) 8) 7) 6) 5) 4) 3) 2) 1) 0)

My hope is that knowing this will make it easier to profile (using desktop tools) what is giving Safari grief executing the resulting JavaScript.

Comment by Mike Fikes [ 18/Dec/14 2:38 PM ]

I don't understand JavaScriptCore's evaluation strategy. I found that if you manually revise the deeply nested composition of cljs.core._conj.call(...) invocations to extract a temporary var or two, as below, then the "doubling" effect is cut short, and the code executes quickly.

This revised code essentially builds a list of length 13 first, and then conses on 3 more elements, and 3 more.

var list0 = cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null,cljs.core.List.EMPTY, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6);

var list1 = cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, list0, 5), 4), 3);

cljs.core._conj.call(null, cljs.core._conj.call(null, cljs.core._conj.call(null, list1, 2), 1), 0)

I found that I can cause something similar to occur from the ClojureScript side by adding a do form, as in:

(conj (conj (conj (conj (conj (do nil
(conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj (conj '() 21) 20) 19) 18) 17) 16) 15) 14) 13) 12) 11) 10) 9) 8) 7) 6) 5)) 4) 3) 2) 1) 0)

Comment by Mike Fikes [ 18/Dec/14 7:00 PM ]

This JavaScriptCore perf problem is easily reproduced purely with JavaScript:

inc=function( x ) { return x + 1; }

alert(
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, inc.call(null, inc.call(null, inc.call(null,
inc.call(null, 1))))))))))))))))))))))

Try this, at, say jsfiddle.net and you will see it max a core and use several GB in Safari, but not FireFox or Chrome.

Comment by Mike Fikes [ 19/Dec/14 9:00 AM ]

As indicated in this ticket's description, this problem doesn't occur with :advanced mode optimizations. Just to confirm, I produced the JavaScript with :pseudo-names set to true for (list 0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18), and you can see that it doesn't use the problematic "call" construct:

$cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$_conj$$($cljs$core$List$EMPTY$$, 18), 17), 16), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1), 0);

Correction 20-DEC-2014: Even with :advanced mode optimization, this occurs. The setting that is needed to avoid the "call" construct is {:static-fns true}, as was set for the above output. With {:static-fnc false}, the emitted coded under :advanced mode is:

$cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$_conj$$.call(null, $cljs$core$List$EMPTY$$, 18), 17), 17), 15), 14), 13), 12), 11), 10), 9), 8), 7), 6), 5), 4), 3), 2), 1);
Comment by Mike Fikes [ 19/Dec/14 9:09 AM ]

Since the fundamental problem is easily reproducible with pure JavaScript, I've opened a StackOverflow regarding it: http://stackoverflow.com/questions/27568249/javascriptcore-deeply-composed-call-performance

Comment by David Nolen [ 19/Dec/14 12:08 PM ]

That was the first thing I was going to suggest trying, repo in plain JavaScript. Thanks for doing this - I believe this may explain some oddities we've encountered elsewhere.

Comment by Mike Fikes [ 19/Dec/14 1:41 PM ]

I’ve filed rdar://19310764 with Apple, and copied it here for reference: http://openradar.appspot.com/radar?id=5864025753649152

Comment by Francis Avila [ 19/Dec/14 5:38 PM ]

Excellent debugging work. Shouldn't this be filed against webkit instead? http://www.webkit.org/quality/reporting.html. (I already searched for this issue in their bug tracker and found nothing.)

This still leaves open the question of whether CLJS should do anything to mitigate. Many of the cljs macros that shadow functions expand recursively--maybe they should expand to loops or reduces instead?

For example:

(defmacro list
  ([] '(.-EMPTY cljs.core/List))
  ([& xs]
    `(let [a# (array ~@(reverse xs))]
       (areduce a# i# l# (list)
         (. l# cljs$core$ICollection$_conj$arity$2 (aget a# i#))))))

Or maybe cheat and emit a ChunkedCons instead?

(defmacro cclist
  ([] '(.-EMPTY cljs.core/LIST))
  ([& xs]
    `(cljs.core/ChunkedCons.
       (cljs.core/ArrayChunk.
         (array ~@(reverse xs)) 0 ~(count xs))
       nil nil nil)))
Comment by Mike Fikes [ 19/Dec/14 6:54 PM ]

Thanks Francis. I've confirmed that it occurs in the WebKit Nightly r177573, and I've moved the ticket to https://bugs.webkit.org/show_bug.cgi?id=139847

Comment by Thomas Heller [ 20/Dec/14 4:19 AM ]

FWIW to toggle between fn.call(null, ...) and fn(...) you can use the compiler option {:static-fns true}. This works with all optimization levels (also :none) but I'm told it causes issues with the REPL. But if you don't use a REPL it is safe to use that option always, I have been for years. Maybe the best short term option.

Comment by Mike Fikes [ 20/Dec/14 8:43 AM ]

Given Thomas's comment, I now realize that my comments above regarding :whitespace and :advanced are incorrect. (My project.clj has a dev and release build, and in addition to a change in the optimization directive, I had a change in the :static-fns directive.)

The correction: This problem occurs for both :whitespace and :advanced, iff :static-fns is set to false.

Comment by David Nolen [ 20/Dec/14 3:22 PM ]

One thing I'm curious about is whether the issue manifests if you're not nesting calls to the same function? This should be easy to confirm with plain JavaScript.

Comment by Mike Fikes [ 20/Dec/14 4:26 PM ]

Hey David, it appears to still cause a problem even with distinct functions. (This fiddle exhibits the problem with distinct functions, so don't follow the link on mobile Safari: http://jsfiddle.net/mfikes/Lwr78cmk/1/ )

Comment by Mike Fikes [ 21/Dec/14 4:16 PM ]

Activity is occurring with the WebKit ticket:

1) It has been confirmed as reproducible
2) It appears to be imported into Apple's system (InRadar keyword added).

Additionally, I succeeded in reproducing the issue on an older PowerPC Mac OS X 10.4.11 Safari 4.1.3 (from 2010), so it is not a recent regression, FWIW.

Comment by Mike Fikes [ 03/Jan/15 7:43 PM ]

Note: CLJS-945 sets :static-fns true as the default.

Comment by David Nolen [ 03/Jan/15 7:49 PM ]

Mike this is true only for cljs.core.

Comment by Mike Fikes [ 06/Jan/15 1:42 PM ]

FWIW, the original ticket I filed with Apple (rdar://19310764) has subsequently been closed by Apple as a duplicate of rdar://19321122, which is the Apple ticket the WebKit team opened in association with https://bugs.webkit.org/show_bug.cgi?id=139847





[CLJS-922] Warning message for non-dynamic vars is incomplete Created: 23/Dec/14  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Michael Griffiths Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: analyzer, cljs

Attachments: Text File CLJS-922-001.patch     Text File CLJS-922-002.patch     PDF File Michael Griffiths Clojure CA.pdf    

 Description   

e.g.

user> (do 
        (cljs.analyzer/analyze nil '((ns dynamic-warning-test-ns)
                                     (def a nil)
                                     (binding [a nil] nil)))
        nil)
WARNING:  not declared ^:dynamic at line 4 
;; => nil

The name of the var is missing at the start of the warning message.



 Comments   
Comment by Michael Griffiths [ 23/Dec/14 3:58 PM ]

Patch attached. Evaluating the above form with patch applied now results in:

WARNING: dynamic-warning-test-ns/a not declared ^:dynamic at line 4

Maybe it would be fine to just use the name of the var rather than the namespace-qualified name - let me know if you want the patch updated.

Comment by David Nolen [ 24/Dec/14 7:52 AM ]

Patch looks good, have you submitted a CA? Thanks.

Comment by Mike Fikes [ 24/Dec/14 8:09 AM ]

I wonder if the fix for CLJS-921 will affect this patch. (I'm thinking that the patch relied on the :name meta containing the fully qualified name, but that with CLJS-921 the :ns would also need to be consulted—as is done in the doc printing impl.)

Comment by David Nolen [ 24/Dec/14 8:16 AM ]

The patch is at the level of the compiler not the runtime so it should be unaffected by CLJS-921.

Comment by Michael Griffiths [ 24/Dec/14 11:29 AM ]

Yeah, I signed an electronic CA the same day I submitted the patch - I'm not sure how often the list of contributors is updated on clojure.org but should be on there soon. Find attached regardless.

Thanks!

Comment by Michael Griffiths [ 05/Jan/15 5:17 AM ]

I'm now listed on http://clojure.org/contributing.

Comment by David Nolen [ 05/Jan/15 9:32 AM ]

The patch needs to be rebased to master.

Comment by Michael Griffiths [ 06/Jan/15 9:54 AM ]

Of course. Rebased patch attached.

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

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





[CLJS-936] Multi arity bitwise operators Created: 30/Dec/14  Updated: 06/Jan/15  Resolved: 06/Jan/15

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

Type: Defect Priority: Trivial
Reporter: Justin Tirrell Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 936.patch     Text File 936.patch    
Patch: Code

 Description   

Unlike regular Clojure, bitwise operators only support two arguments

(bit-or 0 1)

not

(bit-or 0 0 1)



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

Looks good but can we get a new patch that includes tests? Also have you sent in your CA? Thanks!

Comment by Justin Tirrell [ 30/Dec/14 2:13 PM ]

Just attached a new patch that includes tests. I submitted the CA yesterday, should I attach it here?

Comment by David Nolen [ 06/Jan/15 1:39 PM ]

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





[CLJS-957] Parallel compilation Created: 03/Jan/15  Updated: 06/Jan/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.





[CLJS-963] goog dep.js is not recomputed when sources are recompiled Created: 06/Jan/15  Updated: 06/Jan/15  Resolved: 06/Jan/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   

Encountered this in mies when switching from Node.js REPL to Browser REPL



 Comments   
Comment by David Nolen [ 06/Jan/15 7:51 AM ]

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





[CLJS-959] Protocol methods specifying an empty docstring fail to compile Created: 03/Jan/15  Updated: 05/Jan/15  Resolved: 05/Jan/15

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

Type: Defect Priority: Minor
Reporter: Julien Eluard Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File protocol-empty-docstring.diff    

 Description   

Starting ClojureScript 2411 protocols with method specifying an empty docstring fail to compile. Non-empty docstrings are fine.

Following code compiles fine:

(defprotocol Protocol
  (method [_]))
(defprotocol Protocol2
  (method [_] "some doc"))

While following code throws an exception:

(defprotocol Protocol
  (method [_] ""))
clojure.lang.ExceptionInfo: failed compiling file:src/mies_hello_world/core.cljs {:file #<File src/mies_hello_world/core.cljs>}
	at clojure.core$ex_info.invoke(core.clj:4403)
	at cljs.compiler$compile_file.invoke(compiler.clj:999)
	at cljs.compiler$compile_root.invoke(compiler.clj:1029)
	at cljs.closure$compile_dir.invoke(closure.clj:358)
	at cljs.closure$fn__2534.invoke(closure.clj:398)
	at cljs.closure$fn__2484$G__2479__2491.invoke(closure.clj:306)
	at cljs.closure$fn__2527.invoke(closure.clj:412)
	at cljs.closure$fn__2484$G__2479__2491.invoke(closure.clj:306)
	at cljsbuild.compiler.SourcePaths$fn__134.invoke(compiler.clj:67)
	at clojure.core$map$fn__4245.invoke(core.clj:2557)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.core$mapcat.doInvoke(core.clj:2586)
	at clojure.lang.RestFn.invoke(RestFn.java:423)
	at cljsbuild.compiler.SourcePaths._compile(compiler.clj:67)
	at cljs.closure$build.invoke(closure.clj:1018)
	at cljs.closure$build.invoke(closure.clj:972)
	at cljsbuild.compiler$compile_cljs$fn__145.invoke(compiler.clj:81)
	at cljsbuild.compiler$compile_cljs.invoke(compiler.clj:80)
	at cljsbuild.compiler$run_compiler.invoke(compiler.clj:179)
	at user$eval277$iter__295__299$fn__300$fn__312.invoke(form-init1238899578077056082.clj:1)
	at user$eval277$iter__295__299$fn__300.invoke(form-init1238899578077056082.clj:1)
	at clojure.lang.LazySeq.sval(LazySeq.java:40)
	at clojure.lang.LazySeq.seq(LazySeq.java:49)
	at clojure.lang.RT.seq(RT.java:484)
	at clojure.core$seq.invoke(core.clj:133)
	at clojure.core$dorun.invoke(core.clj:2855)
	at clojure.core$doall.invoke(core.clj:2871)
	at user$eval277.invoke(form-init6397079933373263622.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6703)
	at clojure.lang.Compiler.eval(Compiler.java:6693)
	at clojure.lang.Compiler.load(Compiler.java:7130)
	at clojure.lang.Compiler.loadFile(Compiler.java:7086)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	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.ExceptionInfo: Invalid protocol, Protocol defines method method with arity 0 at line 8 src/mies_hello_world/core.cljs {:tag :cljs/analysis-error, :file "src/mies_hello_world/core.cljs", :line 8, :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:1520)
	at cljs.analyzer$analyze_seq.invoke(analyzer.clj:1563)
	at cljs.analyzer$analyze$fn__1360.invoke(analyzer.clj:1654)
	at cljs.analyzer$analyze.invoke(analyzer.clj:1647)
	at cljs.compiler$compile_file_STAR_$fn__2213.invoke(compiler.clj:909)
	at cljs.compiler$with_core_cljs.invoke(compiler.clj:867)
	at cljs.compiler$compile_file_STAR_.invoke(compiler.clj:888)
	at cljs.compiler$compile_file.invoke(compiler.clj:993)
	... 44 more
Caused by: java.lang.Exception: Invalid protocol, Protocol defines method method with arity 0
	at cljs.core$defprotocol$fn__3323.invoke(core.clj:1017)
	at cljs.core$defprotocol.doInvoke(core.clj:1015)
	at clojure.lang.RestFn.applyTo(RestFn.java:146)
	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:1526)
	... 51 more

I figured it was worth reporting giving the error message is a little unexpected.



 Comments   
Comment by David Nolen [ 05/Jan/15 11:44 AM ]

fixed https://github.com/clojure/clojurescript/commit/7561c37231100fbdbe620b5dda7368e7bd2da632





[CLJS-961] REPL process should always be killable Created: 04/Jan/15  Updated: 04/Jan/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   

In the browser REPL case if user never connects browser cannot kill REPL with Ctrl-C or Ctrl-D.






[CLJS-960] On carriage return REPLs should always show new REPL prompt Created: 04/Jan/15  Updated: 04/Jan/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





[CLJS-958] Node.js REPL: Upon error, last successfully item printed Created: 03/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

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

0.0-2657
Node.js REPL set up as per http://swannodette.github.io/2014/12/29/nodejs-of-my-dreams/

uname -a
Linux ubuntu 3.13.0-32-generic #57-Ubuntu SMP Tue Jul 15 03:51:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

java -version
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)

node --version
v0.10.25



 Description   

If at the REPL, you evaluate a form that results in an error, the last successfully printed item prints again.

Here is an example:

ClojureScript:cljs.user> (time (reduce + (range 1000000)))
"Elapsed time: 427 msecs"
499999500000
ClojureScript:cljs.user> (first (js/Date.))
Error: Sat Jan 03 2015 19:48:17 GMT-0500 (EST) is not ISeqable
    at seq (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:672:20)
    at first (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:681:16)
    at repl:1:81
    at repl:9:3
    at Socket.<anonymous> ([stdin]:27:80)
    at Socket.EventEmitter.emit (events.js:95:17)
    at Socket.<anonymous> (_stream_readable.js:746:14)
    at Socket.EventEmitter.emit (events.js:92:17)
    at emitReadable_ (_stream_readable.js:408:10)
    at emitReadable (_stream_readable.js:404:5)
499999500000

Note, specifically the `499999500000` re-printed after the error.



 Comments   
Comment by David Nolen [ 03/Jan/15 7:28 PM ]

The problem is we're allowing Node.js to print the error instead of letting the REPL infrastructure handle it.

Comment by David Nolen [ 03/Jan/15 7:48 PM ]

fixed https://github.com/clojure/clojurescript/commit/9072b4bfdaf5cf9c66c4770c545f066fbec6256e

Comment by Mike Fikes [ 03/Jan/15 7:54 PM ]

Confirmed fixed in my environment with https://github.com/clojure/clojurescript/commit/9072b4bfdaf5cf9c66c4770c545f066fbec6256e

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

Thanks for the confirm.





[CLJS-945] Compile core with :static-fns true by default Created: 01/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/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


 Comments   
Comment by David Nolen [ 03/Jan/15 7:06 PM ]

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





[CLJS-877] to-string of js/Date objects is inconsistent Created: 30/Oct/14  Updated: 03/Jan/15  Resolved: 03/Jan/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: Declined Votes: 0
Labels: None
Environment:

0.0-2371, Chrome



 Description   

When converting a js/Date object to a string, different things happen depending on whether the object is nested in a data structure or not.

For example:

(def date (js/Date.))
(str date) ;; => "Thu Oct 30 2014 16:49:50 GMT-0400 (EDT)"
(str [date]) ;; => "[#inst \"2014-10-30T20:49:50.999-00:00\"]"


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

The behavior is identical to java.util.Date in Clojure. Not saying it isn't a bug but a discussion should probably be started elsewhere first.

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

Closing this for now, behavior mirrors Clojure





[CLJS-759] Exceptions are reported at weird locations in the presence of bad quasiquoting Created: 31/Jan/14  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Minor
Reporter: Jamie Brandon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

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



 Description   

(defn foo [x]
`(f ~@(for [i (range 10)] ~i)))

(foo 1)

This code has an extra unquote next to the i causing:

TypeError: Cannot call method 'call' of undefined
at aurora.compiler.foo.cljs.core.with_meta.call.cljs.core.seq.call.cljs.core.concat.call.iter_7602auto_ (/home/jamie/aurora/src/aurora/compiler.cljs[eval362]:229:100)

Compare this to:

(defn foo [x]
(bar x))

(foo 1)

Which causes:

TypeError: Cannot call method 'call' of undefined
at foo (/home/jamie/aurora/src/aurora/compiler.cljs[eval376]:212:67)

In the first case, the symbol is mangled and the line number and column are incorrect.



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

The stack trace can be a bit strange in some environments but the foo does in fact appear when I tested. It did uncover a bug with cljs.core/unquote now resolved in master https://github.com/clojure/clojurescript/commit/baa2e227cd99b9710e7a0af91d44a15b54d60828





[CLJS-503] v8 penalizes coercive nil? Created: 04/May/13  Updated: 03/Jan/15  Resolved: 03/Jan/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: Declined Votes: 0
Labels: None


 Description   

We check for nil coercively with != and = because we want to handle both JavaScript null and undefined. These operations are slower than !== and ==. We would like to fix this but we can't simply make nil? not be coercive. The persistent data structures rely on this because they reach into JS Arrays that been allocated with a certain size. However in JS the empty slots are filled with undefined and null requiring the more expensive coercive nil? check.

Making nil? non-coercive seems to bring less of performance boost in WebKit/Firefox Nightlies, but v8 is still the king and everyone seems to be moving towards it performance wise.

One issue with making nil? non-coercive is that undefined will then fall through many of the core library functions. This means that users will need to be more careful in interop scenarios.



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

probably too small of a hit to care at this point.





[CLJS-543] printing machinery should take an option to that println as js/console in the browser doesn't emit an extra line feed Created: 16/Jul/13  Updated: 03/Jan/15  Resolved: 03/Jan/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


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

Not sure when this was resolved but fixed.





[CLJS-407] cljs.import-test not run in test suite / ordering problem in the compiler Created: 24/Oct/12  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-407-Hook-up-cljs.import-test-to-the-test-runner.patch    

 Description   

The test code from CLJS-312 is not hooked up to the test suite, which is trivially fixed.

Hooking up the test, however reveals a deeper problem (besides that it has a failing assumption):

Presumably due to the goog.provides later in the files, the compiler orders the output wrong, which results in not provided errors.
Most interestingly, this problem goes away, when compiling from a prepopulated out directory, presumably because then the closure compiler handles the ordering?



 Comments   
Comment by Herwig Hochleitner [ 24/Oct/12 3:23 AM ]

Patch hooks up the tests into the test suite.

If you want to see the provide error, delete out before compiling.

If you want to see the failing assertion from the original test, compile a second time.

Comment by David Nolen [ 24/Oct/12 11:01 AM ]

In order to better understand tickets it best not to complect different issues If there's a problem not addressed by the patch please mention that in another ticket (and feel to reference that one here).

I assume the patch fixes one thing, but reveals an unresolved issue, am I correct?

Comment by Herwig Hochleitner [ 24/Oct/12 11:26 AM ]

You're right, the ordering problem could be a different ticket. Sorry for that.

To clarify, there are three issues at play:

1) Part of the testsuite not executed (fixed by the patch)
2) The part that is now executed fails, didn't have time this morning to find out the original intention of Michał, can probably do tomorrow, if he doesn't pick it up.
3) The compilate is different depending on whether the .js file is cached in out/. This might require some deeper investigation.

The reason I decided to roll it into one ticket is that only addressing one of 2) and 3) leaves the test suite failing. I almost wanted to reopen CLJS-312, but decided against it, since it's water under the bridge.

Comment by David Nolen [ 21/Dec/12 6:07 PM ]

I fixed some dependency ordering issue in another patch, would be nice to know if the ordering issues are addressed. What part of this ticket is still relevant? This is why tickets that only address one thing at a time are nice

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

This no longer seems to be an issues with the arrival of cljs.test





[CLJS-344] clojure.reflect is asynchronous (should use CrossPageChannel) Created: 25/Jul/12  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Defect Priority: Minor
Reporter: David Nolen Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None


 Description   

This makes printing at the REPL a bit annoying. We should be using the CrossPageChannel and not a ajax request to the built in webserver.



 Comments   
Comment by Brandon Bloom [ 03/Sep/12 3:36 PM ]

Was this ticket supposed to be filed against Clojure, not ClojureScript?

Comment by David Nolen [ 03/Sep/12 3:51 PM ]

No. This ticket refers to the HackerSchool work to provide a reflection hook for ClojureScript to make the REPL interactions more friendly. Look at clojure.reflect in the ClojureScript repo.

Comment by Frank Siebenlist [ 08/Oct/12 4:55 PM ]

As we've changed the topic of this issue a little bit..., I'd like to add the additional requirement that the reflection facility should be pluggable, meaning that if I have additional help/reflection functions that I want to make available to the cljs-repl users, then I should not have to wait for acceptance in a new clojurescript version.

Maybe some form of registration API thru which I can add new cljs-functions that communicate with their clj-counterparts thru some naming convention . Not sure about the details here...

Comment by David Nolen [ 08/Oct/12 5:06 PM ]

That's really a separate issue that probably warrants it's own discussion.

Comment by Frank Siebenlist [ 08/Oct/12 5:24 PM ]

Well... you started to add the (seemingly unrelated) CrossPageChannel requirement to the asynchronous one

But seriously, the pluggability requirement is probably important to keep in mind when any implementation for asynchronous clojure/reflect is designed and implemented - I'll open-up another issue for pluggability to keep it clean.

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

clojure.reflect is effectively deprecated now that we have better REPL special fns & static vars





[CLJS-345] clojure.reflect support for Rhino REPL Created: 25/Jul/12  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

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


 Description   

clojure.reflect should work for the Rhino REPL



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

REPL special fns & static vars cover most of the use cases for clojure.reflect quite well





[CLJS-748] Dump analyzer state during a compilation. Created: 13/Jan/14  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Gary Trakhman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler

Attachments: Text File CLJS_748.patch    
Patch: Code

 Description   

CLJS doesn't have the luxury of a fully-reified runtime environment, therefore effective tooling needs a view of the internal compiler state. This issue addresses this need by enabling a dump of the compiler state based on additional cljs compiler options.

The compiler state is a map contained within an atom, either passed in as the 'compiler-env' arg to cljs.closure/build or contained globally within cljs.env/compiler.

The prototype is implemented as such:

:dump-analysis-to
either a string or :print, when :print, output will be written to out, when a string, the argument will be passed to clojure.java.io/writer.

:dump-analysis-full
checked for truthiness, default is false
When this is switched on, the full structure of the compiler state will be printed, which is impractically large for most use cases. In normal operation, :env keys and extended :method-params will be removed from the output, making the analysis represent simply the globally reachable environment. Additionally, only the contents under :cljs.analyzer/namespaces and :cljs.analyzer/constants will be printed.



 Comments   
Comment by Gary Trakhman [ 13/Jan/14 6:46 PM ]

Added implementing patch

Comment by David Nolen [ 16/Jan/14 5:21 PM ]

I question the utility of :dump-analysis-full for now. Lets remove it.

Comment by Gary Trakhman [ 16/Jan/14 5:32 PM ]

I was thinking it might be good along with :print followed by a pipe, but... I don't have an explicit use-case. I'll create a new patch.

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

This ticket is subsumed by :cache-analysis support





[CLJS-449] stack traces for ex-info Created: 23/Dec/12  Updated: 03/Jan/15

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

Type: Enhancement Priority: Major
Reporter: Tom Jack Assignee: Unassigned
Resolution: Unresolved 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.





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

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

Type: Enhancement Priority: Minor
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Unresolved 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)




[CLJS-916] Optimize use of js-arguments in array and variadic functions Created: 22/Dec/14  Updated: 03/Jan/15  Resolved: 03/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File bench-fastcall.txt     Text File bench-master.txt     Text File cljs-916.patch    
Patch: Code

 Description   

There are currently three places where the magic arguments variable is used by the clojurescript codebase directly: the array function, in the outer arity dispatch for variadic functions, and in the max-fixed-arity method for variadic functions and protocols. In each case the goal is to cast the function arguments (or some subset) to a normal array, and in each case arguments is used in a way that is difficult for many vms to optimize. (V8 in particular cannot ever jit-optimize any function body where arguments is passed as a parameter to another function.) Microbenchmark js-perf of argument-to-array approaches

Attached is a patch which handles arguments better by looping over arguments inline and copying to a pre-allocated array. This gives me a 10x speedup in V8 for calls like (vector ...) and (array ...), and less significant speedups elsewhere.

Note this optimization really only matters for higher-order use of variadic functions and for compilation modes where :static-fns is off. Many functions (including array)are shadowed by macros which eliminate the arity dispatches and the :static-fns optimization will remove most other dispatches.

I have attached two advanced-compiled benchmarks (using script/benchmark) in v8, spidermonkey, and javascriptcore comparing master vs this patch on my machine. Note that this adds a few benchmark cases specifically to exercise the patched code. Look for ";; higher-order variadic function calls" near the end.



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

a nice performance enhancement, thanks! fixed https://github.com/clojure/clojurescript/commit/dccc5d0feab9809cb56bd4d7257599a62232ac0d





[CLJS-956] No *print-fn* fn set for evaluation environment new REPL Created: 03/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/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: Duplicate Votes: 0
Labels: None
Environment:

0.0-2644
Node.js REPL
Ubuntu



 Description   

Some evaluations print, like

ClojureScript:cljs.user> (first [1 2 3])
1

But others result in an error regarding print-fn, and interestingly re-print the latest successful print

ClojureScript:cljs.user> (require '[clojure.string :as string])

ClojureScript:cljs.user> (string/join ", " [1 2 3])
"1, 2, 3"
ClojureScript:cljs.user> (time (reduce + (range 1000000)))
Error: No *print-fn* fn set for evaluation environment
    at _STAR_print_fn_STAR_ (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:26:12)
    at string_print (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8009:4)
    at pr_with_opts (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8139:4)
    at cljs.core.prn.prn__delegate (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8191:4)
    at cljs.core.prn.prn (/home/mfikes/clojurescript/.cljs_node_repl/cljs/core.cljs:8190:6)
    at repl:3:15
    at repl:6:3
    at repl:14:3
    at Socket.<anonymous> ([stdin]:27:80)
    at Socket.EventEmitter.emit (events.js:95:17)
"1, 2, 3"


 Comments   
Comment by David Nolen [ 03/Jan/15 10:56 AM ]

This is actually a dupe of http://dev.clojure.org/jira/browse/CLJS-954.

Comment by Mike Fikes [ 03/Jan/15 1:48 PM ]

Confirmed fixed in 0.0-2655.

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

Thanks for the confirmation.





[CLJS-954] require needs to follow Clojure semantics more closely, support :reload, :reload-all Created: 02/Jan/15  Updated: 03/Jan/15  Resolved: 03/Jan/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   

Currently :reload-all is implicit, should be explicit.



 Comments   
Comment by David Nolen [ 02/Jan/15 8:33 PM ]

We need :reload-all and :reload support in the ns form. goog.require only supports a single arity, so we can trivially monkey-patch a two arity function. We need to discard this information in any situation other than :none, otherwise will get in the way of static dependency loading. We can and should follow the logic that is already present in Clojure here around loaded-libs.

Comment by David Nolen [ 02/Jan/15 8:51 PM ]

Plan, we can add a loaded-libs for use by REPLs that monkey-patch the behavior of goog.require. For :reload case we can just emit goog.require("foo.core", true); for :reload-all we can save loaded-libs reload everything and merge the new loaded-libs with the old value same as clojure.core/load-all.

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

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





[CLJS-952] Bad type hinting on bit-test Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Defect Priority: Minor
Reporter: Francis Avila Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File cljs-952.patch    
Patch: Code

 Description   

bit-test returns a boolean, but is incorrectly type-hinted as returning a number.



 Comments   
Comment by David Nolen [ 02/Jan/15 4:47 PM ]

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





[CLJS-953] require REPL special fn can only take one argument Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/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   

Not variadic like Clojure



 Comments   
Comment by David Nolen [ 02/Jan/15 4:46 PM ]

fixed https://github.com/clojure/clojurescript/commit/02524d15db5e25dff4c1934ae4b28a2010ea2fb9





[CLJS-947] REPL require of goog namespaces does not work Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/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 [ 02/Jan/15 4:30 PM ]

fixed https://github.com/clojure/clojurescript/commit/935b0e466c6bd327799ca0a04f062b8aa23dcebe





[CLJS-951] goog.require emitted multiple times under Node.js REPL Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/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   

This causes a namespace to be required multiple times.



 Comments   
Comment by David Nolen [ 02/Jan/15 3:31 PM ]

fixed https://github.com/clojure/clojurescript/commit/251b76be8d7c072e272ca8d90edec64d6d18d5b6





[CLJS-946] goog.require in REPLs will not reload recompiled libs Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/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 makes for a bad REPL experience. (require 'foo.core) will recompile, but the emitted goog.require does not correctly reload the already seen namespace. We should monkey patch goog.require for all REPLs so that this works correctly.



 Comments   
Comment by David Nolen [ 01/Jan/15 4:53 PM ]

This is actually like a problem with Node.js integration, we need to invalidate the cache http://nodejs.org/docs/latest/api/globals.html#globals_require_cache

Comment by David Nolen [ 01/Jan/15 10:23 PM ]

fixed https://github.com/clojure/clojurescript/commit/4665f9a787c2bcb9d76066ba9ddccb89cf378dc3

Comment by David Nolen [ 01/Jan/15 10:25 PM ]

Not quite fixed, the paths are correct, but it appears goog.require has some logic that prevents from even getting to Node.js require

Comment by David Nolen [ 02/Jan/15 1:04 PM ]

fixed https://github.com/clojure/clojurescript/commit/187a45ff599eafd9e8b4f95bb31778a84ac9322f





[CLJS-950] In 0.0-2629, Can not run compiled source with node targert Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Defect Priority: Major
Reporter: tunde ashafa Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

// Compiled by ClojureScript 0.0-2629 {}
#!/usr/bin/env node

Script hashbang on the second line seems to be an issue when running script with node via command line.



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

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

Comment by tunde ashafa [ 02/Jan/15 11:53 AM ]

Thank you





[CLJS-944] deps file produced by :none needs compilation comment Created: 01/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/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   

Without this a different version of the compiler believes that recompilation is not necessary.



 Comments   
Comment by David Nolen [ 01/Jan/15 2:31 PM ]

Fixed in master but is not the root reason project is not recompiled. Needs more investigation.

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

This is a bug in cljsbuild. cljsbuild does it own file change detection to trigger rebuilds - this is at odds with the compiler checking compiled artifacts for staleness.





[CLJS-949] In 0.0-2629, Can not run compiled source with node targert Created: 02/Jan/15  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Defect Priority: Major
Reporter: tunde ashafa Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

// Compiled by ClojureScript 0.0-2629 {} |;; This buffer is for notes you don't want to save, and for Lisp evaluation.
#!/usr/bin/env node



 Comments   
Comment by tunde ashafa [ 02/Jan/15 10:51 AM ]

Please delete this issue. Hit a shortcut to create this ticket prematurely. Updated ticket is #CLJS-950





[CLJS-929] Minor fixes to test script Created: 26/Dec/14  Updated: 02/Jan/15  Resolved: 02/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Alex Dowad Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

Linux Mint 17.1, Linux 3.13.0-24-generic kernel, x86_64 CPU


Attachments: Text File 929.patch     Text File 929.patch     Text File 929.patch    
Patch: Code

 Description   

$[], used in ./script/test, doesn't work in ash or dash. This is a problem for users of Ubuntu and other Debian-based Linux distros, since they ship with dash as the default /bin/sh. (Some other UNIX-like OSs, including various flavors of BSD, as well as Minix, also use ash.)

$(()) works in ash, dash, bash, and zsh.



 Comments   
Comment by Alex Dowad [ 26/Dec/14 1:33 AM ]

Grrr, there was a typo in the first patch. Here is a corrected version.

Comment by David Nolen [ 01/Jan/15 3:35 PM ]

This patch does not apply cleanly to master anymore. Can we get a rebased patch? Thanks.

Comment by Alex Dowad [ 02/Jan/15 3:13 AM ]

Rebased patch attached.

Comment by David Nolen [ 02/Jan/15 8:46 AM ]

fixed https://github.com/clojure/clojurescript/commit/6aaaabe0fb8727ac762343b77c36c16288448589





[CLJS-803] Spurious undeclared Var warning in REPL Created: 05/May/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Minor
Reporter: Paul Butcher Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

OS X Mavericks, Safari 7.0.3, Chrome 34.0.1847.131



 Description   

With the following minimal Compojure/ClojureScript project:

https://github.com/paulbutcher/csrepl

Compile with "lein cljsbuild once", then run "lein trampoline cljsbuild repl-listen". In another window, run "lein ring server".

The src/csrepl/core.cljs file defines an atom called app-state. This can be successfully examined in the REPL as follows:

$ lein trampoline cljsbuild repl-listen
Running ClojureScript REPL, listening on port 9000.
To quit, type: :cljs/quit
ClojureScript:cljs.user> csrepl.core/app-state
#<Atom: {:text "hello world"}>

But, if you switch to the csrepl.core namespace and do the same, a spurious "undeclared Var" warning is generated:

ClojureScript:cljs.user> (ns csrepl.core)
nil
ClojureScript:csrepl.core> app-state
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
#<Atom: {:text "hello world"}>
ClojureScript:csrepl.core> (:text @app-state)
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
"hello world"

Further, once you're in the csrepl.core namespace, the same warning is generated even when using the fully qualified name:

ClojureScript:csrepl.core> csrepl.core/app-state
WARNING: Use of undeclared Var csrepl.core/app-state at line 1 <cljs repl>
#<Atom: {:text "hello world"}>


 Comments   
Comment by David Nolen [ 01/Jan/15 3:41 PM ]

This is not a ClojureScript issue, the REPL need to be configured to analyze the output directory so that it has the correct information. This should be fixed at cljsbuild if it's still a problem there.





[CLJS-930] Spelling mistakes in function documentation Created: 26/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Minor
Reporter: Benjamin Meyer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-spelling-mistakes-in-function-documentation.patch    
Patch: Code

 Description   

The clojurescript function documentation contained a few spelling mistakes.

I have signed the Contributor Agreement and created a patch with the fixes



 Comments   
Comment by David Nolen [ 01/Jan/15 3:37 PM ]

fixed https://github.com/clojure/clojurescript/commit/077ccaad863e0fdb0533e13a82e46963022cb018





[CLJS-902] Give defined type constructors names for debugging's sake Created: 03/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: patch

Attachments: Text File 0001-CLJS-902-Give-ctors-names-for-debugging-purposes.patch    
Patch: Code

 Description   

I've wished more than once for the ability to use (.. x -constructor -name) during debugging to be able to tell what type an object has. The attached patch makes that work.



 Comments   
Comment by David Nolen [ 01/Jan/15 3:36 PM ]

pr-str already does the right thing for ClojureScript types





[CLJS-932] Node REPL errors on exit Created: 29/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/15

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

Type: Defect Priority: Minor
Reporter: Peter Schuck Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File node_repl_close.patch    
Patch: Code

 Description   

When -tear-down is called on NodeEnv the socket atom isn't dereferenced so the close method is called on the socket atom rather then the sockets.



 Comments   
Comment by David Nolen [ 01/Jan/15 3:32 PM ]

fixed https://github.com/clojure/clojurescript/commit/6296384e536212c8997aa6fa05d2f6f29ffbb077





[CLJS-931] cljs.compiler/requires-compilation? ignores changes to build options Created: 27/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/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

Attachments: Text File cljs-931.patch     Text File cljs-931.patch    

 Description   

static-fns, optimize-constants, and elide-asserts change the emitted code. If these build settings change we need to recompile. Compiled files should includes in a JS comment the EDN set literal of the code affecting compiler build options used.



 Comments   
Comment by Thomas Heller [ 29/Dec/14 7:54 AM ]

Not sure that requires-compilation? is the correct entry point but I believe it is vital that files that produced warnings are always recompiled. It is very easy to miss warnings and until you wipe all files you won't see them again.

Comment by David Nolen [ 30/Dec/14 2:00 PM ]

Thomas, I think we want to avoid addressing the symptom over the cause. It may also very well be the case that CLJS-927 is the solution to this particular problem.

Comment by Romain Primet [ 30/Dec/14 5:03 PM ]

I submitted a patch for this issue following an IRC chat earlier today.

Comment by Romain Primet [ 30/Dec/14 5:18 PM ]

Version number issue in the first patch.

Comment by David Nolen [ 30/Dec/14 6:38 PM ]

This looks pretty good, will apply when your CA appears. Thanks very much!

Comment by Romain Primet [ 30/Dec/14 6:44 PM ]

Great, thanks for the guidance!

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

Just tried to apply this to master. 2 tests fail for me when running lein test.

Comment by Romain Primet [ 31/Dec/14 5:00 PM ]

I had this issue, but that was due to there being no cljs version number defined by the build script. Once corrected it passed. Could you try this?

Comment by David Nolen [ 31/Dec/14 5:10 PM ]

The patch should be modified so that the test can pass. A simple way to do this is with with-redefs so you can provide a dummy ClojureScript version.

Comment by Romain Primet [ 31/Dec/14 5:25 PM ]

Fixed the test to pass without requiring a cljs version number

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

fixed https://github.com/clojure/clojurescript/commit/79157c2fbc7aece8c3c13f9d78cc6bbb4cc3be37





[CLJS-943] REPL require special fn is brittle Created: 31/Dec/14  Updated: 01/Jan/15  Resolved: 01/Jan/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   

Spec merging needs to be more robust. Additionally all errors during ns form parsing should throw not just print a warning. Otherwise the information about the ns due to REPL interaction can enter a corrupted state.



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

Fixed https://github.com/clojure/clojurescript/commit/1a7546ecba81683b67278a47dec490761e13718e





[CLJS-938] actual configurable port for Node.js REPL Created: 30/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/14

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 hardcoded on the Node.js side to 5001



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

fixed by CLJS-942 https://github.com/clojure/clojurescript/commit/133721a20a4879253b5ce5f913bd2f6555b8e3c6





[CLJS-942] Randomized port for Node.js REPL Created: 31/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/14

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 need to randomize the port so the user can start multiple repls without bother to reason about which port to use.



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

fixed https://github.com/clojure/clojurescript/commit/133721a20a4879253b5ce5f913bd2f6555b8e3c6





[CLJS-940] Make macro loading of the 'ns form optional Created: 31/Dec/14  Updated: 31/Dec/14  Resolved: 31/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File optional-macro-loading.diff    

 Description   

When parsing the (ns ...) form it has the costly side effect of immediately require'ing all macro namespaces it finds. While this is nescessary for parsing the rest of the file it is not required when just parsing the ns form.

Tools should be able to disable the loading of the macros to allow faster inspection of the ns form without this side-effect.

In shadow-build I have a function which basically does what cljs.build.api/parse-js-ns does just for ClojureScript files, this function is slow initially and I'd like be able to delay the require of macro namespaces as they are not required when just looking at the ns form.



 Comments   
Comment by Thomas Heller [ 31/Dec/14 11:16 AM ]

Whoops, the first patch didn't come out right. Fixed.

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

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





[CLJS-833] A named fn shadows `js/fn-name` Created: 04/Aug/14  Updated: 31/Dec/14  Resolved: 30/Dec/14

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 0001-CLJS-833-Test-for-fn-name-shadowing.patch    

 Description   

Description

The function

(fn console [] js/console)

will return a reference to itself when called.
This happens because the function is transpiled to

(function console(){return console;})

Solution proposals

Mangle internal function names like let bindings

The internal name of a generated js function should be treated like a let binding, hence gensymed.
Thus the function would be transpiled to something like

(function console_1337(){return console;})

References

Brought up in https://groups.google.com/d/msg/clojure/QZmGrjNVurs/NxFtq8yDCFIJ



 Comments   
Comment by Herwig Hochleitner [ 04/Aug/14 4:10 AM ]

Attached test uses js/eval, because that's one of the shortest global identifiers, that should be available on every js runtime.

Comment by Herwig Hochleitner [ 04/Aug/14 12:43 PM ]

As detailed in the ML thread, CLJS-680 introduced :js-globals in an attempt to fix this issue. :js-globals should be removed along with a proper fix.

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

We're not going to do this. If anything we're going to move towards more stable names - it is a requirement if we ever want to achieve CLJS step debugging via remote debugging protocols.

Comment by Herwig Hochleitner [ 31/Dec/14 4:24 AM ]

The reason for closing this seems bogus to me: There will always be some amount of lexical name mangling in clojurescript, because otherwise some code will miscompile (see CLJS-401). Therefore any hypothetical step debugger must already have some sort of source mapping information from the compiler. Why should more mangling do any more harm then?

I say: Just gensym each and every single lexical binding and be done with it forever. JS names are not meant to directly correspond to CLJS names.

Comment by David Nolen [ 31/Dec/14 10:49 AM ]

That "there will be some amount of lexical name mangling" is about as problematic as there will be "some inaccuracy to source mapping". Let's wait and see. Until then we prioritize stable names from source produced by the compiler.

One interesting line of thought is to eliminate gensym'ing altogether and instead use a source map like approach for generating stable names where every symbol is instead uniquely identifiable by namespace index N, and line N, column N.





[CLJS-939] Quit noderepljs with EOF Created: 31/Dec/14  Updated: 31/Dec/14

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

Type: Defect Priority: Minor
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

As is common in every other REPL out there, Ctrl+D on a new line should quit the REPL






[CLJS-675] QuickStart example not working properly Created: 10/Nov/13  Updated: 31/Dec/14  Resolved: 31/Dec/14

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

Type: Defect Priority: Minor
Reporter: Anton Logvinenko Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None
Environment:

To reproduce only have to follow simple steps as described in https://github.com/clojure/clojurescript/wiki/Quick-Start
git clone git://github.com/clojure/clojurescript.git
cd clojurescript
./script/bootstrap


Attachments: Text File CLJS-675.patch    

 Description   

Section "Using ClojureScript on a Web Page"
Executing:

./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

produces error message "java.io.FileNotFoundException: out/constants_table.js (No such file or directory)"

Just creating "out" directory seems to be fixing the problem. The directory is then deleted after the compilation.

mkdir out
./bin/cljsc hello.cljs '{:optimizations :advanced}' > hello.js

Also, "out" directory is mentioned in .gitignore.



 Comments   
Comment by Anton Logvinenko [ 10/Nov/13 5:28 AM ]

UPD: The "out" directory is not deleted after the compilation. It's simply ignored by git.

Comment by Steven Kallstrom [ 15/Oct/14 4:05 PM ]

I encountered the same issue. Patch submitted.

Comment by Daniel Brotsky [ 31/Dec/14 3:45 AM ]

Seems like a simple fix is to mention this issue in the QuickStart instructions.

Comment by David Nolen [ 31/Dec/14 10:38 AM ]

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





[CLJS-937] local fn name should be namespace munged Created: 30/Dec/14  Updated: 30/Dec/14

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   
(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





[CLJS-933] Redef warning omiting namespace of original symbol Created: 29/Dec/14  Updated: 30/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Shaun LeBron Assignee: Unassigned
Resolution: Unresolved 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.





[CLJS-935] script/noderepljs leaves node running after exit Created: 30/Dec/14  Updated: 30/Dec/14  Resolved: 30/Dec/14

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

Type: Defect Priority: Major
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

After starting noderepljs, then exiting it (via :cljs/quit), the node process is left running and listening on port 5001. This causes all subsequent executions of noderepljs to report "Error: listen EADDRINUSE"

clojurescript master mn › ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 5001
ClojureScript:cljs.user> :cljs/quit

clojurescript master mn › ps
PID TTY TIME CMD
17400 ttys000 0:00.10 -bash
20391 ttys000 0:00.17 node
17634 ttys001 0:00.06 -bash

clojurescript master mn › ./script/noderepljs
To quit, type: :cljs/quit
ClojureScript Node.js REPL server listening on 5001
Error: listen EADDRINUSE
at errnoException (net.js:904:11)
at Server._listen2 (net.js:1042:14)
at listen (net.js:1064:10)
at Server.listen (net.js:1138:5)
at [stdin]:42:4
at Object.<anonymous> ([stdin]-wrapper:6:22)
at Module._compile (module.js:456:26)
at evalScript (node.js:536:25)
at ReadStream.<anonymous> (node.js:154:11)
at ReadStream.emit (events.js:117:20)
goog.require could not find: cljs.core
Error: goog.require could not find: cljs.core
at Error (<anonymous>)
at Object.goog.require (/Users/mtnygard/work/clojurescript/.cljs_node_repl/goog/base.js:470:13)
at repl:1:6
at Socket.<anonymous> ([stdin]:26:80)
at Socket.emit (events.js:95:17)
at Socket.<anonymous> (_stream_readable.js:764:14)
at Socket.emit (events.js:92:17)
at emitReadable_ (_stream_readable.js:426:10)
at emitReadable (_stream_readable.js:422:5)
at readableAddChunk (_stream_readable.js:165:9)
ClojureScript:cljs.user>



 Comments   
Comment by David Nolen [ 30/Dec/14 11:22 AM ]

fixed https://github.com/clojure/clojurescript/commit/86fa7da377c94aa9bab5b7f53374d16aa9eb2298





[CLJS-522] Make REPL reflection functions synchronous Created: 16/Jun/13  Updated: 30/Dec/14  Resolved: 30/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Pending CLJS-489, make it so that any "reflection" calls (doc, macroexpand, and meta so far) print/return their results synchronously, so that the REPL prompt doesn't impinge.

(Apparently goog xhr API calls can be made synchronous easily.)



 Comments   
Comment by David Nolen [ 30/Dec/14 8:18 AM ]

deprecating the reflection namespace, REPL special fns + static vars provide most of what's desired with this.





[CLJS-925] Node.js REPL Created: 24/Dec/14  Updated: 29/Dec/14  Resolved: 29/Dec/14

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

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


 Description   

JVM based ClojureScript REPLs suffer from loading compiled cljs/core.cljs. Supporting a modern JS runtime like Node.js would make plain ClojureScript REPL interactions considerably faster.



 Comments   
Comment by David Nolen [ 24/Dec/14 11:35 AM ]

Some basic benchmarking information, for Node.js, JavaScriptCore, V8, SpiderMonkey loading the compiled cljs/core.js (including goog deps) takes 100-200ms on a 2010 Macbook Pro 2.26ghz. Under Rhino ~800ms. Under Nashorn ~8500ms.

Comment by David Nolen [ 29/Dec/14 9:00 AM ]

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





[CLJS-926] Caching support for all REPLs Created: 24/Dec/14  Updated: 29/Dec/14  Resolved: 29/Dec/14

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

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


 Description   

All ClojureScript REPLs would benefit wrt. to load time from the same caching support provided to builds.



 Comments   
Comment by David Nolen [ 29/Dec/14 9:00 AM ]

Just required threading build options through the repl support fns https://github.com/clojure/clojurescript/commit/caaf970a503eaeae5acdd269e41f4f6bbb1c215c





[CLJS-837] Cache namespace env Created: 08/Aug/14  Updated: 28/Dec/14  Resolved: 28/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Nicola Mometto
Resolution: Duplicate Votes: 1
Labels: None

Attachments: Text File 0001-add-support-for-cached-env.patch    

 Description   

See: https://github.com/clojure/tools.analyzer.js/blob/master/src/main/clojure/clojure/tools/analyzer/js.clj#L524-L548



 Comments   
Comment by Nicola Mometto [ 11/Aug/14 8:09 AM ]

I've written an initial patch based on the same approach I used for t.a.js, it's attached as 0001-add-support-for-cached-env.patch
Currently I'm seeing an OOM exception when I invoke backup-env, after loading cljs.core, which is exactly what David told me he was getting when toying with a similar patch.

At a quick glance, it seems like the analyzer's ::namespace map contains significant more fields per var map than tools.analyzer.js's one, so that's probably why backup-env works fine for t.a.js but not for cljs

Comment by Nicola Mometto [ 28/Dec/14 12:49 PM ]

This is being addressed (or has already been addressed) by David in recent commits.





[CLJS-927] :recompile-dependents build option Created: 24/Dec/14  Updated: 27/Dec/14

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

Type: Enhancement Priority: Major
Reporter: David Nolen Assignee: David Nolen
Resolution: Unresolved 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.





[CLJS-404] Automate Browser REPL testing Created: 23/Oct/12  Updated: 26/Dec/14

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

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


 Description   

It's worth investigating Selenium, PhantomJS, etc. as solutions to sanity check the Browser REPL when we run the other tests.



 Comments   
Comment by Robert Krahn [ 22/Dec/14 1:22 PM ]

An attempt: https://github.com/clojure/clojurescript/pull/42

Comment by David Nolen [ 24/Dec/14 8:57 AM ]

This looks like an interesting patch, thanks!

Comment by Robert Krahn [ 26/Dec/14 10:57 AM ]

I'll post a patch here, first I'll investigate the load-file issue, though.





[CLJS-928] Add cljs.test/are macro Created: 24/Dec/14  Updated: 26/Dec/14  Resolved: 24/Dec/14

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

Type: Enhancement Priority: Major
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

cljs.test is lacking "are" macro. It can be copied as-is from clojure.test



 Comments   
Comment by Alex Dowad [ 26/Dec/14 3:42 AM ]

What is this ticket a duplicate of?

Comment by Nikita Prokopov [ 26/Dec/14 4:20 AM ]

Alex, https://github.com/clojure/clojurescript/commit/6023cd14bd642e31b9c3ea5ace14e258ab0950f2





[CLJS-889] 2 characters not supported in re-pattern: \u2028 \u2029 Created: 18/Nov/14  Updated: 26/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Dave Sann Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

[org.clojure/clojurescript "0.0-2371"]
ubuntu


Attachments: Text File 0001-re-pattern-works-on-strings-containing-u2028-or-u202.patch     Text File 889.patch    

 Description   

The following 2 patterns

(re-pattern "[\u2028]")
(re-pattern "[\u2029]")

Cause:

SyntaxError: Invalid regular expression: missing terminating ] for character class

They are probably somewhat rare characters in practice.

http://www.fileformat.info/info/unicode/char/2028/index.htm
http://www.fileformat.info/info/unicode/char/2029/index.htm



 Comments   
Comment by Alex Dowad [ 18/Dec/14 1:51 PM ]

The problem is that re-pattern uses a (.*) regexp to pick out the "pattern" portion of a regexp string (separate from the "flags" portion), and "." doesn't match \u2028 and \u2029.

Comment by Alex Dowad [ 18/Dec/14 2:15 PM ]

Sorry, JIRA's helpful formatting mangled that patch. Here it is again.

Comment by Alex Dowad [ 26/Dec/14 1:31 AM ]

Here is a fixed version of the patch, tested on V8, Nashorn, and Spidermonkey. It includes a regression test.





[CLJS-897] AOT core.cljs Created: 02/Dec/14  Updated: 24/Dec/14

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


 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.





[CLJS-899] AOT cache core.cljs analysis Created: 02/Dec/14  Updated: 24/Dec/14

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





[CLJS-918] Analysis caching discard metadata Created: 22/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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 [ 24/Dec/14 10:53 AM ]

We only want to preserve :arglists metadata. Fixed https://github.com/clojure/clojurescript/commit/fdcf333aadd8728e93e1044040b90d67e17b7f57





[CLJS-924] Better error message for mistaken use of 'def' Created: 24/Dec/14  Updated: 24/Dec/14

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

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

Attachments: Text File 0001-Better-error-message-if-def-is-mistakenly-substitute.patch    
Patch: Code

 Description   

ClojureScript shares what is IMHO one of the biggest weaknesses of the current JVM Clojure implementation: those (in)famous stack traces going down into the innards of the compiler if you get your syntax wrong. An easy mistake for noobs is to use 'def' in place of 'defn'; this patch makes that mistake a lot less painful to debug.

Feedback please!






[CLJS-917] invalid "all arguments must be numbers" warning Created: 22/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

With CLJS build 2498, I get this:

WARNING: cljs.core//, all arguments must be numbers, got #{nil clj-nil} number instead. at line 51 resources/public/cljs/out/cljs_time/coerce.cljs

For this code:

(defn to-epoch
  "Convert `obj` to Unix epoch."
  [obj]
  (let [millis (to-long obj)]
    (and millis (/ millis 1000))))        ; this is line 51

From cljs-time v0.2.4: https://github.com/andrewmcveigh/cljs-time



 Comments   
Comment by David Nolen [ 24/Dec/14 9:01 AM ]

Should be fixed by CLJS-907, the problem was to-long had a method call which was not type inferred prior to CLJS-907 patch.





[CLJS-907] False positives from arithmetic checks Created: 12/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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   
WARNING: cljs.core/>=, all arguments must be numbers, got [#{nil clj-nil} number] instead. at line 59 file:/Users/kimmoko/.m2/repository/prismatic/dommy/1.0.0/dommy-1.0.0.jar!/dommy/core.cljs

Line 59 from core.cljs is the body of when-let:

        (when-let [i (utils/class-index class-name c)]
          (>= i 0))))))
(defn class-index
  "Finds the index of class in a space-delimited class-name
    only will be used when Element::classList doesn't exist"
  [class-name class]
  (loop [start-from 0]
    (let [i (.indexOf class-name class start-from)]
      (when (>= i 0)
        (if (class-match? class-name class i)
          i
          (recur (+ i (.-length class))))))))


 Comments   
Comment by David Nolen [ 12/Dec/14 10:06 AM ]

The problem is that the inferred type of class-index doesn't include number.

Comment by David Nolen [ 24/Dec/14 9:00 AM ]

fixed https://github.com/clojure/clojurescript/commit/576b85d83bf0eb0e232db3b2b8bddee0405f29d9





[CLJS-896] Investigate AOTed ClojureScript JAR Created: 02/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

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


 Comments   
Comment by David Nolen [ 24/Dec/14 8:43 AM ]

Probably best if this is handled at the level of tooling.





[CLJS-898] Cache analysis results to disk Created: 02/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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


 Comments   
Comment by David Nolen [ 24/Dec/14 8:41 AM ]

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





[CLJS-919] compare-and-set! relies on Atom record structure instead of protocols Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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: Completed Votes: 0
Labels: atom, protocols

Attachments: Text File cljs-919-generic-cas.patch    

 Description   

Here CAS uses .-state property, thus binding itself to Atoms only:

(defn compare-and-set! [a oldval newval]
  (if (= (.-state a) oldval)
    (do (reset! a newval) true)
    false))

We can replace it with -deref call instead, opening it up for alternative implementations:

(defn compare-and-set! [a oldval newval]
  (if (= (-deref a) oldval)
    (do (reset! a newval) true)
    false))


 Comments   
Comment by David Nolen [ 24/Dec/14 8:39 AM ]

thanks fixed https://github.com/clojure/clojurescript/commit/4ee89df427644c096d32117194ae1d76f8dec836





[CLJS-920] add-watch/remove-watch should return reference, as in Clojure Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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: Completed Votes: 0
Labels: None

Attachments: Text File cljs-920-add-remove-watch-return-iref.patch    

 Description   

For now, Atom/-add-watch returns atom, Atom/-remove-watch returns (.-watchers atom). Implementers of IWatchable should be careful to return this, otherwise they may incidentally broke this convention.

Attached patch changes wrapper functions add-watch/remove-watch to always return iref passed in, ignoring what -add-watch/-remove-watch implementations return.



 Comments   
Comment by David Nolen [ 24/Dec/14 8:09 AM ]

Thanks! In the future it would be nice to get the accompanying test with a patch like this.

Comment by David Nolen [ 24/Dec/14 8:20 AM ]

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





[CLJS-921] cljs.repl/doc output includes namespace twice Created: 23/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

r2511



 Description   

The first line of cljs.repl/doc output is the fully-qualified name of the var, and in this output the namespace is repeated.

Here is an example. Note that cljs.core is repeated:

(cljs.repl/doc str)
-------------------------
cljs.core/cljs.core/str
([] [x] [x & ys])
  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.

This appears to be rooted in the fact that the :name meta contains the namespace (in contrast with Clojure, which this is not the case).

(meta (var str))
=> {:arglists ([] [x] [x & ys]), :test nil, :column 1, :line 2134, :file ".repl/cljs/core.cljs", :doc "With no args, returns the empty string. With one arg x, returns\n  x.toString().  (str nil) returns the empty string. With more than\n  one arg, returns the concatenation of the str values of the args.", :name cljs.core/str, :ns cljs.core}


 Comments   
Comment by David Nolen [ 24/Dec/14 8:02 AM ]

fixed https://github.com/clojure/clojurescript/commit/88b3d1e54ba43c595e11aeab226cfd071ca6f657





[CLJS-923] Inconsistent warning defaults Created: 24/Dec/14  Updated: 24/Dec/14  Resolved: 24/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File inconsitent-warning-defaults.diff    
Patch: Code

 Description   

The cljs compiler option :warnings is initialized with inconsistent defaults [1]

(def ^:dynamic *cljs-warnings*
  {:preamble-missing true
   :unprovided true
   :undeclared-var false
   :undeclared-ns false
   :undeclared-ns-form true
   :redef true
   :dynamic true
   :fn-var true
   :fn-arity true
   :fn-deprecated true
   :protocol-deprecated true
   :undeclared-protocol-symbol true
   :invalid-protocol-symbol true
   :multiple-variadic-overloads true
   :variadic-max-arity true
   :overload-arity true
   :extending-base-js-type true
   :invoke-ctor true
   :invalid-arithmetic true
   :protocol-invalid-method true
   :protocol-duped-method true
   :protocol-multiple-impls true})

If :warnings is not set

(opts :warnings true)
will enable
[:unprovided :undeclared-var :undeclared-ns :undeclared-ns-form]
, if :warnings is false it will disable them. [2]

If warnings is given a map like {:invalid-arithmetic false} it will also disable (or leave disabled) the :undeclared-var and :undeclared-ns which would otherwise be enabled by the previous logic. Having the effect of disabled 3 warnings instead of 1.

The patch only changes the defaults, although at some point the warning logic in cljs.closure/build should probably be cleaned up a little.

Why does {:warnings false} only disable 4 warnings? [2]

[1] https://github.com/clojure/clojurescript/blob/d3da2349c175a9066f4d9e4476302ff497f51937/src/clj/cljs/analyzer.clj#L40-L62
[2] https://github.com/clojure/clojurescript/blob/d3da2349c175a9066f4d9e4476302ff497f51937/src/clj/cljs/closure.clj#L981-L991



 Comments   
Comment by Thomas Heller [ 24/Dec/14 7:25 AM ]

Shouldn't paste from the patched version.

Comment by David Nolen [ 24/Dec/14 7:51 AM ]

thanks! fixed https://github.com/clojure/clojurescript/commit/b9e475698426b8bb8862ef32706b925bd6d691cd





[CLJS-912] Minor enhancements to bootstrap script Created: 18/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Dowad Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Check-dependencies-in-bootstrap-script.patch     Text File 0001-Enhancements-to-bootstrap-script.patch     Text File 0002-Display-status-message-if-download-fails-while-boots.patch     Text File 0003-Retry-failed-downloads-in-bootstrap-script-before-gi.patch    
Patch: Code

 Description   

Just some small tweaks to make things a bit friendlier for first-time users. See the attached *.patch files.

I would like to remove the -s flag from invocations of curl as well, at least for the larger downloads. I like feedback about what a script is doing, while it's doing it. What do you think? Silence is golden?



 Comments   
Comment by Alex Dowad [ 18/Dec/14 1:29 PM ]

Just filled in and signed the Clojure CA.

Comment by David Nolen [ 18/Dec/14 1:39 PM ]

Great! Can we please get a single squashed patch? Thanks!

Comment by Alex Dowad [ 18/Dec/14 2:21 PM ]

OK, here is one squashed commit. I would have thought that you would like "semantic commits" – one commit for each conceptual change. I guess this stuff is too trivial to litter the history with a lot of fine-grained commits.

Comment by David Nolen [ 20/Dec/14 3:57 PM ]

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





[CLJS-914] thrown-with-msg? is unable to get message of exception Created: 19/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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

Type: Defect Priority: Major
Reporter: Matthew Boston Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 914.patch    

 Description   

In JavaScript, the message of an Error is retrieved with the Error.prototype.message property but the implementation of cljs.test/thrown-with-msg? uses the method .getMessage. This is causing the following error when writing tests with thrown-with-msg?.

Test console: expected: (thrown-with-msg? ArithmeticException #"Divide by zero" (\ 1 0))
Test console: actual:
Test console: #<TypeError: 'undefined' is not a function (evaluating 'e_5184auto_.getMessage()')>



 Comments   
Comment by Matthew Boston [ 19/Dec/14 5:31 PM ]

patch file for CLJS-914

Comment by David Nolen [ 20/Dec/14 3:35 PM ]

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





[CLJS-915] On empty call, List and PersistentQueue do not retain meta, sorted-set/sorted map do not retain comparator Created: 19/Dec/14  Updated: 20/Dec/14  Resolved: 20/Dec/14

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: Completed Votes: 0
Labels: collections

Attachments: Text File cljs-915-fix-empty-on-collections.patch    

 Description   

-empty method is implemented incorrectly at List, PersistentQueue, PersistentSortedSet, PersistentSortedMap

(-> (sorted-set-by (comp - compare))
    empty
    (into [2 3 1])) => #{1 2 3} (should be #{3 2 1})
(-> (sorted-map-by (comp - compare))
    empty
    (into [[2 :b] [3 :c] [1 :a]])) => {1 :a, 2 :b, 3 :c} (should be backwards)
(meta (empty '^{:a 1} (1 2 3))) => nil (should be {:a 1})
(meta (empty (with-meta (.-EMPTY PersistentQueue) {:b :c}))) => nil (should be {:b :c})

Patch with tests attached: cljs-915-fix-empty-on-collections.patch



 Comments   
Comment by David Nolen [ 20/Dec/14 3:27 PM ]

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





[CLJS-913] Error when trying to convert javascript object to clojure (js->clj obj) under :advanced compilation Created: 18/Dec/14  Updated: 18/Dec/14

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

Type: Defect Priority: Major
Reporter: Erik Wickstrom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

ClojureScript 0.0-2496

Tested on OSX 10.10 with Chrome 39



 Description   

I'm trying to convert a javascript object (parsed JSON from a web service) into a clojure data structure. It works fine if I use :simple optimization with the closure compiler, however when I switch to :advanced optimization, I get the following error:

(let my-data #js {} ; see below for JSON
(.info js/console "converted to clojure" (str (js->clj my-data))))

Uncaught Error: No protocol method IEmptyableCollection.-empty defined for type object: [object Object]

Note that this also only seems to happen with this chunk of JSON and a few other samples (though according to all the JSON linters I've tried, it is valid). I've passed other input through without issue. So it is somewhat intermittent but deepish object hierarchies seem to be a commonality.

Here is the JSON (also posted here http://pastebin.com/PLffFrFf )

[{"address_components":[{"long_name":"11810","short_name":"11810","types":["postal_code"]},{"long_name":"16 de Septiembre","short_name":"16 de Septiembre","types":["neighborhood","political"]},{"long_name":"Miguel Hidalgo","short_name":"Miguel Hidalgo","types":["sublocality_level_1","sublocality","political"]},{"long_name":"Ciudad de México","short_name":"México D.F.","types":["locality","political"]},{"long_name":"Distrito Federal","short_name":"D.F.","types":["administrative_area_level_1","political"]},{"long_name":"Mexico","short_name":"MX","types":["country","political"]}],"formatted_address":"16 de Septiembre, Miguel Hidalgo, 11810 Ciudad de México, D.F., Mexico","geometry":{"bounds":{"Ea":{"j":19.4043293,"k":19.3997335},"wa":{"j":-99.21262619999999,"k":-99.2045263}},"location":{"D":-99.20755880000002,"k":19.402037},"location_type":"APPROXIMATE","viewport":{"Ea":{"j":19.4043293,"k":19.3997335},"wa":{"j":-99.21262619999999,"k":-99.2045263}}},"types":["postal_code"]},{"address_components":[{"long_name":"11810","short_name":"11810","types":["postal_code"]},{"long_name":"West Jakarta","short_name":"West Jakarta","types":["locality","political"]},{"long_name":"Kamal","short_name":"Kamal","types":["administrative_area_level_4","political"]},{"long_name":"Kalideres","short_name":"Kalideres","types":["administrative_area_level_3","political"]},{"long_name":"West Jakarta City","short_name":"West Jakarta City","types":["administrative_area_level_2","political"]},{"long_name":"Jakarta","short_name":"Jakarta","types":["administrative_area_level_1","political"]},{"long_name":"Indonesia","short_name":"ID","types":["country","political"]}],"formatted_address":"Kamal, Kalideres, West Jakarta City, Jakarta 11810, Indonesia","geometry":{"bounds":{"Ea":{"j":-6.095065399999999,"k":-6.110835},"wa":{"j":106.68747699999994,"k":106.71448510000005}},"location":{"D":106.70282500000008,"k":-6.101219},"location_type":"APPROXIMATE","viewport":{"Ea":{"j":-6.095065399999999,"k":-6.110835},"wa":{"j":106.68747699999994,"k":106.71448510000005}}},"types":["postal_code"]},{"address_components":[{"long_name":"11810","short_name":"11810","types":["route"]},{"long_name":"Příbram District","short_name":"Příbram District","types":["administrative_area_level_2","political"]},{"long_name":"Central Bohemian Region","short_name":"Central Bohemian Region","types":["administrative_area_level_1","political"]},{"long_name":"Czech Republic","short_name":"CZ","types":["country","political"]},{"long_name":"261 01","short_name":"261 01","types":["postal_code"]}],"formatted_address":"11810, 261 01, Czech Republic","geometry":{"bounds":{"Ea":{"j":49.7328257,"k":49.7102303},"wa":{"j":13.979755599999976,"k":13.986990699999978}},"location":{"D":13.982032200000049,"k":49.7225575},"location_type":"GEOMETRIC_CENTER","viewport":{"Ea":{"j":49.7328257,"k":49.7102303},"wa":{"j":13.979755599999976,"k":13.986990699999978}}},"types":["route"]}]

Here is the original post to the mailing list: https://groups.google.com/forum/#!topic/clojurescript/MZ9esXbn2tg






[CLJS-806] support ^:const Created: 09/May/14  Updated: 17/Dec/14

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

Attachments: Text File cljs_806.patch    
Patch: Code and Test

 Description   

Currently def's do not support ^:const annotations, this is useful in conjunction with case.



 Comments   
Comment by Peter Schuck [ 17/Dec/14 4:53 PM ]

def's marked with ^:const are now treated as constants, they can't be redefined or set to a different value. Currently only case takes advantage of this. Additionally the emitted var is annotated as a constant for the closure compiler.





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

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






[CLJS-651] optimize true branch of satisfies? usage Created: 01/Nov/13  Updated: 16/Dec/14

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

Attachments: Text File cljs_651.patch    
Patch: Code

 Description   

The true branch of a satisfies? test should be hinted so that the type doesn't need type hints



 Comments   
Comment by Peter Schuck [ 16/Dec/14 2:51 PM ]

All paths taken on satisfies are now hinted as boolean





[CLJS-908] Duplicate goog.require emit when using :refer Created: 14/Dec/14  Updated: 16/Dec/14  Resolved: 16/Dec/14

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

Type: Defect Priority: Trivial
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File duplicate-require-emit.diff    
Patch: Code

 Description   

In a CLJS (ns ...) a (:require [some-ns :refer (something)]) will end up with two goog.require("some_ns") statements in the generated .js. This also used to happen with (:require [clojure.string :as str]) but was fixed some time ago. The attached page should fix it for all cases.



 Comments   
Comment by David Nolen [ 16/Dec/14 12:22 PM ]

Thanks!

Fixed
https://github.com/clojure/clojurescript/commit/ec0023bd45a7f956b1e58aab84cb207a94e35965





[CLJS-696] remove arguments usage from defrecord constructor Created: 23/Nov/13  Updated: 16/Dec/14  Resolved: 16/Dec/14

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

Attachments: Text File cljs_696_v1.patch    
Patch: Code

 Description   

There is no need for the arguments usage in the defrecord constructor and it's a perf hit for construction. We should always construct defrecords by passing in the extra three arguments: __extmap, __meta, and hash automatically.



 Comments   
Comment by Peter Schuck [ 16/Dec/14 11:59 AM ]

The constructor now has __extmap, __meta, and __hash in all the places it's constructor is called, the positional factory, map factory, and direct constructor invocation. This is the first time going deep into the ClojureScript compiler so there may be some clean up to do or other places a records constructor is called that I didn't take care of.

Comment by David Nolen [ 16/Dec/14 12:20 PM ]

Excellent work! Thanks! https://github.com/clojure/clojurescript/commit/491dd1bb6ba446407298d6fb93dd6cbd578d3b76





[CLJS-909] Add stable api for consumers of compiler data. Created: 15/Dec/14  Updated: 16/Dec/14  Resolved: 16/Dec/14

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

Type: Enhancement Priority: Minor
Reporter: Bruce Hauman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJS_909.patch    

 Description   

The ClojureScript compiler is under very active development. It would be nice for consumers of internal compiler data to have a stable api.



 Comments   
Comment by Bruce Hauman [ 15/Dec/14 2:50 PM ]

Here's the patch

Comment by David Nolen [ 16/Dec/14 12:10 PM ]

fixed https://github.com/clojure/clojurescript/commit/163f079407d1310755d326884b6965d9d74ed3c5





[CLJS-905] Dependency tree fail Created: 11/Dec/14  Updated: 14/Dec/14  Resolved: 12/Dec/14

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

Type: Defect Priority: Major
Reporter: Dusan Maliarik Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug, compiler


 Description   

In the following repo

https://github.com/skrat/deporder-bug-cljs

Looking at the compiled source, I would expect the `bug.b` module to be initialized before `bug.a`. It is not, and it leads to a runtime error about `bug.b/registry` being undefined. Now, when I switch things around (`around` branch in the repo), rename `bug.a` to `bug.b` and vice versa, I will get a compiler warning about `bug.a/registry` being undeclared, but at runtime everything works as expected.

This example was extracted from a large project that uses similar macro that expands to `swap!`, where with `none` optimizations everything works, but with `simple` and `advanced` it throws during runtime.



 Comments   
Comment by Thomas Heller [ 11/Dec/14 6:38 AM ]

Technically not a "bug" but a weakness due to the nature ClojureScript handles macros.

'bug.macros requires 'bug.b cause it emits code calling it. 'bug.a requires 'bug.macro and therefore technically requires 'bug.b but since it cannot figure this out you technically have to tell it by also doing a (:require [bug.b]) in 'bug.a.

I had this problem a while back [1] and implemented a workarround by letting the macro declare what namespaces it requires, which in your case would look like

(def macro
  ^{:js-requires '[bug.b]}
  zwap [v]
  `(swap! bug.b/registry conj ~v))

But I was turned down so the feature never made it.

The recommended way is to have macros in namespaces that have a matching CLJS namespace and using :include-macros/:refer-macros when requiring them. Has its own issues cause a) everyone needs to know that bug.b uses macros and b) everyone needs to know what actually is a macro.

Anyways, in your case move bug/macros.clj to bug/b.clj and rewrite bug/a.cljs to

(ns bug.a
  (:require [bug.b :refer-macros (zwap)]))

[1] https://groups.google.com/forum/?fromgroups#!searchin/clojurescript/macro$20require/clojurescript/sLRGCIa8E1c/N9sFcTP_i9wJ

Comment by Thomas Heller [ 11/Dec/14 6:49 AM ]

Shameless plug: shadow-build [1] supports the macro meta feature. Still sort of hack-ish since it only works when macros are referred directly but my use of macros is very limited, basically only whats in [2].

[1] https://github.com/thheller/shadow-build
[2] https://github.com/thheller/shadow/blob/master/src/clj/shadow/macros.clj

Comment by Dusan Maliarik [ 11/Dec/14 8:33 AM ]

I wonder how this works in Clojure land, but I would still say it's a bug in ClojureScript compiler, because, afaik, it first expands macros, which results in a symbol from another namespace ('bug.b), and at that point, 'bug.b should be added as a dependency, resulting in goog.require('bug.b') in the compiled 'bug.a, isn't that right?

Comment by Francis Avila [ 11/Dec/14 9:18 AM ]

In java Clojure you still need to ensure that the symbols you expand in your macros are resolvable by the time the expanded form is executed. For example, if you create a macro that includes a clojure.data/diff call, you need to require clojure.data somewhere in your program, ideally in the namespace that defines the macro. Symbols which refer to other namespaces do not automatically require those namespaces. Most likely you will get a {{ClassNotFoundException [namespace-part]}} when you try to execute the form.

However Clojurescript introduces the extra problem that the macro expansion and execution environments are different. (In Clojure they are the same.) So there are additional constraints not present in Clojure:

1. A separate, completely different namespace dependency tree exists at compile time vs runtime.
2. Because of the Google Closure compiler, the entire runtime dependency tree must be known statically at compile time. (Otherwise some code you need at runtime might not be compiled into the final js.)
3. Because of javascript's dynamism, some symbols may not be known even to the Google Closure compiler. For example, if you have a macro which expands to calls to jQuery code, neither clojure, clojurescript, nor google closure, nor even your browser have a mechanism to "require" jQuery. The code must simply trust that you have done whatever is necessary to ensure jQuery/whatever is resolvable when it finally executes.

Additionally, automatically requiring namespaces from macro expansion is