<< Back to previous view

[TNS-35] Read/analyze ClojureScript and conditional read sources Created: 24/Jul/15  Updated: 03/Nov/15  Resolved: 03/Nov/15

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: None


 Description   

Clojure 1.7 and ClojureScript 3308 introduced support for the Conditional Reader.

tools.namespace started out as a Clojure-only library. Support for the conditional reader was added in TNS-34, but it was hard-coded for Clojure (.clj and .cljc) files only.

Some high-level features of tools.namespace, such as code reloading via c.t.n.repl/refresh, are coupled to the Clojure(JVM) implementation, and could not easily be ported to ClojureScript. I regard that as a non-issue for now, since other tools (e.g. Figwheel) exist to do reloading in ClojureScript.

However, other features of tools.namespace, such as namespace dependency analysis, do not depend specifically on Clojure(JVM). It might be useful to apply those tools on ClojureScript source files.

The deeper problem is the multi-layered structure of tools.namespace. The Clojure/ClojureScript distinction only really matters in the "lower" layers such as c.t.n.file and c.t.n.parse, but the layered structure makes it awkward to pass options through from the "higher" layers such as c.t.n.dir and c.t.n.find.

See also comments on TNS-29.

This ticket is a place to capture work and notes about this problem.



 Comments   
Comment by Stuart Sierra [ 24/Jul/15 3:23 PM ]

Breakdown by namespace:

c.t.namespace is deprecated

c.t.n.dependency has no JVM-specific coupling, can be trivially ported to .cljc

c.t.n.track has no JVM-specific coupling, can be trivially ported to .cljc

c.t.n.parse has read-ns-decl calling clojure.core/read, which always uses default platform feature :clj. Can be modified to support reading .cljc files with the :cljs feature by replacing clojure.core/read with tools.reader.

c.t.n.file defines clojure-file? hard-coded to .clj and .cljc since TNS-34. Would be trivial to add clojurescript-file?.

c.t.n.dir calls c.t.n.file/clojure-file?

c.t.n.repl calls c.t.n.dir/scan

c.t.n.find calls c.t.n.file/clojure-file? and also uses hard-coded .clj and .cljc as file extensions to search within JAR files.

c.t.n.move is hard-coded to .clj files but is still "alpha" and not widely used

Comment by Laurent Petit [ 24/Jul/15 6:01 PM ]

couldn't `read-ns-decl` call `clojure.core/read` with `:read-cond` and `:features` options set appropriately? Is it really necessary to add `tools.reader` as a dependency (I would avoid it if possible)?

Comment by Stuart Sierra [ 25/Jul/15 10:10 AM ]

The reason you can't just pass different options to clojure.core/read is because, on Clojure(JVM) read will always add the platform feature :clj. There is no way to call read and force it to omit the platform feature.

See the Reader Conditionals design page, "The platform feature (:clj) will always be present."

Comment by Laurent Petit [ 25/Jul/15 3:10 PM ]

Oh, I didn't know that. It's kind of a shame, no, since it is well known that ClojureScript is compiled via Clojure (JVM) ...

Comment by Stuart Sierra [ 25/Jul/15 6:51 PM ]

As far as I know, ClojureScript has used tools.reader instead of clojure.core/read for some time.

Comment by Stuart Sierra [ 26/Jul/15 10:00 AM ]

My current thinking is:

c.t.n.dependency and c.t.n.track are platform agnostic.

c.t.n.file and c.t.n.parse can be extended to support Clojure & ClojureScript by adding an optional argument read-opts passed through to tools.reader/read.

c.t.n.find can be extended with optional arguments to select a "platform," either Clojure or ClojureScript, which will encapsulate both valid file extensions and reader options.

Reload/refresh functionality will remain Clojure(JVM) only for now: c.t.n.dir, c.t.n.reload, and c.t.n.repl.

c.t.n.move is still alpha and does not even support .cljc files. It can be handled under separate tickets or deprecated.

Work-in-progress visible on branch TNS-35-cljs-experiments currently at commit e9327295

Comment by Stuart Sierra [ 03/Nov/15 11:55 AM ]

.cljc support for read/parse/dependencies in 0.3.0-alpha* release





[TNS-36] Use java.classpath Created: 26/Jul/15  Updated: 06/Nov/15  Resolved: 06/Nov/15

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-TNS-36-Use-java.classpath.patch     Text File 0001-TNS-36-Use-java.classpath.patch    
Patch: Code

 Description   

Past versions of tools.namespace copied some functions (e.g. one, two) from java.classpath to avoid a dependency.

However, since the addition of CLASSPATH-5, tools.namespace has been missing some functionality necessary to fully resolve the classpath in some environments.

The purpose of this ticket is to 1) add a dependency on java.classpath; and 2) replace the duplicated functions in tools.namespace with their equivalents in java.classpath.

No public APIs in tools.namespace will be affected.



 Comments   
Comment by Stuart Sierra [ 06/Nov/15 2:13 PM ]

Included in 0.3.0-alpha1 release





[TNS-38] circular dependency when CLJS namespace requires CLJ for macros Created: 03/Nov/15  Updated: 05/Jan/16  Resolved: 05/Jan/16

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 2
Labels: None


 Description   

When a .cljs file defines a namespace with :require-macros of the same namespace name as a .clj file, tools.namespace reports a circular dependency error.

See https://github.com/clojure-emacs/refactor-nrepl/issues/127 for example



 Comments   
Comment by Stuart Sierra [ 05/Nov/15 2:21 PM ]

Possible short-term fix: just ignore the special case of a cljs namespace with :require-macros on itself.

Comment by Stuart Sierra [ 06/Nov/15 3:05 PM ]

Or maybe :require-macros shouldn't be parsed at all, since it is crossing the boundary between .cljs and .clj

Comment by Stuart Sierra [ 06/Nov/15 4:44 PM ]

Short-term fix in commit 6b19f942

Comment by Stuart Sierra [ 13/Nov/15 9:26 AM ]

Included in 0.3.0-alpha2 release

Comment by Stuart Sierra [ 28/Nov/15 9:07 AM ]

Reopening. Solution in -alpha2 is inadequate, see commit message at commit 149f4650

Comment by Stuart Sierra [ 05/Jan/16 7:50 AM ]

Fixed in commit 5d6957d by ignoring :require-macros when analyzing dependencies. I believe this is the correct solution for now: tools.namespace will treat Clojure and ClojureScript as separate worlds, and will not attempt to analyze dependency relationships which cross that boundary.





[TNS-5] Allow any valid .clj* source file to be parsed/analysed Created: 01/Nov/12  Updated: 05/Nov/15  Resolved: 05/Nov/15

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: enhancement

Attachments: Text File e3cd6d1fa6e0c900bc1086e4a93bbc9cb343a820.patch    

 Description   

This broadens the allowed file types to anything ending with #"\.clj.?$", meaning this would work for clj, cljs, cljc, cljx and possibly other Clojure implementations with their own extension in the future.

This allows libraries such as codox (and possibly autodoc) to work with ClojureScript and others implementations without any modification.

Note: My CA is on the way, I sent it a week ago, I am not sure if it arrived yet (it was sent from Switzerland with normal mail, with an ETA of 1 week).



 Comments   
Comment by Max Penet [ 02/Nov/12 6:51 AM ]

CA received it seems (I am listed on http://clojure.org/contributing ).

Comment by Stuart Sierra [ 02/Nov/12 3:23 PM ]

I'm not sure about this. If you're only using c.t.n.find in isolation, it's fine. But if you're using code-reloading and c.t.n.repl, it could incorrectly try to reload .cljs files in JVM Clojure.

We really need [Feature Expressions]http://dev.clojure.org/display/design/Feature+Expressions or something like it to get away from multiple file extensions.

Until then, I think it has to be optional. I don't know how best to achieve this. The APIs in c.t.n.repl and c.t.n.dir are not amenable to extension. I'll think about it.

Comment by Max Penet [ 06/Nov/12 6:11 PM ]

True, I didn't realize that.

Maybe using a dynamic var to hold the regex pattern (or a predicate?) could be a reasonable solution in the meantime, this would allow to rebind it in the case of codox & similar libs.
I don't really "like" to use dynamic vars (or regexes!), but in this case it might make sense.
Not to mention it would also allow more flexibility on projects where you don't want to have your clj codoxed, but only your cljs for instance.

Comment by Max Penet [ 24/Nov/12 7:47 AM ]

Any thoughts on my last comment/edit? I don't think there is a single doc lib that works with cljs at the moment, it is a bit painful to be honest.

Comment by Stuart Sierra [ 24/Nov/12 4:25 PM ]

Yes, I think a dynamic var would be OK. However, I would like to know of a real use case, not just a potential one.

Comment by Max Penet [ 13/Dec/12 2:40 AM ]

The idea was to be able to use codox on cljs files, I tried locally but there are other problems with this approach to be able to get to cljs vars metadata. So in the end I think you were right, maybe it's better to wait for feature expressions for this.

Comment by Stuart Sierra [ 05/Nov/15 2:20 PM ]

.cljs and .cljc are supported as of 0.3.0-alpha1 release





[TNS-40] Don't catch all exceptions in read-file-ns-decl Created: 24/Nov/15  Updated: 05/Jan/16  Resolved: 05/Jan/16

Status: Resolved
Project: tools.namespace
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Defect Priority: Minor
Reporter: Daniel Compton Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None


 Description   

I spent a while trying to work out where a bug was coming from after upgrading tools.namespace. Eventually I tracked it down to an exception being thrown in parse/read-ns-decl when it calls reader/read. This was an error in tools.namespace code, not the parsing code. The error was an ArityException that was occurring because the wrong version of tools.reader was on the Classpath.

Is it desirable to catch all exceptions here, or would it be better to only catch exceptions thrown by the parsing (which seems to be the intent)? I'm not sure if it's possible to distinguish these though...

Thoughts?



 Comments   
Comment by Stuart Sierra [ 24/Nov/15 8:02 AM ]

I've never been entirely happy with the behavior of ignoring exceptions. But the alternative has been to break on any syntax error in any file. Since it's fairly common to have syntax errors during development, the trade-off was to allow tools.namespace to continue working.

In 0.3.0 I moved the try/catch one level higher from c.t.n.parse/read-ns-decl to c.t.n.file/read-file-ns-decl, specifically to detect this case.

With the tools.reader replacing clojure.core/read in tools.namespace 0.3.0, it may be possible to distinguish parsing errors from other kinds of errors.

Comment by Stuart Sierra [ 05/Jan/16 8:05 AM ]

It appears that tools.reader wraps all exceptions in ex-info with :type :reader-exception

Comment by Stuart Sierra [ 05/Jan/16 2:32 PM ]

fix included in 0.3.0-alpha3 release





Generated at Fri Feb 12 12:20:46 CST 2016 using JIRA 4.4#649-r158309.