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

.cljc conversion for ClojureScript use #12

Open
cemerick opened this issue Sep 7, 2017 · 5 comments
Open

.cljc conversion for ClojureScript use #12

cemerick opened this issue Sep 7, 2017 · 5 comments

Comments

@cemerick
Copy link
Contributor

cemerick commented Sep 7, 2017

Looks like this will be pretty straightforward.

My preference would be to move the rendering bits into a new dorothy.jvm namespace. That's obviously a breaking change, but it would leave room for node- and browser-specific namespaces for rendering in those environments later. If you'd prefer not to do that, let me know and I'll keep the jvm rendering stuff where it is.

@cemerick
Copy link
Contributor Author

cemerick commented Sep 8, 2017

This is effectively done, over on this branch:

https://github.com/cemerick/dorothy/tree/cljc

I'll not open a PR before hearing re: whether the split of rendering stuff into a dedicated namespace is acceptable. Either way, I'll then update the documentation and such before then opening a PR.

@daveray
Copy link
Owner

daveray commented Sep 8, 2017

Hey Chas. I'm totally open to these changes. I trust your judgement and since I haven't really worked on or used this code in quite a while, I don't really have a strong opinion. Would the upgrade path for existing code just be changing a :require to account for the new namespace?

@cemerick
Copy link
Contributor Author

cemerick commented Sep 8, 2017

Yup, it'd be changing e.g.:

(ns foo
  (:require [dorothy.core :as dot :refer (render)]))

to:

(ns foo
  (:require [dorothy.core :as dot]
            [dorothy.jvm :refer (render)])))

@daveray
Copy link
Owner

daveray commented Sep 8, 2017

Works for me.

@cemerick
Copy link
Contributor Author

cemerick commented Sep 9, 2017

Opened the PR. Some things you'll need to do I can't:

  • change the project description to indicate the lib is usable from ClojureScript
  • activate the repo in travis so CI builds will proceed, and update the README badge appropriately
  • when you're ready, cut a release to clojars (I've deployed a build from my fork to [org.clojars.cemerick/dorothy "0.0.7-cljs"] if you want to try it out first)

I'm now using the results of this with some nontrivial graphs, and it all looks 👍 FWIW

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants