<< Back to previous view

[CLJS-1226] Add on-testing-complete hook Created: 27/Apr/15  Updated: 13/May/15  Resolved: 12/May/15

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

Type: Enhancement Priority: Minor
Reporter: Sebastian Bensusan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, enhancement, test

Attachments: Text File cljs_1226.patch     Text File cls_1226_v2.patch    
Patch: Code

 Description   

When a test runner runs async tests created with cljs.test/async there is no reliable way to return the control from the async code in the test suite to the test runner. This is problematic since the test script might need the tests results to proceed or terminate.

A function to be called after all tests are done is proposed: cljs.test/*on-testing-complete-fn* and it would take the test summary as its only argument

It can be set by the user by calling cljs.test/set-on-testing-complete! which should be callable from JS (^:export)

Notes:
In the patch, the function cljs.test/successful? also has the ^:export metadata to be called from JS test runners.
The code was tested manually with V8, Spidermonkey, Nashorn, SlimerJs, and PhantomJS but not with JavaScript Core.



 Comments   
Comment by Jenan Wise [ 01/May/15 2:40 PM ]

Rather than relying on a global, what about if run-tests took a callback? It would differentiate the API from clojure.test but they are already different due to the env arg and nil return value.

Comment by Sebastian Bensusan [ 01/May/15 2:59 PM ]

It is currently in a global to allow easy access from outside cljs (js runners). By having it to pass it to run-tests it would be harder to make reusable test runners from outside cljs/ Said runners would have to know both the ns to run (TBD in each project) and the callback (what we want to reuse, i.e. phantom.exit(0))

Comment by David Nolen [ 05/May/15 7:02 AM ]

Is there any particular reason to not invoke do-report with a suitable dispatch value instead of adding a new function here?

Comment by Sebastian Bensusan [ 05/May/15 7:29 AM ]

I'm not sure that I understand. I thought do-report and report were meant for printing out the summary as a report. This on-testing-complete hook should allow the caller to do whatever he/she wants after all tests are done. Do you want to allow the caller to specify how report works for some new dispatch value by adding:

(defmethod report [::default :on-testing-complete] [m]
..custom-code)

Comment by David Nolen [ 05/May/15 7:32 AM ]

The purpose of report is just side effects, printing is only one possibility. Note there are several testing events that aren't even really used by the standard reporter.

Comment by Leon Grapenthin [ 05/May/15 12:37 PM ]

IMHO if you want to run anything synchronously before after or between tests, you should use fixtures or if necessary compose the desired blocks yourself.

At least making the latter possible is why I made run-block and the block builder fns public.

You can easily run

(run-block (concat (run-tests-block 'a-ns-to-be-tested 'another-ns-to-be-test )
[(fn []
;; your continuation code here
)]))

This is a reliable way to pick up control after testing.

This patch only affects testing done via run-tests. E. g. on-testing-complete-fn won't be invoked if a user invoked test-ns and testing is complete. You could hack it into the block composer in test-ns, but still users composing blocks in the way mentioned above couldn't rely on it being invoked.

Comment by Sebastian Bensusan [ 11/May/15 10:25 AM ]

I followed Leon's suggestions and used run-block to make my own test-block. The only problem is that my continuation function doesn't get the summary/env as an argument. From what I understood I could pass env as an argument to run-tests but it would get cleared right before my continuation function. I don't know how to work around this yet.

If you think my solution is a correct use of the cljs.test API then this patch isn't needed. If you believe that run-tests should report on-testing-done I'd be happy to submit a new patch adding a new event to the report event system. Then I could get the summary/env from a custom report method as David suggested.

Comment by David Nolen [ 11/May/15 10:37 AM ]

I would not consider anything at the level of run-block part of the public API nor anything that anyone should be looking into. This stuff may change.

Please add a real reporting hook thanks.

Comment by Sebastian Bensusan [ 12/May/15 5:39 AM ]

Added :end-run-test event to cljs.test and a dummy event handler for it.

Comment by David Nolen [ 12/May/15 3:04 PM ]

fixed https://github.com/clojure/clojurescript/commit/31736450c6ca951c63bb685d744c11074fbfe323

Comment by Leon Grapenthin [ 12/May/15 4:09 PM ]

For API consistency this should also be in the blocks of `test-all-vars` in `test-ns`, `test-var` and `test-vars`.

Comment by David Nolen [ 12/May/15 4:24 PM ]

Happy to see a new ticket + patch that addresses the consistency issue.

Comment by Sebastian Bensusan [ 13/May/15 2:51 AM ]

Leon, I filled CLJS-1267 to address your point. Feel free to add any other API inconsistencies that :end-run-tests introduced.





[CLJS-1191] Update clojure.walk to the current version on clojure Created: 06/Apr/15  Updated: 10/May/15

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

Type: Defect Priority: Minor
Reporter: Stuart Mitchell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, performance

Attachments: Text File 50.patch     Text File CLJS-1191.patch    
Patch: Code

 Description   

Currently clojure.walk can not handle records

It is using an old version of clojure.walk and clojure has improved the implementation because of

http://dev.clojure.org/jira/browse/CLJ-1105


src/cljs/clojure/walk.cljs | 4 ++++
1 file changed, 4 insertions

diff --git a/src/cljs/clojure/walk.cljs b/src/cljs/clojure/walk.cljs
index f2ebd8d..541ecea 100644
— a/src/cljs/clojure/walk.cljs
+++ b/src/cljs/clojure/walk.cljs
@@ -43,7 +43,11 @@ the sorting function."}
{:added "1.1"}
[inner outer form]
(cond
+ (list? form) (outer (apply list (map inner form)))
+ (satisfies? IMapEntry form) (outer (vec (map inner form)))
(seq? form) (outer (doall (map inner form)))
+ (satisfies? IRecord form)
+ (outer (reduce (fn [r x] (conj r (inner x))) form form))
(coll? form) (outer (into (empty form) (map inner form)))
:else (outer form)))



 Comments   
Comment by David Nolen [ 07/Apr/15 5:56 AM ]

Please attach the patch as a file. Thanks!

Comment by Stuart Mitchell [ 07/Apr/15 7:18 PM ]

I think this one works

it is a mail formatted patch

Comment by David Nolen [ 08/Apr/15 6:07 AM ]

Please follow the patch conventions described here https://github.com/clojure/clojurescript/wiki/Patches. Thank you.

Comment by David Nolen [ 12/Apr/15 4:53 PM ]

Stuart, I don't see you on the list of contributors. Please submit a CA so I can apply the patch. Thanks!

Comment by Stuart Mitchell [ 13/Apr/15 7:49 PM ]

Hopefully this is the right format

Comment by Stuart Mitchell [ 13/Apr/15 7:50 PM ]

Contributor agreement signed as well

Comment by Sebastian Bensusan [ 01/May/15 1:24 PM ]

Tested the patch on the REPL, works as expected except for walking fns that modify the keys:

> (defrecord Foo [name])
cljs.user/Foo
> (w/prewalk #(if (keyword? %) (str %) %) (Foo. "foo"))
#cljs.user.Foo{:name "foo", ":name" "foo"}

It is not consistent with walking the same fn over a map:

> (w/prewalk #(if (keyword? %) (str %) %) {:name "foo"})

{":name" "foo"}

This behavior was noted in the original Clojure patch as well: http://dev.clojure.org/jira/browse/CLJ-1239 and might be undesirable since it surprises the user.





[CLJS-1153] Typed Array backed PersistentVector based on clojure.core/Vec Created: 19/Mar/15  Updated: 29/Apr/15

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

Type: Enhancement Priority: Minor
Reporter: Adrian Medina Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: Text File 1153.patch    
Patch: Code

 Description   

Goal is to add support for vectors based on clojure.core/Vec, built on top of JavaScript Typed Arrays.

My hope is that this would allow for both efficient creation of vectors from existing Typed Arrays without intermediate conversion to normal JavaScript arrays, as well as efficient concatenation of the composite arrays of the vector back into a Typed Array when necessary via an enhanced cljs.core/into-array.

Implementation is based heavily on clojure/core/gvec.clj, cljs.core/PersistentVector, and cljs.core/TransientVector.

Performance should be comparable to cljs.core/PersistentVector, although there is additional constant overhead with TypedArray instantiation compared to js/Array.

Adds cljs.core/Vec, cljs.core/TransientVec, cljs.core/vector-of, and updates cljs.core/into-array.



 Comments   
Comment by Adrian Medina [ 19/Mar/15 8:39 PM ]

I still have to test, I will update the issue when that is complete. I just wanted to get my first patch up for review as quickly as possible.

Comment by Francis Avila [ 19/Mar/15 11:59 PM ]

No mention of Uint8ClampedArray.

Should Vec type- or range-check assignments? In Clojure these fail (even with unchecked-math):

  • (vector-of :byte 128) returns [-128]
  • (vector-of :byte "1") returns [1]
  • (vector-of :byte (js-obj)) returns [0]

If we're going to expose host primitive arrays via cljs apis, should we also bring the various other array functions in line with Clojure (and ClojureCLR, which also has extra uint, ubyte, etc types) like you are doing with into-array? Some or all of these issues may warrant another ticket instead, or maybe even a design page:

  • make-array ignores type argument and lacks higher dimensions.
  • object-array, int-array, etc. maybe should return TypedArrays.
  • Missing ubyte-array, ushort-array, uint-array (like ClojureCLR)
  • Missing aset-* setters. (Meaningless in js unless we range-check.)
  • aclone and amap preserve type of input array in Clojure, but not in cljs.
  • missing array casters: bytes, shorts, chars, ints, etc.
  • While we're at it, primitive coercion functions (e.g. int, long, unchecked-int, etc) are either no-ops or differ from clojure. (e.g., int in cljs is like unchecked-int in clojure, but unchecked-int in cljs does nothing). Maybe these should be dropped or should match the javascript ToInt32, ToInt16, etc abstract operations (i.e. those used when assigning to TypedArrays). Maybe these match java semantics also?




[CLJS-994] print a warning when :externs file paths can't be found. Created: 30/Jan/15  Updated: 10/May/15

Status: Reopened
Project: ClojureScript
Component/s: None
Affects Version/s: 0.0-3269
Fix Version/s: Next

Type: Enhancement Priority: Major
Reporter: Crispin Wellington Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement, errormsgs, patch,
Environment:

Linux 64bit

java version "1.7.0_65"
OpenJDK Runtime Environment (IcedTea 2.5.3) (7u71-2.5.3-0ubuntu0.14.04.1)
OpenJDK 64-Bit Server VM (build 24.65-b04, mixed mode)


Attachments: Text File clojurescript-extern-missing-warning.patch    
Patch: Code

 Description   

clojurescript silently ignores missing externs files possibly leading a developer to chase their tail.

Presently it can be very confusing using advanced compilation if you have made a mistake in the path name of one of your :externs files. This patch makes the compiler print a warning on stderr so you can quickly determine the cause of the broken advanced compilation output.

As a side effect, when doing a basic lein-cljsbuild a warning is always printed:

```
WARNING: js resource path closure-js/externs does not exist
```

This is because lein-cljsbuild quietly adds this extra path to your :externs listing without you knowing.



 Comments   
Comment by David Nolen [ 31/Jan/15 1:59 PM ]

You need to bind *out* to *err*, or just print to it directly a la cljs.util/debug-prn.

Comment by Crispin Wellington [ 31/Jan/15 7:30 PM ]

I did bind out to err. Check the patch.

Comment by David Nolen [ 01/Feb/15 12:30 PM ]

Crispin, oops sorry you are correct. Thanks.

Comment by David Nolen [ 13/Mar/15 7:33 AM ]

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

Comment by David Nolen [ 14/Mar/15 5:55 AM ]

The solution does not work for cljsbuild. It's unclear why there so much machinery in place over the approach taken for deps.clj.

Comment by David Nolen [ 15/Mar/15 10:37 AM ]

Stalled on this cljsbuild issue https://github.com/emezeske/lein-cljsbuild/issues/383

Comment by Crispin Wellington [ 23/Mar/15 2:50 AM ]

This lein-cljsbuild issue is what made me make it just a warning initially, and not a hard error like raising IllegalArgumentException does. Though I agree it should be a hard error. If we start with a warning, it enables the immediate problem for the developer to be resolved, and leaves a wart that the cljs-build project can then see that need fixing on their end. Then when that end is fixed it could be made a hard error. If cljsbuild is fixed fairly soon then all is well, but if it takes a long time, a warning might be a good first step.





[CLJS-791] Cross-target ClojureScript to the DartVM. Created: 06/Apr/14  Updated: 08/Apr/14  Resolved: 08/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Yesudeep Mangalapilly Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cross-target, dart, enhancement, vm
Environment:

Any.



 Description   

ClojureScript is a better language than many others, I figure.
The DartVM is picking up speed. Can CLJS be retargetted to generate
Dart code as well?



 Comments   
Comment by David Nolen [ 08/Apr/14 9:22 AM ]

Queries like this are best directed toward the mailing list before opening tickets. Thanks.





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

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

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

clojurescript in browsers, wrapper for Google Closures WebSocket.


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

 Description   

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

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

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

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



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

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

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

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





[CLJS-420] Unexpected behavior with dispatch on Keyword via protocols Created: 18/Nov/12  Updated: 08/Sep/13  Resolved: 08/Sep/13

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

Type: Defect Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, enhancement


 Description   

At the moment if you create a protocol and expect it to be able to dispatch on keywords you need to do it in js/String, trying to extend on cljs.core.Keyword doesn't work as keywords are just Strings.

See https://gist.github.com/4104635



 Comments   
Comment by David Nolen [ 18/Nov/12 3:14 PM ]

This is a known issue which will have to wait for if and when Keywords and Symbols become proper types in ClojureScript.

Extending js/Object is not recommended, if you actually need to add functionality to the base JS native types the convention is different from Clojure: default instead of Object, string instead of js/String. Please refer to core.cljs if you want more examples.

Comment by Max Penet [ 18/Nov/12 3:27 PM ]

Thanks for the pointer about default and string, I didn't know about that.

I reported the issue at the demand of bbloom. It does seem to be difficult to address without taking a (major) performance hit unfortunately.

Comment by David Nolen [ 08/Sep/13 6:27 PM ]

Made moot by CLJS-576





[CLJS-419] Exclude cljs source file from compilation Created: 17/Nov/12  Updated: 27/Jul/13  Resolved: 18/Dec/12

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

Type: Enhancement Priority: Major
Reporter: Mimmo Cosenza Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement
Environment:

every environment


Attachments: Text File 0001-CLJS-419-exclude-file-or-dir-from-compilation.patch    

 Description   

Scenario:

  1. you have a :dev build and a :prod build
  2. In the :dev build you want to have a brepl connected with the browser and you coded that connection in a isolated cljs source file.
  3. In the :prod build you do not want that connection active, meaning you don't want source cljs file enabling the connection to be included in the compilation.

Given this scenario, you need to duplicate all the cljs source files but the one that enable the connection. This mean you have to manually maintain two code bases.

It could be useful to have a way to :exclude some files from :source-path.



 Comments   
Comment by David Nolen [ 18/Nov/12 3:16 PM ]

This could easily be done by adding support for this in closure.clj - patch welcome!

Comment by Mimmo Cosenza [ 20/Nov/12 6:11 AM ]

I propose to add a new keyword/value in the optimization option map, namely :exclude-path. the value of this option should be a subdir of the source-dir.

Top level scenario:

1. you use lein-cljsbuild to define your cljs project
2. you define more builds, e.g. a :dev build and a :prod build.
(defproject ...
:cljsbuild {:builds
{:dev {:source-path "src/cljs" {:compiler {:output-to "resources/public/js/main_dbg.js"
:optimizations :whitespace
:pretty-print true}}}
:prod {:source-path "src/cljs" {:compiler {:output-to "resources/public/js/main_dbg.js"
:optimizations :advanced
:exclude-path "some-path"}}}})

3. lein-cljsbuild plugin will instruct CLJ compiler by passing it the soruce-dir (e.g. "src/cljs") and the options map which now can contain also an optional :exclude-path keyword.

4. During compilation the compiler will exclude from source-dir any cljs source which is contained in the named excluded directory.

I'll start bottom-up from 4. then I'll try to patch lein-cljsbuild too.

Mimmo

Comment by Mimmo Cosenza [ 07/Dec/12 10:30 AM ]

Hi David, here is the flattened patch relative. The two guys which worked on the patch under my direction are interns in my own company. Next monday we'll send their signed CA.

My best
Mimmo

Comment by Brenton Ashworth [ 07/Dec/12 11:13 AM ]

In general, we should not complicate the compiler with additional options when functionality can be provided by external tools.

I think this feature can be provided by external tools. The compiler will only automatically pull in things that are actually dependencies of the sources that we provide to the compiler. External tools should provide ways to limit what is handed to the compiler.

I would first attempt to modify lein-cljsbuild to do what you want.

When using the compiler directly, you can provide your own implementation of Compilable which, when given a directory, will filter out sources based on some criteria you provide. In my projects I have custom implementations of Compilable that do just this. You should be able to do the same thing in lein-cljsbuild.

-Brenton

Comment by Mimmo Cosenza [ 07/Dec/12 12:30 PM ]

Thanks for the advice Brenton. I'll try to understand from the maintainer of `lein-cljsbuild` where to start from. I agree with you about keeping the compiler clean from options that can be implemented by the tools. But I'm no so sure that patching lein-cljsbuild we'll be as easy as adding `:exclude` option to the compiler.

Mimmo

Comment by Brenton Ashworth [ 07/Dec/12 1:04 PM ]

It doesn't matter which one is easier to do. Every new option and special case that we add to the compiler makes it more difficult to understand how changes will impact users.

Comment by David Nolen [ 07/Dec/12 5:40 PM ]

I agree that anything that can be solved at higher level tools is better - it wasn't clear to me from the implementation that Compilable could be used to control this - but I see now.

Mimmo, cljs.closure/build takes a Compilable and a map of options. So lein-cljsbuild could construct the custom Compilable that understands :excludes and pass it along.

Comment by Evan Mezeske [ 09/Dec/12 9:48 PM ]

FWIW, I agree with Brenton that this should be in lein-cljsbuild.

I didn't know that cljs.closure/build was flexible enough to do this already. I always thought that it needed to be extended so that a vector of files could be passed in, or something, but it sounds like the Compilable approach should work just fine.

I will happily accept a patch for this. One thing to keep in mind, though, is that the :exclude entry should not be in the :compiler map if lein-cljsbuild is handling it. The :compiler map is passed straight through as options to cljs.closure/build. So, the :exclude entry should be a neighbor of the :compiler entry.

Comment by Mimmo Cosenza [ 10/Dec/12 4:20 AM ]

Hi all,
we all agree with Brenton and yes, the :exclude option has to be at the same hierarchical level of :source-path. I'll verify if we can also extend :source-path from taking a String to taking a vector of String, in such a way that the user could ask to compile more cljs src-dir.

Mimmo

Comment by Mimmo Cosenza [ 10/Dec/12 4:37 AM ]

Hi all,
just a small add on to the previous comment. I don't think we're going to update cljsc/cljsc.clj, which I consider a kind of tool, much more fragile and limited than cljsbuild plugin, to interoperate with CLJS compiler.

My best

Mimmo

Comment by David Nolen [ 18/Dec/12 3:09 PM ]

Should be resolved by tools that use the compiler.





[CLJS-402] Add a facility to obtain the version information of the clojurescript build that is in use Created: 21/Oct/12  Updated: 04/Nov/13  Resolved: 04/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Frank Siebenlist Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement

Attachments: File CLJS402-3rd-patch-file.diff    

 Description   

Currently there is no function or var defined in the clojurescript library that can be used to easily obtain the version information of the build that is in use.

Without that version information, debugging and reporting of errors is more cumbersome than needed, wastes time, causes confusion discussing issues...

A simple function and/or var like the ones used for clojure would be very helpful:

user=> (clojure-version)
"1.4.0"
user=> clojure-version
{:major 1, :minor 4, :incremental 0, :qualifier nil}



 Comments   
Comment by Frank Siebenlist [ 21/Oct/12 2:16 PM ]

Auto-generation from the build script of version_autogen.clj and version_autogen.cljs files
that both define the cljs.version-autogen/clojurescript-version
with the current version info for both the clj and cljs environments

Comment by David Nolen [ 21/Oct/12 3:43 PM ]

Why a separate namespace for this?

Comment by Frank Siebenlist [ 21/Oct/12 9:15 PM ]

Good point - probably better to add the clojurescript-version var to the cljs.core namespace.

New patch change the build script to auto-generates the clojurescript-version assignment statements in both cljs/core.clj and cljs/core.cljs to reflect the correct version info.

In that way it resembles more the clojure.core use of clojure-version.

Note that the two separate assignments for clj and cljs are needed to detect out-of-sync of repl-compiler version with the compiler used to generate the js-code that was downloaded thru the webpage.

Difficult to write a test-script, but it seems to work in my repl:


user=> (require 'cljs.core)
nil
user=> cljs.core/*clojurescript-version*
{:minor 0, :revision 1515, :major 0}
user=> (run-repl-listen)
"Type: " :cljs/quit " to quit"
ClojureScript:cljs.user> *clojurescript-version*
{:revision 1515, :major 0, :minor 0}
ClojureScript:cljs.user>

Comment by Frank Siebenlist [ 21/Oct/12 9:16 PM ]

CLJS-402: build script auto-generates the clojurescript-version assignment statements in both cljs/core.clj and cljs/core.cljs to reflect the correct version info

Comment by Frank Siebenlist [ 22/Oct/12 12:00 AM ]

When I tried to implement a "clojurescript-version" function like the "clojure-version" one, I (re)discovered that cljs/core.clj is not your average cli-file but some funky file used by the clojurescript compiler to bootstrap - in other words it's not a good file to require and run normal functions from your clj-environment.

So I moved the *clojurescript-version* var to cljs/compiler.clj as it seemed to make sense to associate the version with the compiler.

On the cljs site, I left the *clojurescript-version* in cljs/core.cljs, although I was tempted to create a cljs.compiler namespace to store that var on the cljs side to make it symmetric .

Also added a (clojurescript-version) function to both the clj and cljs sides, with an added special-fn in the cljs/repl.clj such that you can ask for the compiler version of the repl-server from the cljs-repl.

Updated 3rd patch file replaces the previous one.

Just wanted to record an example of a mismatch in compilers:

user=> (run-repl-listen)
"Type: " :cljs/quit " to quit"
ClojureScript:cljs.user> (clojurescript-version)
"0.0.1515"
ClojureScript:cljs.user> (clj-clojurescript-version)
"0.0.1516"
ClojureScript:cljs.user>

This happens when you generate the javascript from your cljs with a "lein cljsbuild once" command with compiler version "0.0.1515",
then upgrade the compiler to "0.0.1516" and then run the cljs-repl without regenerating the javascript for the webpage-download.
Very obscure things can happen after that...

Comment by Frank Siebenlist [ 22/Oct/12 12:03 AM ]

build script auto-generates the clojurescript-version assignment statements in both cljs/compiler.clj and cljs/core.cljs to reflect the correct version info
Function "clojurescript-version" is added to both compiler.clj and core.cljs to mimic equivalent "clojure-version"
Special-fn "clj-clojurescript-version was added to repl.clj to obtain the compiler's clojurescript-compiler version from the cljs-repl

Comment by David Nolen [ 22/Oct/12 7:06 AM ]

The patch uses sed, I'm not sure this is such a great idea since whatever the patch does should probably work with whatever build setup we have on Hudson.

Comment by Chouser [ 22/Oct/12 8:05 AM ]

Would it make sense to call this 'clojure-version' as well, instead of clojurescript-version? It's a map, and so various flags could be added to differentiate jvm, js, python, c, etc. runtimes. What does ClojureCLR do?

Comment by Frank Siebenlist [ 22/Oct/12 10:52 AM ]

sed was already used in the build script... a few lines down, the pom file is transformed with:

sed -e s/CLOJURESCRIPT_VERSION/0.0-$REVISION/ < "$POM_TEMPLATE" > "$POM_FILE"

Comment by Frank Siebenlist [ 22/Oct/12 11:15 AM ]

A quick scan of the src-code showed that ClojureCRL uses clojure-version and *clojure-version*.

However, there is less chance for confusion as you are supposedly running on the CLR already.

With ClojureScript you can have these three intertwined "clojure" environments that are all live at the same time: the clojure version, the repl clojurescript version, and the clojurescript version used to compile the initially loaded js. Not sure if overloading the clojure-version fn-name and relying on namespaces to differentiate will be helpful... but it's not that big of a deal and I'm easily persuaded otherwise - somebody should make the call and I'll change it.

Comment by David Nolen [ 26/Feb/13 7:51 AM ]

Sorry just now coming back to this ticket. I think it's probably preferable that the map of properties be called the same thing everywhere. I would apply the patch with this change. I note it no longer applies to master.

Comment by David Nolen [ 04/Nov/13 7:47 PM ]

fixed https://github.com/clojure/clojurescript/commit/6a9cae995837223cc7d236d110e01d723ae81f8a





[CLJS-398] new macro cljs.core/extend-instance Created: 19/Oct/12  Updated: 27/Jul/13  Resolved: 19/Oct/12

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

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-new-macro-clojure.core-extend-instance-can-be-used-t.patch     Text File 0001-Test-for-extend-instance.patch    
Patch: Code and Test

 Description   

Patch introduces an extend-instance macro, similar to extend-type.
It doesn't extend the .prototype property, but assigns the prototype impl members to the instance itself.

This is useful to let existing instances, e.g. parsed JSON directly implement a protocol.



 Comments   
Comment by David Nolen [ 19/Oct/12 5:13 PM ]

Rich Hickey already had a good name for an operation like this - specify.

Comment by Herwig Hochleitner [ 19/Oct/12 6:10 PM ]

OK, I've declined the ticket, since not only the name but also the patches should be disregarded.

The problem with above impl is that the generated implementing fns get instantiated, every time extend-instance is evaluated. That is not acceptable on a per-instance basis.

I will work on a new macro called specify, which allows passing implementing fns.

Is a syntax similar to clojure.core/extend right for specify?
Should I create a new ticket?

Comment by David Nolen [ 19/Oct/12 7:51 PM ]

As far as I understand it the interface for specify should be the same as extend-type.





[CLJS-397] Omit var reads in statement context Created: 19/Oct/12  Updated: 27/Jul/13  Resolved: 23/Oct/12

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch, size

Attachments: Text File 0001-CLJS-397-var-reads-in-a-statement-context-get-omitte.patch    
Patch: Code

 Description   

Attached patch updates cljs emitter to not emit var references in statement context.
That causes toplevel deftypes to not return their generated type, which lets gclosure strip them if unused

cljs helloworld shrinks from ~90K to ~69K

https://groups.google.com/d/topic/clojure/LNfJRw07u8I/discussion



 Comments   
Comment by David Nolen [ 19/Oct/12 5:15 PM ]

Can we get the ticket # in the commit message? Thanks!

Comment by Herwig Hochleitner [ 19/Oct/12 5:42 PM ]

Of course, sorry! Here is a new patch, if you please.

Comment by David Nolen [ 19/Oct/12 7:32 PM ]

Thanks. I'm confused about the second aspect of the patch, what do you mean by bogus error messages?

Comment by Herwig Hochleitner [ 19/Oct/12 11:23 PM ]

Well, the repl uses a :statement context to generate the javascript which gets printed out as part of a stack trace. When setting :expr back to :statement in the repl, you can try the following scenario:

  • enter into the repl (def val :worked) (if (> (Math/rand) 0.5) val (throw ""))
  • half of the time you will get :worked
  • half of the time you will get a stacktrace citing 'ns.val = ":worked"; if (Math.rand() > 0.5){}{throw ""}'
  • that makes no sense

With the full patch, the printout will be smth like '(function(){ns.val = ":worked"; if (Math.rand() > 0.5){return ns.val}{throw ""}})()', which is noisier, but more like what actually got evaluated by the repl client.

##EDIT#APPEND##

The reason the repl still works, even when analyzing in a statement context with the patch is, that in order to get the return value, the repl statement has to be evaluated in an :expr context anyway.
This happens by unquoting it into a wrapper form. With the patch the wrapper form also gets analyzed in an :expr context, which doesn't hurt.
Only when an exception gets caught, the js of the unwrapped expr is used, which now always matches the its counterpart in the wrapper form.

Comment by David Nolen [ 20/Oct/12 9:33 AM ]

I'm still confused. Does this additional modification have anything to do with the ticket? By that I mean does the patch require the second modification? If it does can you explain a further? Thanks.

Comment by Herwig Hochleitner [ 20/Oct/12 1:17 PM ]

The patch doesn't require the second modification in the sense that everything still works if it's left out.

The patch requires the modification in the sense that it would break stack traces on the REPL otherwise + the two changes should be reverted together if and when we move such optimizations out of the emitter into an optimization pass.

So the trade off is in always having the correct code in a REPL stacktrace in exchange for making it more verbose. Again, this doesn't influence behavior, so it's a matter of prioritizing requirements.

Comment by David Nolen [ 21/Oct/12 3:48 PM ]

I still don't understand why/how the part of the patch that addresses the ticket breaks stack traces. Can you explain?

Comment by Herwig Hochleitner [ 22/Oct/12 10:06 AM ]

1) var read statements get omitted
2) typing "var" into the repl still works, because it's analyzed in a wrapper form
3) if it throws, the printed generated js is the original form in a statement context, you can observe this with above code sample.

Comment by David Nolen [ 22/Oct/12 10:16 AM ]

Ok I understand now, I don't think the second part of the patch is relevant. The user entered two statements, not one combined in an implicit do which is what the other part of the patch does. Can we get a new patch that only includes the part that addresses the ticket? Thanks!

Comment by Herwig Hochleitner [ 22/Oct/12 2:58 PM ]

For the record: There was some talk in the chat, where I laid down my rationale for changing the repl specifically: "Technically repl inputs have to be in an expr context, to capture the return val. And they are, by virtue of the wrapping form. The change makes that fact explicit, thereby keeping stack traces sane."

I also discovered, that two other ops, beside a var read, don't emit in a statement context either and therefore have same issue. I created http://dev.clojure.org/jira/browse/CLJS-403 for the REPL change.

Updated attached patch contains just the optimization.

Comment by David Nolen [ 23/Oct/12 6:49 PM ]

fixed, http://github.com/clojure/clojurescript/commit/97e5fbd1e1597d58be35fd8320c8044ccc9d3a3d





[CLJS-387] Add docstring from def and ns definitions to @namespaces metadata map, and make reflect functions make use of that Created: 07/Oct/12  Updated: 27/Jul/13  Resolved: 17/Oct/12

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

Type: Enhancement Priority: Minor
Reporter: Frank Siebenlist Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docs, enhancement, patch,, reflection
Environment:

clojure/clojurescript "0.0-1450"


Attachments: File add-ns-def-doc-patch.diff    
Patch: Code

 Description   

The docstrings were parsed from the definitions-forms for def and ns, but not added to the @namespaces metadata map.
There is no :doc entry used in the ns' metadata in the @namespaces.
No ns's :doc info is communicated to the browser in the reflect functions with reflect/doc.
Patch-file is attached with code-changes that add the :doc info for ns and def to the @namespaces, and enhances the reflect functions to communicate that info to the browser in the reflect/doc call.



 Comments   
Comment by David Nolen [ 15/Oct/12 11:06 PM ]

This patch no longer applies, mind updating it?

Comment by Frank Siebenlist [ 16/Oct/12 12:10 AM ]

This patch should apply to master version on Mon, 15 Oct 2012 22:03:19 -0700 (4defcbcf19112b9be6a4a27b5d8855552bf94948)

Comment by David Nolen [ 17/Oct/12 10:57 AM ]

Excellent, fixed http://github.com/clojure/clojurescript/commit/bef56a74f2eeecabfe0c0a28d89b455dce576ea3

Please at the ticket # to the commit message though, thanks!





[CLJS-260] Add clojure.core/shuffle implementation Created: 17/May/12  Updated: 27/Jul/13  Resolved: 20/May/12

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

Type: Enhancement Priority: Minor
Reporter: Evan Mezeske Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch,, test

Attachments: Text File 0001-Add-clojure.core-shuffle-and-a-test.patch     Text File shuffle.patch     Text File shuffle.v2.patch    
Patch: Code and Test

 Description   

I added a simple implementation of clojure.core/shuffle, which uses goog.array's Fisher-Yates for its implementation. Included in the patch is a test to make sure it works.



 Comments   
Comment by David Nolen [ 17/May/12 7:07 PM ]

Why don't you return a vector like Clojure does?

Comment by Evan Mezeske [ 17/May/12 7:11 PM ]

Return a vector like Clojure.

Comment by David Nolen [ 18/May/12 9:23 PM ]

The patch is not correctly formatted with attribution.

Comment by Evan Mezeske [ 20/May/12 4:24 PM ]

Use git format-patch instead of git diff, to hopefully provide the right patch format.

Comment by David Nolen [ 20/May/12 10:28 PM ]

fixed, http://github.com/clojure/clojurescript/commit/b06905863a9b0ce3618634d8fe0effb3aefd4063





Generated at Sat May 30 15:50:08 CDT 2015 using JIRA 4.4#649-r158309.