r0man / ring-cors

Ring middleware for Cross-Origin Resource Sharing.
http://github.com/r0man/ring-cors
166 stars 44 forks source link

Make it easier to use a different response-handler. #18

Closed stoyle closed 7 years ago

stoyle commented 7 years ago

In order to reuse ring-cors with aleph, which provides async response handling, i.e. with manifolds, splitting the functions up this way makes it way easier, to reuse bigger parts.

The library aleph-middleware does this for a few ring-middlewares but not ring-cors.

Currently I have to do a the following, with not too much reuse:

(defn wrap-cors [handler & access-control]
  (let [access-control (cors/normalize-config access-control)]
    (fn [request]
      (if (and (cors/preflight? request) (cors/allow-request? request access-control))
        (let [blank-response {:status 200
                              :headers {}
                              :body "preflight complete"}]
          (cors/add-access-control request access-control blank-response))
        (if (cors/origin request)
          (if (cors/allow-request? request access-control)
            (if-let [response (handler request)]
              (d/chain'
                response
                #(cors/add-access-control request access-control %)))
            (handler request))
          (handler request))))))

The only change I need is in the if-let. With this change I can do the following:

(defn async-response-handler [request access-control response]
  (d/chain' response #(cors/default-response-handler request access-control %)))

(defn wrap-cors [handler & access-control]
  (let [access-control (cors/add-access-control access-control)]
    (fn [request]
      (handle-cors handler request access-control async-response-handler))))

I am, of course, open for other suggestions, if this is a change you are willing to do.

r0man commented 7 years ago

@stoyle I'm open to support Aleph, but not too familiar with it. Ring recently added support for async handlers [1]. So my suggestion would be to implement the wrap-cors handler in that style (an additional 3 arity fn). Would that also work for you & Aleph? That way we don't have to expose another public fn like handle-cors, but make the library ready for async use case with Ring. Does that make sense?

[1] https://github.com/ring-clojure/ring/blob/master/SPEC

stoyle commented 7 years ago

Definitely agree, this change should be compatible with the proposed Ring spec. But, I am not entirely sure I understand your suggestion.

The 3 arity function would be (fn [request cb-response cb-exception] ...) or something like that? Or is the spec talking about changing the signature of wrap-cors itself?

If I am able to utilize cb-callback to handle and return the response, asynchronously, e.g. just by putting using async-response-handler, then I would be good.

Just to be clear. All I need is to be able to handle the response, and I would like to reuse as much as possible from ring-cors.

On a sidenote, I see my last example contains a few errors though, sorry. Two changes, add-access-control inside async-response-handler and normalize-config to create access-control in wrap-cors. It should have been:

(defn async-response-handler [request access-control response]
  (d/chain' response #(cors/add-access-control request access-control %)))

(defn wrap-cors [handler & access-control]
  (let [access-control (normalize-config access-control)]
    (fn [request]
      (handle-cors handler request access-control async-response-handler))))

Shouldn't be coding in a github textarea :) The actual commit itself is correct.

r0man commented 7 years ago

@stoyle Ok, got it. I merged this and pushed 0.1.10 to Clojars.

stoyle commented 7 years ago

Wow, that was quick, thanks. Already tested in production :)

r0man commented 7 years ago

Cool :)