search

metosin / reitit

A fast data-driven routing library for Clojure/Script
https://cljdoc.org/d/metosin/reitit/
Eclipse Public License 1.0
1.43k stars 256 forks source link

Expand protocol seems to be broken #201

Closed piranha closed 5 years ago

piranha commented 5 years ago

There is reitit.core/Expand protocol, which in theory provides extension point for adding various types of handlers (etc). I have tried adding support for vars like that:

(extend-protocol route/Expand
  clojure.lang.Var
  (expand [this _] {:handler this}))

This code executes with no problems and if I (prn reitit.core/Expand) in repl, I see clojure.lang.Var there, but compilation of routes still fails with No implementation of method: :expand of protocol: #'reitit.core/Expand found for class: clojure.lang.Var.

What fixes it though is passing {:expand reitit.core/expand} as an option for router. It seems that having expand as a default parameter in router captures protocol state as it is defined in reitit.core source.

Not sure what's the best way to proceed, but it feels like this is not intended behavior. :-)

ikitommi commented 5 years ago

Oh, that's not intented. I guess it would work if the default options were provided via a no-args function instead. I guess there isn't a test for this...

piranha commented 5 years ago

Is there any need for expand to be provided as an option though? It seems to me protocol should be enough - of course, that would limit customization options, but OTOH isn't limiting options in some ways a goal of a good library design? :-)

ikitommi commented 5 years ago

Excellent point.

Protocol is good but it has opinions about expanding keywords & functions, so it is made interchangeable. Not sure if overriding Protocol dispatch definitions would work, but it would be global side-effect and one couldn't have different routers with different expansion under same ClassLoader. Much cleaner to allow the implementation to be changed.

Here's an example use case btw: https://metosin.github.io/reitit/advanced/shared_routes.html#using-custom-expander

Example problem with non-pluggable impl: https://github.com/dakrone/cheshire/issues/77

Interested in a PR for this? ;)

piranha commented 5 years ago

Yeah, the way with option of course is much more customizable... Okay, do you think (let [route-expand (:expand opts expand)] ... is the right way to go? :-)

ikitommi commented 5 years ago

Would making default-router-options a defn work?

piranha commented 5 years ago

Ah! I guess so! :-)

piranha commented 5 years ago

Sorry for not coming up with patch, wasn't happy with my test and got distracted. :\