Moving the io.pedestal.interceptor package over to cljc files

[claj]

Description

According to @ohpauleez , one outstanding issue for releasing Pedestal 0.5.2 is to convert the io.pedestal.interceptor package to cljc. I'll try to make it clear what the main purpose of the implementation and which of the JVM-specific parts of the implementation is worth to spend time on.

I'm not super-skilled in ClojureScript nor CljC, so keep an eye out for strange assumptions.

Previous work

The popular Re-frame project has a surprisingly compact implementation that works as Pedestal's interceptors in interceptor.cljc.

Expected Behavior(?)

Is the aim to make pedestal work in webservers as node.js, or is this mainly a way to be able to use parts of pedestal (url-for, client-side request handling/pausing) in a client application?

Is the interceptor logic to be used with terse-routing syntax?

Implementation decisions

The interceptor namespace consists of four namespaces, io.pedestal.interceptor, .chain, .helpers, .error.

I have looked through the source code and have some questions on how to solve some issues.

io.pedestal.interceptor

(:19) What is the equivalent of print-method in ClojureScript?

Should we keep the backward compability around :interceptor, :interceptorfn in (:37) for the clojurescript version?

The places using valid-interceptor? (:68) begs for clojure.spec, but I guess that's outside the scope for now.

The implementations of IntoInterceptor for IPersistentList and Cons uses eval which, I think, is mostly used in the terse route syntax, which I think is something pedestal is abandoning. Eval is probably possible to use in ClojureScript, but it can come with potentially high costs in advanced compilation. I'm not sure about the resolve in the implementation for Symbol in ClojureScript either. Should the IntoInterceptor perhaps only accept IPersistentMap and Interceptor in the ClojureScript version?

Should we consider to move the more esoteric implementations of IntoInterceptor to the terse-routing namespaces?

.chain

Contains the JVM-specific AtomicLong. I assume this could use an atom just keeping a number in the ClojureScript version.

There is a dependency on io.pedestal.log. Should io.pedestal.log be converted to cljc as well, probably using js.console/log?

I'm ignorant to what's the equivalent of Throwable (:74) is in js.

.error

Nothing JVM-specific here.

.helpers

defsimpleinterceptordef

I think the defsimpleinterceptordef constructs was said to be deprecated at one time or another.

I suggest we don't port the defsimpleinterceptordef to ClojureScript because it's not used in other parts of pedestal.

Ring-targeted functions

The various functions for wrapping ring middleware and handlers (middleware, on-request, on-response) seems less usable in ClojureScript, if not a lot of ring (or other) libraries has been ported already. It's certainly no big deal to just lift them over, but I think it's worth considering.

Or do these constructs increase the usability of client libs like cljs-http, server libs like espresso?

I suggest we keep all the functions for simplicity in later porting to clojurescript.

The argument parsing logic begs for being done with clojure.spec, but I guess that's outside the scope.

Dependencies

The ClojureScript version should most likely be latest stable release, currently 1.9.293.

Pedestal version

Targeted for pedestal 0.5.2

[martinklepsch]

Contains the JVM-specific AtomicLong. I assume this could use an atom just keeping a number in the ClojureScript version.

Since JS is single threaded you can use a simple volatile! (like an atom but without watches and all the extra fluff).

I'd be very interested in helping with this if help is wanted.

[martinklepsch]

Another thing that probably can not be reproduced in CLJS is anything related to bindings. As soon as things get async in ClojureScript bindings will be lost. I'd suggest to communicate that as a "missing feature" in the ClojureScript version.

[claj]

@martinklepsch I have not had the opportunity to do any work on this yet, so feel free to go ahead. The volatile! sounds like a good idea to me.

Are you more familiar with any work on ring-like wrappers for node.js? I'm a bit unsure wheter the work to port various middleware-to-interceptors functions makes sense. I guess it's good to do it.

[madbonkey]

I'm a bit unsure wheter the work to port various middleware-to-interceptors functions makes sense. I guess it's good to do it.

I agree that it makes sense. In porting them, I think there's a lot of room for up-front design thoughts about getting pedestal's routing awesomeness to the client.

What's the status of this issue, anyway? Long time without updates - I am super happy seeing this being considered, however :)

[hlship]

My two cents - we should not be concerned with any of those helper functions; IMHO they add syntax and abstraction with very little benefit.

[hlship]

This is obviously very old and very low priority; is there a modern justification for this? Essentially, it's "I want to use interceptors in Node", and that's not a very compelling case, especially because of the interaction between the interceptors, core.async, and a dash of logging.

[hlship]

This issue has been open for quite some time. We apologize if it has not received the attention it deserves; given the amount of time that has passed, we are focusing on other urgent issues. We are closing this issue for the meantime, please re-open if it is still relevant.