<< Back to previous view

[CLJS-878] Missing preamble file causes unhelpful stack trace Created: 30/Oct/14  Updated: 31/Oct/14  Resolved: 31/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Darrick Wiebe Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: cljs, errormsgs


 Description   

I think it would be helpful to throw an exception with the missing file path rather than the current

java.lang.IllegalArgumentException: No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil
.

Right now you need to work your way down about 10 lines into the stacktrace to find

cljs.closure$preamble_from_paths$fn__3097.invoke(closure.clj:526)
which is the only hint as to what has happened.



 Comments   
Comment by David Nolen [ 31/Oct/14 2:42 AM ]

This is a simple enhancement, a patch is welcome. Thanks.

Comment by David Nolen [ 31/Oct/14 2:44 AM ]

Actually this already appears fixed in master https://github.com/clojure/clojurescript/commit/9fd6bf5bd55421c3d5becacc5230ed661d6fb3c3

Comment by David Nolen [ 31/Oct/14 2:45 AM ]

See CLJS-869





[CLJS-819] cljs.reader cannot handle character classes beginning with slashes in regex literals Created: 20/Jun/14  Updated: 01/Jul/14  Resolved: 01/Jul/14

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

Type: Defect Priority: Critical
Reporter: Ziyang Hu Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: bug, cljs, reader

Attachments: Text File cljs-819.patch    

 Description   

cljs.user> (cljs.reader/read-string "#\"\\s\"")
Compilation error: Error: Unexpected unicode escape \s



 Comments   
Comment by Ziyang Hu [ 20/Jun/14 10:03 AM ]

This in particular means that (cljs.reader/read-string (str [#"\s"])) won't work

Comment by Francis Avila [ 25/Jun/14 11:46 AM ]

Patch and test.

Comment by David Nolen [ 01/Jul/14 9:25 PM ]

fixed https://github.com/clojure/clojurescript/commit/32259c5ff3f86ea086ae3949403df80c2f518c7e





[CLJS-723] add :preamble option to compiler Created: 10/Dec/13  Updated: 17/Dec/13  Resolved: 17/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: Travis Vachon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs

Attachments: Text File cljs_723.patch    

 Description   

Per this thread:

https://groups.google.com/forum/#!searchin/clojurescript/preamble/clojurescript/rUIlLfcYZvE/Yskfh4znL_0J

1) :preamble 's value will be a vector of paths
2) the compiled output is prepended with the contents of the files at those paths
3) the generated source map points to the correct/adjusted line numbers

Additionally, when compiling for the :nodejs target the preamble contents will default to the hashbang we currently write in that situation.



 Comments   
Comment by Travis Vachon [ 11/Dec/13 4:59 PM ]

patch to add :preamble

I'm not confident this is the best way to do the source map stuff - any thoughts/suggestions would be very welcome

Comment by Michael Bradley, Jr. [ 13/Dec/13 5:55 PM ]

Could the issue (and patch) be expanded to also support a :postamble option?

With both :preamble and :postamble options available, it would be easy to implement more sophisticated compiler-output wrappers, such as the one used by David Nolen's mori library (which I helped adapt from the Q javascript library).

See: https://github.com/swannodette/mori/blob/master/support/wrapper.js

Comment by David Nolen [ 14/Dec/13 2:37 PM ]

I'm fine with extending this to {:postamble}.

Comment by David Nolen [ 14/Dec/13 2:39 PM ]

fixed https://github.com/clojure/clojurescript/commit/454bcb70fbbd9e9feb20ada7b4043eab70dafea2

Comment by David Nolen [ 14/Dec/13 2:40 PM ]

Oops resolved wrong ticket

Comment by David Nolen [ 17/Dec/13 4:26 PM ]

fixed, https://github.com/clojure/clojurescript/commit/136bf46c656265a93dd15c40925f11edb34bd127.

If someone wants to open a postamble ticket and attach a patch go for it.





[CLJS-722] Support parse.com's Cloud Code environment in :nodejs builds Created: 09/Dec/13  Updated: 23/Jan/14  Resolved: 23/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Travis Vachon Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs

Attachments: Text File cljs_722.patch     Text File cljs_722v2.patch     Text File cljs_722v3.patch     Text File parse.patch    
Patch: Code

 Description   

parse.com's Cloud Code is very similar to node.js but expects no hashbang and doesn't provide util.js, so provide a compiler argument to elide the header and wrap the util.js require in a try...catch



 Comments   
Comment by David Nolen [ 10/Dec/13 9:21 AM ]

Please format the patch so it can be applied with git am. See http://github.com/clojure/clojurescript/wiki/Patches

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

git am-able patch

Comment by Travis Vachon [ 10/Dec/13 9:47 AM ]

sorry about that! will update all the tickets I've filed recently as well

Comment by Travis Vachon [ 10/Dec/13 1:22 PM ]

so actually, I think a better solution to this will be to add support for :preamble as proposed here:

https://groups.google.com/forum/#!searchin/clojurescript/preamble/clojurescript/rUIlLfcYZvE/Yskfh4znL_0J

I'm imagining that the default preamble for the :nodejs target will be the hashbang

that will let us a) redefine the require function in our compiled output to squelch the util import and b) avoid writing the hashbang

Comment by David Nolen [ 11/Dec/13 10:52 AM ]

so is this ticket superseded by CLJS-723?

Comment by Travis Vachon [ 11/Dec/13 12:02 PM ]

yeah - if I can write arbitrary js at the beginning of my compiled file I don't need this patch. Can close.

Comment by Travis Vachon [ 12/Dec/13 8:22 AM ]

