Pass full options down to content handlers

[nblumoe]

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]

fixes #55

[Deraen]

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

[Deraen]

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]

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

[nblumoe]

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]

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]

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]

@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]

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]

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

[nblumoe]

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]

@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.