<< Back to previous view

[CLJS-1637] Missing docstrings for a few vars Created: 30/Apr/16  Updated: 30/Apr/16

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

Type: Defect Priority: Trivial
Reporter: Mike Fikes Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

Docstrings are missing for a few vars, where Clojure has docstrings that can be copied verbatim or used as a basis for ClojureScript:

symbol
newline
special-symbol?
defonce






[CLJS-1636] Mark some symbols in core macros ns as private Created: 27/Apr/16  Updated: 27/Apr/16

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

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

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

 Description   

There are some symbols in the core macros namespace that are not meant for external consumption. Some of these are marked private and some aren't. This ticket asks that the others be marked private as well.

An example of one symbol marked private is defcurried.
An example of one symbol not marked private is caching-hash.



 Comments   
Comment by Mike Fikes [ 27/Apr/16 8:21 AM ]

In CLJS-1636.patch, I checked and it appears nothing in the compiler codebase is explicitly using these symbols outside of the cljs.core namespace. But, it is still worth scanning through these to check if they make sense. For example js-debugger and js-comment are a couple that might actually be meant for public use, but it is difficult to tell.

Comment by Mike Fikes [ 27/Apr/16 2:43 PM ]

Note, that in #cljs-dev slack, there appears to be interest in caching-hash being public.

(I don't mind revising the patch to suit whatever is needed. At the same time, I'm certainly not in a position to take decisions on what is public API or not.)

Comment by Mike Fikes [ 27/Apr/16 2:43 PM ]

Note, that in #cljs-dev slack, there appears to be interest in caching-hash being public.

(I don't mind revising the patch to suit whatever is needed. At the same time, I'm certainly not in a position to take decisions on what is public API or not.)





[CLJS-1635] Var type implements IEquiv but not IHash Created: 26/Apr/16  Updated: 30/Apr/16

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

Type: Defect Priority: Minor
Reporter: Chris Vermilion Assignee: David Nolen
Resolution: Unresolved Votes: 0
Labels: deftype
Environment:

Tested on OS X 10.11, Chrome.


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

 Description   

The Var type implements IEquiv based on the var's symbol, but not IHash. That means that two vars with the same symbol compare equal but don't hash equal, which will cause strange results if you put them in a hash-{map,set}:

