lilactown / punk

A data REBL built for the web
Eclipse Public License 2.0
148 stars 5 forks source link

Attempt at a JVM adapter #12

Closed sogaiu closed 5 years ago

sogaiu commented 5 years ago

Here's some initial work on a JVM adapter including an "example". As discussed elsewhere, the work contains minor changes to punk.core and frame for use on the JVM. The network portion is done with aleph, manifold, and ring. The node adapter code and example were very helpful.

It uses deps.edn and thus the included "example" should be testable via a clj REPL:

(require 'punk-example.core)

or just skip the example and go for:

(require '[punk.adapter.jvm :as paj])
(paj/start)

Then, connect to http://localhost:9876 via a browser, and from the REPL:

(tap> {:expression :smile :bag [:hammock :notebook :pencil :thermos]})

If we are still on the happy path, the browser should be displaying appropriately :)

Caveats and Notes:

sogaiu commented 5 years ago

BTW, I briefly tested the use of punk.adapter.jvm in an existing deps.edn-using test project and initially encountered an issue in getting it to work.

The test project's deps.edn had an alias that looked like:

 :punk {:extra-deps {
                      org.clojure/clojure {:mvn/version "1.10.0"}
                      local/punk.core {:local/root "../punk.local/core"}
                      local/punk.frame {:local/root "../punk.local/frame"}
                      local/punk.adapter.jvm {:local/root "../punk.local/adapter-jvm"}
                    }
       }

where ../punk.local is where a local checkout of the adapter-jvm branch lives.

Upon invoking clj, the type of error message encountered was like:

Error building classpath. Manifest type not detected when finding deps for frame/frame in coordinate #:local{:root "../frame"}

Modifying some of the deps.edn files in the punk repository seemed to fix / work-around the issue. Specifically, adding the key-value pair:

:deps/manifest :deps

to all instances of:

{:local/root "../core"}}

and

{:local/root "../frame"}}

With the changes made, I was able to examine values in punk for the test project after:

clj -A:punk

and then following the appropriate steps in this pull-request's first comment.

Not sure whether there is a better way to address the encountered issue. Any ideas?

lilactown commented 5 years ago

My preliminary testing is going well! AFAICT it's working as expected compared to Node.js.

Regarding your notes:

punk.core's dataficate is just a pass-through -- please check / advise

This is fine. dataficate is a workaround for certain kinds of values that don't (or can't) properly implement the Datafiable protocol, but we still want to be able to browse them. In CLJS, this is currently just exceptions, possibly JS objects. I don't know of anything in CLJ we'd need that for yet.

read-string's options map may be shabby (see ws-handler in punk.adapter.jvm) -- please also check / advise

This is a good question. I'm not sure right now if there is a benefit to actually reading the literal in vs. just letting it hang out as a tagged-literal type inside the host process. Seems to be fine for now since none of the default tagged literals have any real behavior tied to nav or datafy.

stopping the server appears problematic (get an exception) -- there is a possibly relevant issue at the aleph repository: ztellman/aleph#365 ... may be upcoming releases of aleph / manifold will fix this?

This is bothersome but perhaps not a reason to block it from being merged. Perhaps we could consider using something other than aleph, but the WS implementation is so easy that it seems worth it.

reader conditional use in punk's core.cljc and frame's interop.cljc might be improved with an aim toward readability, possibly in combination with wrapper functions

It doesn't seem too bad right now. For interop.cljc we could consider putting them in separate files (that's what re-frame does). But those two files are relatively stable and I don't expect them to grow too much over time. I'm not worried about it yet. :)

Regarding your second post:

I haven't attempted to use Punk in an external deps.edn project with a local checkout, which is probably why it wasn't smooth to do so. The deps.edn files I put there for intra-repo development, plus previously I was depending on an unreleased version of CLJS for clojure.datafy support.

It sounds like the changes you speak of to the deps.edn files aren't going to impact negatively and would setup others for doing local development with deps.edn projects, so I think it's good!

lilactown commented 5 years ago

If there's nothing else that you might want to do, I'll merge this in to master

sogaiu commented 5 years ago

Thanks for your detailed feedback and the merge.

Should I make a PR for the :deps/manifest changes to the deps.edn files?

lilactown commented 5 years ago

That would be helpful šŸ˜Š

On Tue, Feb 19, 2019, 3:51 PM sogaiu notifications@github.com wrote:

Thanks for your detailed feedback and the merge.

Should I make a PR for the :deps/manifest changes to the deps.edn files?

ā€” You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/Lokeh/punk/pull/12#issuecomment-465360100, or mute the thread https://github.com/notifications/unsubscribe-auth/ACkApPvTa7HkmLQ2RkOTQmZ9kManvpBrks5vPI3ygaJpZM4a_-oG .