General Questions

[martinklepsch]

First of all: thanks for all the effort that went into this library, I'm a huge fan of interceptors and I've long wanted a .cljc implementation that is still somewhat close to the Pedestal implementation.

Some questions came up when taking a look at Sieppari. I was thinking of discussing those with @ikitommi but I figured I might also just open an issue.

1. Why no namespaced keys for :stack and :queue?

In Pedestal they're namespaced and that seems like a solid way to save users from accidentally overwriting the execution queue or stack.

2. Why the added request concept?

In Pedestal the model is very consistent:ctx in, ctx out. In Sieppari there is an additional concept of request that doesn't seem to have a clear meaning in the context of interceptor execution and, from my current perspective, adds complexity by introducing additional terminology/concepts.

For instance the example given in the Readme. The inc-x-interceptor receives the full context whereas the handler just receives the value of the :request key. At a first reading I thought supplying a function would be a shorthand for an :enter-only interceptor but that's not the case. It seems like the idea behind this is to be closer to the Ring model but I'm not sure if that's the only reason?

I found Pedestal's "context in, context out" model perfectly fine and never saw the need for additional nesting at the level of the interceptor library.

;; Example from the Readme
(def inc-x-interceptor
  {:enter (fn [ctx]
            (update-in ctx [:request :x] inc))})

;; handler, take `:x` from request, apply `inc`, and return an map with `:y`
(defn handler [request]
  {:y (inc (:x request))})

(sieppari/execute
  [inc-x-interceptor handler]
  {:x 40})

With the :request concept removed the example above would look like this which, to me, is much simpler since the only thing that is ever relevant is the context.

(def inc-x-interceptor
  {:enter (fn [ctx]
            (update ctx :x inc))})

;; handler, take `:x` from request, apply `inc`, and return an map with `:y`
(defn handler [ctx]
  {:y (inc (:x ctx))})

(sieppari/execute
  [inc-x-interceptor handler]
  {:x 40}) ; {:y 41}

[ikitommi]

Excellent points. The :queue and :stack were originally unqualified to support non-sieppari specific interceptor-libs and because of the :request & :response pattern, handlers don't see the context so easier not to break 'em. But, there hasn't been any need for dynamic queueing in reusable interceptor libraries. A client project anyway chooses a executor lib (pedestal, sieppari, tripod etc) and could use it's own helpers to manipulate the queue.

Pedestal has the special case for function->handler, #14 revisits that.

0.0.0-alpha6 allows any changes. I'll talk with Jarppe.

[ikitommi]

... reitit uses the dynamic queuing, but there are reitit-sieppari and soon reitit-pedestal that are anyway specific to the interceptor runner.m

[martinklepsch]

But, there hasn't been any need for dynamic queueing in reusable interceptor libraries.

I've been using dynamic queuing (inspecting, and modifying the queue during execution) with Pedestal for a non-HTTP problem and believe that they are part of the proposition of interceptors — so I would strongly favour a library that exposes queue and stack. I know Sieppari is currently doing this but I just wanted to explicitly voice support for this feature.

because of the :request & :response pattern, handlers don't see the context so easier not to break 'em.

I see that this is desirable but I'm inclined to say that it shouldn't be part of the interceptor implementation itself — perhaps something like this could be built in top of a "context in, context out" bare-bones implementation for those who work on problems where this pattern is useful... something like sieppari.request-response?

[ikitommi]

My 2 cents:

1. public api should be ctx => ctx (or with success & error callbacks)
2. for http, there would be :request and :response keys in the context
3. stack and queue => no need to be part of the imaginary interceptor spec, so could be :sieppari/queue & :sieppari/stack too

For the 2 & 3 - as Sieppari tries to be a fundamental core library (e.g. in reitit), we would like to make it as fast as possible. Because of this, the context is internally a sieppari.core/Record, which gives a big perf boost with predefined & non-namespaced keys. There could be a RequestResponseContext record with :request and :response fields, but the stack & queue need some way to be fast too.

[ikitommi]

What about:

(ns user
  (:require [clojure.core.async :as a]
            [sieppari.core :as s]))

(defn interceptor [k f n] 
  {:enter #(update % k (fnil f 0) n)})

(defn async-interceptor [k f n]
  {:enter #(a/go (update % k (fnil f 0) n))})

;;
;; synchronous
;;

(-> (s/context)
    (s/enqueue [(interceptor :n + 10) (interceptor :n * 2)])
    (s/run)
    :n)
; 20

;;
;; async
;;

(-> (s/context)
    (s/enqueue [(async-interceptor :n + 10) (async-interceptor :n * 2)])
    (s/run))
; #object[clojure.core.async.impl.channels.ManyToManyChannel]

(-> (s/context)
    (s/enqueue [(async-interceptor :n + 10) (async-interceptor :n * 2)])
    (s/run)
    (s/await)
    :n)
; 20

;;
;; sync, revisited
;;

(-> (s/context)
    (s/enqueue [(interceptor :n + 10) (interceptor :n * 2)])
    (s/run)
    (s/await)
    :n)
; 20

[martinklepsch]

That looks nice to me :+1:

[jjttjj]

Just wondering if there's been any further thought on this. I also am in a situation where I'd like to use an interceptor library instead of rolling my own, and an "async agnostic" library like sieppari would be preferable to pedestal, but I'm not doing http and the request/response model is awkward for me to fit into. A ctx => ctx api would be perfect for me.

[ikitommi]

This is the way to go and we have a working code for this but the tests don't pass yet. Tested a version where the Context would be even slimmer, just a protocol instead a map-like thing (map would implement the Context as one option). With this, we get stellar performance with it but would ripple even more changes to the current codebase.

I propose to do a Sieppari 1.0.0 epic with all the changes in it as issues. We could collect the epic this week, lot's of small changes, all for the better. Effect all users of reitit/http so should be good from day1.

[ikitommi]

would be the first to go from 0.0.0-alpha7 to 1.0.0 :)

[den1k]

Any updates on the removing the :request / :response pattern? I'd love to use sieppari as part of the public API in a (soon to be released) OSS project. However, request/response makes this problematic.