search

ngrunwald / ring-middleware-format

Ring middleware for parsing parameters and emitting responses in JSON or other formats
163 stars 49 forks source link

Pass full options down to content handlers #54

Closed nblumoe closed 5 years ago

nblumoe commented 8 years ago

Previously, the top-level options of content handlers (e.g. decoder, predicate) could not be overriden from the outside caller. With these changes this gets fixed.

Options to the decoder inside the handlers need to be passed as :decoder-options now.

nblumoe commented 8 years ago

fixes https://github.com/ngrunwald/ring-middleware-format/issues/55

Deraen commented 8 years ago

Is there a reason to break API by changing the name of option? I want to keep the API as stable as possible.

Deraen commented 8 years ago

Hmm, I see now why the name change.

This will need some consideration. I want to either keep current API working or add a compile time error if user is using old options.

Deraen commented 8 years ago

Also, if the change is done, it would be nice to change format-response API in similar way.

nblumoe commented 8 years ago

tbh I cannot see anymore, why I thought the name HAS to be changed. I mean in addition to being clear about the intent, instead of the very generic options. However, obviously for this library, not introducing a breaking API change is way more important than that.

Do you mind explaining, why it needs to be renamed? Is there any conflict somewhere?

Good point about the response handlers, will look into that.

Deraen commented 8 years ago

Well I guess it strictly doesn't need to be changed as format specific middlewares :decoder-options takes in the same kind of value as :options used to.

The really breaking change is that restful middleware :format-options now works differently.

Deraen commented 8 years ago

One option would be to leave restful middleware format-options alone and add a parallel new option. Would just need to find a new name for that...

Unlike Compojure-api, we are not using macros so I don't think it's possible to do compile time option validation. I guess runtime validation would be nearly as good as middlewares are initialized when starting the app. But then, I'm not sure how to differentiate old and new format-options values for validation.

nblumoe commented 8 years ago

@Deraen I had a look at the response side of things. Creating the writers and handling options is a bit different there.

What I ended up with now, is just passing a new option writer-fn like this:

(wrap-restful-format :formats [:transit-json :json-kw]
                           :response-options {:transit-json {:writer-fn (fn [out _ options] (om/writer out options))}})

Being picked up in format_response.clj like this:

(defn ^:no-doc make-transit-encoder
  [fmt {:keys [verbose writer-fn] :as options}]
  (fn [data]
    (let [out (ByteArrayOutputStream.)
          full-fmt (if (and (= fmt :json) verbose)
                     :json-verbose
                     fmt)
          wrt ((or writer-fn transit/writer) out full-fmt options)]
      (transit/write wrt data)
      (.toByteArray out))))

The same would be possible on the params side: Instead of replacing the whole decoder, just pass a reader constructor down to make-transit-decoder.

As you can see, I want to use the transit writer provided by om.next and I can do so with this approach. So at least for my use case, this would be sufficient and not be a breaking API change.

However, the options passing is still broken or at least misleading with respect to replacing decoder, binary, predicate and such...

What do you think?

Deraen commented 8 years ago

Is transit writer okay with additional options? I guess the writer-fn option is being passed to it as option that the example. Other than that, looks like it wouldn't break existing set ups so it would be good.

Is there real use case for setting per type options? I think it usually makes sense to use the same predicate option for all formats and I don't know when binary option would need to be changed.

I consider this library to be mostly complete and in maintenance mode. I plan on building a new library in future which would fix mostly the implementation problems with this library, allow easier extending and fix some problems with the API (options).

Deraen commented 8 years ago

On comment about the example: Move (or writer-fn transit/writer) to outside of inside function so that it's only called once.

nblumoe commented 5 years ago

Hey @Deraen I am just going through old open PRs I got. Do you plan to do anything on this and #55? Otherwise maybe just close it for hygienic reasons! :)

Deraen commented 5 years ago

@nblumoe I don't don't have plans for new releases breaking the current API or adding new features, so in this case I'd say this PR is not going to be merged.