<< Back to previous view

[CLJS-993] binding macro returns non-nil with empty body Created: 30/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

Type: Defect Priority: Minor
Reporter: Stefano Pugnetti Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: binding, test
Environment:

Ubuntu Linux 12.04
Clojurescript 0.0-2740



 Description   

In Clojure the binding macro returns nil when called with an empty body. In Clojurescript it returns a non empty object.

In this repository
https://github.com/stepugnetti/binding-issue
I've put a simple project in which the two behaviors are compared. The same code

(defn foo []
  (if (= nil (binding [*test-var* 1]))
    (.log js/console "Ok!")
    (.log js/console "Too bad!")))

can be run with
1) Clojure: lein run
2) Clojurescript: lein cljsbuild once main && node target/binding-issue.js

I was expecting the same result (nil)...

See discussion at https://groups.google.com/forum/#!topic/clojurescript/anbDq9pjvEs



 Comments   
Comment by David Nolen [ 30/Jan/15 5:47 PM ]

fixed https://github.com/clojure/clojurescript/commit/996f33e5250712072eaefb5eff13bb9372d5e1b6





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

fixed https://github.com/clojure/clojurescript/commit/87a14e1b8d253e28c2647c98ea91a42cd24a9972





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

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

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

Attachments: Text File CLJS-976.patch    

 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.



 Comments   
Comment by Mike Fikes [ 29/Jan/15 9:59 PM ]

Hey Joel, this appears to be rooted in the Node process itself dying.

I reproduced using your initial steps and confirmed that my Node process disappears. You can produce the same java.lang.IllegalArgumentException: Value out of range for char: -1 if you simply kill the node process out from underneath the REPL, or do something else that will cause it to give up the ghost (I ran into it having it evaluate (set (range 20000000)))

If you try to issue a subsequent command you will then get a java.net.SocketException: Broken pipe when the attempt is made to write the JavaScript out to the Node process on the (defunct) socket.

The IllegalArgumentException associated with the failure to handle -1 being returned from the socket read could be converted to an IOException, perhaps giving the user more of a clue:

diff --git a/src/clj/cljs/repl/node.clj b/src/clj/cljs/repl/node.clj
index 563d63a..9ea81db 100644
--- a/src/clj/cljs/repl/node.clj
+++ b/src/clj/cljs/repl/node.clj
@@ -16,7 +16,7 @@
             [clojure.data.json :as json])
   (:import java.net.Socket
            java.lang.StringBuilder
-           [java.io File BufferedReader BufferedWriter]
+           [java.io File BufferedReader BufferedWriter EOFException]
            [java.lang ProcessBuilder ProcessBuilder$Redirect]))
 
 (defn socket [host port]
@@ -39,6 +39,7 @@
   (let [sb (StringBuilder.)]
     (loop [sb sb c (.read in)]
       (cond
+       (= c -1) (throw (EOFException.))
        (= c 1) (let [ret (str sb)]
                  (print ret)
                  (recur (StringBuilder.) (.read in)))

But perhaps an even better solution would be to, in addition, add a try-catch in node-eval to handle all IOException s encountered when attempting to communicate with the Node process and to simply print a suitable meaningful error to the user stating such. (I suppose you can't necessarily conclude that the Node process is actually gone; it may be a timeout that leads to an IOException). But this would arguably be more robust than derailing trying to cast -1 to a char.

Comment by Mike Fikes [ 30/Jan/15 8:42 AM ]

The attached patch adds robustness by handling errors reading and writing to the underlying Node process and by additionally detecting if Node has died.

For Joel's example, it doesn't fail fast, but when you attempt a subsequent evaluation, it does provide diagnostics:

ClojureScript:cljs.user> (+ 1 1)
IOException communicating with Node process: Connection closed
Node process has exited with value: 1
nil

and with the example I encountered (in a fresh REPL):

ClojureScript:cljs.user> (set (range 20000000))
FATAL ERROR: CALL_AND_RETRY_2 Allocation failed - process out of memory
IOException communicating with Node process: Connection closed
Node process has exited with value: 134
nil

(This last example is on OS X. Note that 134 - 128 = 6, which is SIGABRT.)

Comment by David Nolen [ 30/Jan/15 3:01 PM ]

The correct solution is to prevent the Node.js process from ever dying from uncaught exceptions.

Fixed in master https://github.com/clojure/clojurescript/commit/2d81bde0a7e1fbcf1f4883cb144bf2b10cea393a

Comment by Mike Fikes [ 30/Jan/15 3:17 PM ]

Confirmed fixed for Joel's example, and for mine, the outcome is also still reasonable given that the out-of-memory condition is displayed:

ClojureScript:cljs.user> (set (range 20000000))
FATAL ERROR: CALL_AND_RETRY_2 Allocation failed - process out of memory
java.lang.IllegalArgumentException: Value out of range for char: -1
	at clojure.lang.RT.charCast(RT.java:962)
	at cljs.repl.node$read_response.invoke(node.clj:47)
	at cljs.repl.node$node_eval.invoke(node.clj:57)
	at cljs.repl.node.NodeEnv._evaluate(node.clj:164)
	at cljs.repl$evaluate_form.invoke(repl.clj:206)
	at cljs.repl$evaluate_form.invoke(repl.clj:168)
	at cljs.repl$eval_and_print.invoke(repl.clj:263)
	at cljs.repl$repl_STAR_.invoke(repl.clj:427)
	at user$eval3562.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)
java.lang.IllegalArgumentException: Value out of range for char: -1
nil




[CLJS-965] Make :foreign-libs more useful Created: 07/Jan/15  Updated: 30/Jan/15  Resolved: 30/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: 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?

Comment by David Nolen [ 30/Jan/15 3:13 PM ]

fixed in 0.0-2719





[CLJS-961] REPL process should always be killable Created: 04/Jan/15  Updated: 30/Jan/15  Resolved: 30/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: Duplicate Votes: 0
Labels: None


 Description   

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



 Comments   
Comment by David Nolen [ 30/Jan/15 3:12 PM ]

Duplicate CLJS-989





[CLJS-986] Add :target to the list of build options that should trigger recompilation Created: 21/Jan/15  Updated: 30/Jan/15  Resolved: 30/Jan/15

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

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


 Comments   
Comment by David Nolen [ 30/Jan/15 3:11 PM ]

fixed https://github.com/clojure/clojurescript/commit/1611bdc7fa7ef21ed2e8543afaefd81b516bacec





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

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

Type: Defect Priority: Major
Reporter: Richard Davies Assignee: Unassigned
Resolution: Not Reproducible 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.

Comment by David Nolen [ 30/Jan/15 3:07 PM ]

Cannot reproduce. Please do not reopen this ticket until a minimal case can be supplied thanks.





[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





Generated at Sat Jan 31 01:13:00 CST 2015 using JIRA 4.4#649-r158309.