arg, it looks like I spoke too soon - redefining require in the cloud code env appears to be impossible. I spent a couple hours trying every scoping trick I could find - it looks like the only way to redefine require is by defining a function (assignment just doesn't seem to take) and that function is always hoisted to the very top, so there's no way to get a handle on the original require function. Parse's sandbox doesn't seem to have another way to reference require, either.

How would you feel about moving {{ (set! cljs.core/string-print (.-print (require "util"))) }} into a function node.js people can call rather than doing it when the module is loaded?

Comment by David Nolen [ 12/Dec/13 8:38 AM ]

Honestly I'd be happy to remove it altogether and let people just handle this themselves. Or allow people to disable it if they are using the nodejs target option.

Comment by Travis Vachon [ 12/Dec/13 10:07 AM ]

ok great. I'm inclined to leave it in a function because it feels sort of non-obvious to me - would be nice to help people avoid reinventing in every codebase. I'll attach a new patch.

Comment by Travis Vachon [ 12/Dec/13 10:53 AM ]

add new patch that just moves the problematic require into a function. the hashbang issue the original patch addressed is better fixed with the preamble work in http://dev.clojure.org/jira/browse/CLJS-723

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

Isn't this a breaking change for existing Node.js users?

Comment by Travis Vachon [ 22/Jan/14 2:00 PM ]

it is yeah, I assumed that was ok because you said you'd be happy to remove it altogether - I'm ok with removing it if you think that's a preferable breaking change.

Comment by David Nolen [ 22/Jan/14 2:11 PM ]

Lets rename it to enable-util-print! to match enable-console-print!.

Comment by Travis Vachon [ 22/Jan/14 2:13 PM ]

cool! will do. I'll update the patch today.

Comment by Travis Vachon [ 22/Jan/14 7:22 PM ]

rename function to enable-util-print!

Comment by David Nolen [ 23/Jan/14 7:52 AM ]

fixed, https://github.com/clojure/clojurescript/commit/b299a6b1e9a522de0b1c3345f54f164bc3524f8f





[CLJS-636] ClojureScript Analyzer/Compiler should have extensible hooks to handle warnings Created: 24/Oct/13  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: Minor
Reporter: Sean Grove Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, cljsbuild

Attachments: Text File cljs-warning-middleware.patch    
Patch: Code

 Description   

To make the build process more configurable and stricter/less strict as needed by various projects, the cljs compiler should provide hooks that're invoked whenever a warning is emitted. Eventually these should be exposed through something like cljsbuild.

The attached patch changes the analyzer to emit warnings and emit dynamic warning handlers.

Note sure where the test for this could go though, it doesn't look like there's any infrastructure for the clj side.



 Comments   
Comment by Chas Emerick [ 25/Oct/13 11:24 AM ]

Couple of things:

1. You don't need when-not; if the handlers var is empty or nil, doseq will handle it just fine.
2. The default value of the handlers var should probably be a function that mimics current functionality?
3. I'd like to suggest that all of the relevant data for each warning be passed in addition to the type of warning. i.e. instead of just :dynamic, {:type :dynamic :name (:name ev)}, etc.

Comment by David Nolen [ 25/Oct/13 12:14 PM ]

I agree that the default value of the handler should do what we currently do and that we should pass all information about the warning we have.

Comment by Sean Grove [ 26/Oct/13 12:14 AM ]

Updated to remove (when-not (nil?..., and moves the current behavior into cljs-warning-handlers as the default handler.

It also changes the warning types to introduce more nuanced differences, allowing the default-warning-handler to be entirely self-contained. It relies on different keys being in the map for any given error type when producing the warning message, but it's probably manageable.

It also means there's a neat place to to addition error-type-specific information for handlers to pull out as needed. Right now just enough goes into each type to reproduce the current default behavior, but more could be passed along as needed - it's not always a standard set of data for each type, so maybe this is a good way to go about it.

Comment by Sean Grove [ 26/Oct/13 12:32 AM ]

A few fixes, tests pass, make sure (warning ... is passed the correct warning type, fix no-warn macro.

Comment by David Nolen [ 26/Oct/13 9:58 AM ]

Can we please get a squashed patch? Thanks!

Comment by Sean Grove [ 26/Oct/13 12:28 PM ]

Squashed patched

Comment by David Nolen [ 26/Oct/13 2:22 PM ]

Chas this looks pretty good to me, good enough for lein cljsbuild to use?

Comment by Sean Grove [ 27/Oct/13 11:10 AM ]

Fix default-warning-handler to respect cljs-warnings. Tests still pass, but would like having some clj-side tests to assert behavior around this stuff. Should I open another issue about getting clojure.test setup and tied into the build, or is that uninteresting right now?

Comment by David Nolen [ 27/Oct/13 8:07 PM ]

We already have placeholder test files for the analyzer, compiler, and closure. Feel free to add tests!

Comment by Chas Emerick [ 27/Oct/13 9:50 PM ]

Will review ASAP. Thanks for working on this!

Comment by Chas Emerick [ 28/Oct/13 8:02 AM ]

The patch looks good. I think people are going to want to delegate to default-warning-handler as a fall-through (e.g. to emit the message corresponding to a warning, even if a particular tool is going to choose to treat it as an error), but we can address that later. Once this lands, we'll be able to tie up https://github.com/emezeske/lein-cljsbuild/issues/225. Thanks!

Comment by David Nolen [ 28/Oct/13 8:35 AM ]

Sean, when I run (fn [] (+ a b)) at the REPL (script/repljs) prior to this patch I get warnings about undeclared vars. After applying the patch I no longer get warnings.

Comment by Sean Grove [ 28/Oct/13 9:50 AM ]

Will take a look and add some clj tests to confirm the behavior.

Comment by Sean Grove [ 02/Nov/13 2:46 PM ]

Made sure all references to cljs-warnings were updated appropriately with the new warning types, added some basic tests, changed confirm-var-exists behavior to only call warning if a warning case has been triggered.

Comment by Sean Grove [ 02/Nov/13 2:53 PM ]

There are some bigger-than-expected changes in here, and it'd be nice to get some extra eyes on it to make sure warnings are triggered where expected. I tested a lot of it out in the repl and added some tests, but don't have enough code to exercise all of the paths.

Comment by David Nolen [ 04/Nov/13 8:02 AM ]

Patch looks good to me and it appears to work as advertised

Comment by David Nolen [ 04/Nov/13 8:05 AM ]

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





[CLJS-630] Old-style property accessor raises warning on compile Created: 19/Oct/13  Updated: 24/Oct/13  Resolved: 24/Oct/13

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

Type: Defect Priority: Minor
Reporter: Sean Grove Assignee: Sean Grove
Resolution: Completed Votes: 0
Labels: cljs
Environment:

cljs


Attachments: Text File fix-clojure-browser-repl-property-accessor.patch     Text File fix-various-property-accessor.patch    
Patch: Code

 Description   

clojure.browser.repl is accessing the property of an event via the / syntax, which seems to have been deprecated:
https://github.com/clojure/clojurescript/blob/f74b50967ef4041db585bd6c6f2f490285f61961/src/cljs/clojure/browser/repl.cljs#L71

It causes warnings on compile:

WARNING: No such namespace: e at line 71 resources/public/js/bin-debug/clojure/browser/repl.cljs

Attached is a fix



 Comments   
Comment by Greg Chapman [ 19/Oct/13 3:25 PM ]

e/currentTarget also appears inside the clojure.reflect module (reflect.cljs, line 19 - where e is again an event object). Presumably this should also be changed if this style of property access has been deprecated.

Comment by Sean Grove [ 20/Oct/13 5:27 PM ]

Updates another e/currentTarget instance as pointed out by Greg Chapman.

Comment by David Nolen [ 24/Oct/13 9:01 AM ]

fixed, https://github.com/clojure/clojurescript/commit/2ce425abccd83887001c056e962aadb89683a86b https://github.com/clojure/clojurescript/commit/d31b5365bef4423e59f8112657a0c2402e254195





[CLJS-576] Implement Reified Keywords Created: 26/Aug/13  Updated: 08/Sep/13  Resolved: 08/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Sean Grove Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: cljs, keywords

Attachments: Text File real_keywords.patch    

 Description   

Implement real/reified keywords in ClojureScript so we can stop modifying the String.prototype for strings-as-keywords. Attached patch does that.

One potential notice for the community is that `("a" {"a" 10})` previously worked in ClojureScript before this patch (though not on the jvm).



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

fixed, http://github.com/clojure/clojurescript/commit/2cef52840fcd9d88f077d374cc81e50c7e645b9f
http://github.com/clojure/clojurescript/commit/2e8b32aa28399c69500e511377b1841a6679c9f2





[CLJS-540] Switch to tools.reader for cljs.analyzer/forms-seq Created: 15/Jul/13  Updated: 29/Jul/13  Resolved: 29/Jul/13

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

Type: Enhancement Priority: Major
Reporter: Sean Grove Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: analyzer, cljs, reader, sourcemap
Environment:

CLJS master


Attachments: Text File tools_reader_with_passing_tests.patch    
Patch: Code

 Description   

Switches cljs.analyzer to use tools.reader, so we can get more accurate column location for symbols, prepping for more fleshed-out source maps.



 Comments   
Comment by David Nolen [ 15/Jul/13 10:43 AM ]

Excellent, we need two more things in this patch, can you update bootstrap.sh and the POM file? Thanks.

Comment by Sean Grove [ 15/Jul/13 11:00 AM ]

Updated with POM information and bootstrap update

Comment by Sean Grove [ 22/Jul/13 2:20 PM ]

CA's been processed and I'm listed on the contributing page, so shouldn't be blocked by that any longer.

Comment by David Nolen [ 27/Jul/13 2:09 PM ]

I tried applying this patch and rerunning the bootstrap script. This works but when I try to run script/test I get an error about the tools.reader not being on the classpath. Even trying to require tools.reader at via the repl doesn't work for me.

Comment by David Nolen [ 29/Jul/13 11:14 AM ]

fixed http://github.com/clojure/clojurescript/commit/9f010ff5d4a122b0f1dc93905647f309cc45c699





[CLJS-487] Make cljsc copy js sources to public directory Created: 16/Mar/13  Updated: 29/Jul/13  Resolved: 29/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: John Chijioke Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: cljs, cljsc, js
Environment:

Linux jetty web server



 Description   

I'll like cljsc to copy my js sources to the public directory as it does when compiling the cljs sources and make the correct entry in the generated deps.js so that the source can be retrievable with GET without extra configuration.



 Comments   
Comment by David Nolen [ 29/Jul/13 11:23 PM ]

enhancements to the build process at this point are unlikely outside of source maps and enabling external tools that can implement enhancements like this themselves.





Generated at Fri Oct 31 16:36:17 CDT 2014 using JIRA 4.4#649-r158309.