Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clojurescript support #25

Open
dundalek opened this issue Dec 3, 2022 · 2 comments
Open

Clojurescript support #25

dundalek opened this issue Dec 3, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@dundalek
Copy link
Contributor

dundalek commented Dec 3, 2022

I was wondering if the tool could be also used on Clojurescript projects.

It seems that tools.namespace suuports cljs. I did a quick experiment and was able to make it parse cljs files by replacing (namespace.find/find-ns-decls-in-dir file) with (namespace.find/find-ns-decls-in-dir file namespace.find/cljs) in parser.clj. So it should be possible to expose it up with some kind of option.

Following that I encountered circular dependency issue when namespaces require macros which halts the analysis.

For example foo.cljs:

(ns foo
  (:require-macros [foo]))

or foo.cljc:

(ns foo
  #?(:cljs (:require-macros [foo]))

There does not seem to be a way to override how tools.namespace treats circular dependencies, but I found a workaround by wrapping a line in dependency.clj in try/catch and ignoring circular dependencies. I don't see much value for the analysis tool to check circular dependencies in Clojure given it is already enforces that. But perhaps there could be an option for that.

(try
  (namespace.dependency/depend graph (:name namespace) dep)
  (catch Exception e
    (if (= (:reason (ex-data e))
           ::namespace.dependency/circular-dependency)
      graph
      (throw e)))))

With that I was able to make the analysis work. What probably needs some thinking is how to expose the option. Whether to specify it globally or just per layer(s). I am thinking for full-stack projects that consist of server clj code, client cljs and shared cljc, it would be useful to check all dialects at the same time.

@fabiodomingues fabiodomingues added the enhancement New feature or request label Dec 4, 2022
@fabiodomingues
Copy link
Owner

What probably needs some thinking is how to expose the option

About this point would be when someone wants to enable parsing only one or the other, right?

Do you see any side effects of always considering both by default (clojure.tools.namespace.find/clj and clojure.tools.namespace.find/cljs)?

What comes to mind at first is just more processing consumption 🤔


BTW, if we are going to put this at the configuration level, I would put a property at the global level that by default would be both platforms, and that would also allow overwriting at the layer level.

This property looks something like this: :platform #{:clj :cljs}.

I'll open a PR as a proof of concept to see how it looks.

@dundalek
Copy link
Contributor Author

dundalek commented Dec 7, 2022

Do you see any side effects of always considering both by default (clojure.tools.namespace.find/clj and clojure.tools.namespace.find/cljs)?

What comes to mind at first is just more processing consumption thinking

I like your thinking, it does make sense to me to start with two separate passes by default.
The perf penalty is that filesystem is traversed twice, and cljc files will likely get parsed twice.
But if it turns out it will work feature-wise, it can be optimized later.

BTW, if we are going to put this at the configuration level, I would put a property at the global level that by default would be both platforms, and that would also allow overwriting at the layer level.

This property looks something like this: :platform #{:clj :cljs}.

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Backlog
Development

No branches or pull requests

2 participants