cljs.user=> (def foo "bar")
#'cljs.user/foo
cljs.user=> (= #'foo #'foo)
true
cljs.user=> (= (hash #'foo) (hash #'foo))
false

Patch forthcoming.



 Comments   
Comment by Chris Vermilion [ 26/Apr/16 10:41 PM ]

Patch note: The patch fixes the issue but I haven't added a test. It didn't seem like the hash behavior of basic types was tested in general, but moreover while I think this behavior is desirable I'm not sure it should be guaranteed. Happy to write a test if that would be useful.

Comment by Chris Vermilion [ 26/Apr/16 10:48 PM ]

Aside for the curious on how this came up at all: the Schema library uses Vars to specify recursive schemas, and does internal caching by with a map keyed by schemas themselves. If you defined the same recursive schema multiple times, the results would be unpredictable, since two equivalent recursive schemas would compare equal but wouldn't necessarily be interchangeable as map keys.





[CLJS-1634] Track bound dynamic variables to support binding in async mechanisms. Created: 26/Apr/16  Updated: 30/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Christian Weilbach Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: cljs, enhancement
Environment:

Any cljs version.



 Description   

The issue has been raised before:

While the reasoning behind the proposal is still valid, the original approach has made no progress due to the performance penalty. I have implemented a simplified approach with mutable JavaScript datastructures to minimize the performance impact. Because we are single-threaded we can use js assignment and don't need to port Clojure's binding frame. A small penalty is paid by the user of binding (see benchmark8) and a higher one by async mechanisms capturing and restoring the bindings (benchmark1-7):

https://gist.github.com/whilo/a8ef2cd3f0e033d3973880a2001be32a

I would provide patches to ClojureScript, if this looks like a worthwhile approach.



 Comments   
Comment by Antonin Hildebrand [ 30/Apr/16 6:05 AM ]

Just for record I commented on it here: https://gist.github.com/whilo/a8ef2cd3f0e033d3973880a2001be32a#gistcomment-1764489. Not sure if GitHub sends out notifications about new gist comments.

Comment by Christian Weilbach [ 30/Apr/16 6:18 AM ]

Thanks for pointing it out. David Nolen has also pointed out prototype chains to address this issue and now I see what he meant. I am not familiar enough with the internals of "this" in JavaScript, but one problem I see is that you need to distinguish dynamic vars at the call site. The advantage of using an object directly and capturing and restoring the frame explicitly when you enter and leave the code is that call sites are totally unaffected. The cost is only paid a little at the binding site and mostly in async libraries (bound-fn). But I might still need to look further into "this" . I have not got the gist comment from github.

Comment by Antonin Hildebrand [ 30/Apr/16 7:23 AM ]

Correct.

> you need to distinguish dynamic vars at the call site

I agree. My initial motivation was to solve a bit different problem without cooperation from library authors. I didn't want to modify ClojureScript behaviour and wanted to be just touching own code or doing trivial changes in library forks. Just wanted to share my thoughts about the implementation.

I have a feeling that solving this "async context" problem will be difficult. You will need async library authors to adapt their libraries. And users unaware of this will be running into issues anytime they step outside of bound-fn aware async libraries (for example using raw js interop). I believe Angular people solved this robustly in https://github.com/angular/zone.js. The implementation is quite scary monkey-patching, but if they were able to wrap all existing async calls at lowest level, maybe we could just build on top of their foundation and use zone.js as parallel mechanism for `binding`.





[CLJS-1633] Improve error associated with invalid foreign-libs :file path Created: 26/Apr/16  Updated: 26/Apr/16

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

Type: Enhancement Priority: Minor
Reporter: Oliver George Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

The current error reported when :advanced compiling with an invalid :foreign-libs :file path is effectively (slurp nil):

Caused by: java.lang.IllegalArgumentException: Cannot open <nil> as a Reader.

With a small patch this could be improved to provide a specific message and relevant context, something like:

Caused by: clojure.lang.ExceptionInfo: Unable to resolve url for foreign-deps source {:foreign true, :url nil, :source-url nil, :provides ["cljsjs.example-thing"], :requires (), :lines nil, :source-map nil, :file "broken-path-to-example-thing.js"}

I've created a simple repo based on the mies template to demonstrate the problem. Note that core.cljs requires the foriegn-lib which is defined in deps.clj (but with an invalid :file path). scripts/release.clj shows current behaviour. scripts/release-with-patch.clj shows proposed behaviour.

https://github.com/condense/apr26-foreign-libs-path-error.core.git

Below shows an isolated fix to cljs.closure/foreign-deps-str which checks for a nil url. An alternative approach might be to check at the point where the source maps are prepared (something like (assert (or url url-min) "xxx")) but this would be more likely to have side effects.

(defn foreign-deps-str [opts sources]
  (letfn [(to-js-str [ijs]
            (if-let [url (or (and (#{:advanced :simple} (:optimizations opts))
                                  (:url-min ijs))
                             (:url ijs))]
              (slurp url)
              (throw (ex-info "Unable to resolve url for foreign-deps source" ijs))))]
    (str (string/join "\n" (map to-js-str sources)) "\n")))

I'd be happy to prepare a patch if this seems like the right approach. Have signed contributor agreement.






[CLJS-1632] Typos, c/e, and consistency of docs/params Created: 24/Apr/16  Updated: 25/Apr/16  Resolved: 25/Apr/16

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

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

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

 Description   

Fixes typos, consistency, making params match docs, etc.

Covers what was previously CLJS-1510, CLJS-1522, CLJS-1526, CLJS-1534, CLJS-1535, and other similar changes.



 Comments   
Comment by David Nolen [ 25/Apr/16 6:15 AM ]

fixed https://github.com/clojure/clojurescript/commit/8d33dd4e55cc87c205bb5c37d0234af6e2132388





Generated at Sun May 01 08:49:41 CDT 2016 using JIRA 4.4#649-r158309.