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

Add partial ClojureScript support for building queries #26

Closed
wants to merge 11 commits into from
Closed

Add partial ClojureScript support for building queries #26

wants to merge 11 commits into from

Conversation

danielcompton
Copy link
Collaborator

This isn't ready to merge, I'm opening it for discussion and feedback while it's WIP.

The project I'm working on needs to generate RethinkDB queries in the browser with ClojureScript, then send them to a server to be evaluated. This pull request implements partial ClojureScript support for the query portion of clj-rethinkdb.

I know you said that you weren't interested in ClojureScript support in #21, but wondered if this would route would be acceptable to you. Another possibility would be splitting the project in two, making the query building Clojure/ClojureScript compatible, and leaving the network communications part in Clojure. Unless you were going to use ClojureScript on NodeJS I'm not sure that it would be possible or make sense to port this whole library to ClojureScript.

What are your thoughts?

Shortcuts I took to get this working:

  • Commenting out time related queries because I haven't picked a clj-time replacement for the ClojureScript port (cljs-time looks promising though).
  • I hard coded the request and term types by copying them from the protocol buffer file. We need a better way to read this. Possibly using the protodef.js translator?
  • Handle UUID's. It looks like this might be the best option. I would have expected something from Google Closure for this though. Also worth looking at this gist
  • Properly handle fn's
  • Check Regexs are correct, probably need platform specific versions

@apa512
Copy link
Owner

apa512 commented May 24, 2015

I think this is outside the scope of this library. Just requiring the needed parts in another library is probably better.

@danielcompton
Copy link
Collaborator Author

Hi @apa512, to be able to use the Query parts of your library, they either need to be copied into .cljs files with the parts that don't work removed, or add Reader Conditionals or cljx to let the Clojure and ClojureScript compilers only see the parts they need.

Probably the best and cleanest solution would be to:

  • Rename the .clj files related to querying to .cljc files
  • Add the necessary reader conditionals to the query files to avoid the ClojureScript compiler tripping up on Java parts.

With those two things done, we could require this library into our own library and add any other code we needed in there.

Would that work for you?

@apa512
Copy link
Owner

apa512 commented May 30, 2015

I don't like cljx (more dependencies and annoying compile step) but cljc seems fine. that's still a very experimental feature though?

@danielcompton
Copy link
Collaborator Author

CLJC is a new feature in Clojure 1.7 which is expected to be released quite soon. So it's pretty new, although it's probably past the experimental stage though. However it isn't backwards compatible with Clojure 1.6 or below, so it may make more sense to maintain this in a parallel branch until you're happy to drop 1.6 support.

@danielcompton
Copy link
Collaborator Author

Continuing in #59

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

Successfully merging this pull request may close these issues.

None yet

2 participants