taoensso / tower

i18n & L10n library for Clojure/Script
https://www.taoensso.com/tower
Eclipse Public License 1.0
277 stars 24 forks source link

Compilation of tower.cljs fails with ClojureScript 0.0-2913 #58

Closed robert-stuttaford closed 9 years ago

robert-stuttaford commented 9 years ago
> NS form of taoensso/tower.cljs can't be parsed: unrecognized ns part

clojure.lang.ExceptionInfo: unrecognized ns part 
{:form (ns taoensso.tower "Tower ClojureScript stuff - still pretty limited." 
{:author "Peter Taoussanis"} (:require-macros [taoensso.tower :as tower-macros]) 
(:require [clojure.string :as str] [taoensso.encore :as encore])), 
:part {:author "Peter Taoussanis"}}
ptaoussanis commented 9 years ago

Hey Robert,

I've been running into a fair number of bugs with ClojureScript releases lately, so have started lagging by 3-4 versions in my own production environments. To be expected I think, it's undergoing pretty aggressive development (bringing new, good stuff!).

Not familiar with this error in particular, or possible causes. If the usual stuff doesn't help (lein clean, lein deps :tree to check for dep conflicts) - then it may be a bug or breaking change in ClojureScript?

On some high-priority work atm, so won't have any time to look into this myself right now. I'd maybe check the Clojure/Script Google group to see if there's any reports of similar errors, and go from there?

Would be happy to accept a PR if you can identify some specific change on Tower's end that would help. Otherwise sorry I couldn't be of more assistance right now. Good luck, cheers! :-)

robert-stuttaford commented 9 years ago

OK, no problem, Peter, I just thought I should note that there is an issue with that Cljs version. I suspect it's the {:author ...} hash that's confounding it. I'll verify in my own code and if I do, I'll let the Cljs folks know as well. We'll get it sorted one way or another!

ptaoussanis commented 9 years ago

Any update on this Robert? Will be cutting a new Tower release shortly so this would be a good time to make changes here if any are necessary.

robert-stuttaford commented 9 years ago

I tested with one of our own .cljs files, and found that a doc string OR a meta map is fine, but not both together.

To resolve this, I think you can remove the author map from your namespaces. The git history will always prove you as the author :-)

ptaoussanis commented 9 years ago

Hey Robert,

Not sure why you're seeing this, a doc-string followed by an attr-map is (or should be) valid. It certainly is in Clojure and versions of ClojureScript I have in prod. Any chance this is a new ClojureScript change/bug?

To resolve this, I think you can remove the author map from your namespaces. The git history will always prove you as the author :-)

I'm not concerned about the authorship, but about the ns form working as intended :-)

robert-stuttaford commented 9 years ago

Ok. I'll check with David what the next step is.

ptaoussanis commented 9 years ago

Sure, thanks. If this turns out to be an intentional breaking change for some reason, I'll update Tower (and all my other Cljs libs, which have the same ns form). Would prefer we ascertain first that this isn't a compiler bug or tooling issue though.

Will keep the GitHub issue open for now until we have a conclusion?

Thanks again for doing all the legwork on this.

swannodette commented 9 years ago

ClojureScript has never supported this kind of ns form, nothing specific to 0.0-2913. Enhancement patch of course welcome.

ptaoussanis commented 9 years ago

Hi @swannodette, thanks for chiming in. To be clear: we're talking about ns forms with both a doc-string and attr-map?

I have a lot of ClojureScript code with forms like this that compile fine - never had an issue with them before. This particular file is compiling fine with 0.0-2505, for example.

Is it possible these forms happened (unintentionally) to compile okay with older Cljs compilers and that something's changed recently to now mean the accidental support has broken?

swannodette commented 9 years ago

@ptaoussanis the relevant commits go back to 2012 and 2013. Neither appear to support this type of ns form. The fact that it compiled "fine" is completely accidental as far as I can tell, such ns forms would have always discarded information even if they did not throw an error.

ptaoussanis commented 9 years ago

@swannodette Thanks David, appreciate the quick confirmation.

@robert-stuttaford Okay- going to need a little time to update. Have a few files (incl. supporting libs) that I have to check. Is this an urgent fix for you?

robert-stuttaford commented 9 years ago

@ptaoussanis not at all urgent. At your convenience :-)

swannodette commented 9 years ago

@ptaoussanis actually I read the intent of the parse 'ns source incorrectly (it's pretty old stuff that I didn't work on), https://github.com/clojure/clojurescript/blob/de130a335bd761d580d04dadb8a5a43d3c0a35b4/src/clj/cljs/analyzer.clj#L1311

It looks OK to me now. While it does not appear to me the logic has changed recently I'm not certain. Also the error in this issue is not particularly informative. No stacktrace and the error message is not something that ClojureScript itself produces. Can we get something with more information @robert-stuttaford? Thanks.

thheller commented 9 years ago

Sorry folks, my bad.

Recently wrote a new parse-ns implementation for shadow-build which didn't recognize the docstring+meta bits in the ns. Wasn't aware this was allowed as I had never seen that before. Fixed in alpha9.

robert-stuttaford commented 9 years ago

Thanks @thheller and thanks @swannodette and @ptaoussanis for your attention on this!

ptaoussanis commented 9 years ago

@thheller @robert-stuttaford No problem at all, thanks for chasing this up.