domkm / silk

Routing for Clojure & ClojureScript
222 stars 13 forks source link

Allow adding arbitrary values to routes #24

Closed pupeno closed 7 years ago

pupeno commented 8 years ago

Right now, what I get out of silk is mostly a name of a route and then I have another map that I use to dispatch based on the name, which function to run. These two maps have to stay in sync or the system would be broken, which led me to believe they should be one map. I implemented some wrapping functions to take care of this but I think it would be better if Silk itself would provide a way to do this.

I came up with two potential APIs.

The first one keeps routes being vectors but optionally, if the first item is a map, it'll allow adding arbitrary data. If it's a map, it should define :name with the route name and then all other key/value pairs are merged with the result whet matched:

(silk/route [{:name :route-name :arbitrary :data} [["users" "list"] {"filter" :filter "limit" :limit} {:scheme "https"}]])

I think this option is the one that requires the least amount of changes but it's a bit obscure, which might be good or bad. Backwards compatibility is retained by dispatching on whether the first item is a vector or not.

The second option is making routes a hasmap:

(silk/route {:name :route-name 
                    :pattern [["users" "list"] {"filter" :filter "limit" :limit} {:scheme "https"}]]
                    :arbitrary :data})

In this case, the change to the API is a bit bigger but the result is cleaner. Again, arbitrary data is just merged into the result of matching. In both cases, keys generated by the match, like :routes or :url should override arbitrary data (this poses the problem that adding new keys in the future might break systems out there, so, maybe it shouldn't do that, an alternative would be to put arbitrary data in a key, like :extra).

In this case backwards compatibility can be kept by dispatching on whether the argument to route as a whole, is a vector or a map.

I'm happy to do the work of implementing this.

Deraen commented 8 years ago

I have one implementation for this using existing Pattern interface:

(deftype AdditionalData [data]
  silk/Pattern
  (-match [_ _]
    data)
  (-unmatch [_ _]
    nil)
  (-match-validator [_]
    (constantly true))
  (-unmatch-validators [_]
    {}))

(defn data
  "Merges additional data into route data.
  Useful for:
  - marking if the route is private or public
  - marking what menu item should be active"
  [x]
  (AdditionalData. x))

(comment
  (silk/routes [[:login [["login"] (data {:public? true})]]
                [:items [["items"]]
                [:item [["items" :id] (data {:routing/nav :items})]])
  )

https://github.com/metosin/metosin-common/blob/master/src/cljs/metosin/ui/routing.cljs#L12-L30 https://github.com/metosin/metosin-common/blob/master/test/cljs/metosin/ui/routing_test.cljs#L10-L26

(Not perfect, doesn't work correctly with query string parameters, I think it should fixable.)

domkm commented 8 years ago

Thanks for the suggestion, @pupeno. I agree that attaching arbitrary data is useful. What do you think of @Deraen's suggestion?

tomconnors commented 8 years ago

I too wanted to be able to attach arbitrary data to routes. I used @Deraen's solution, which works well for getting the arbitrary data for matched routes, but doesn't help with getting the arbitrary data of an unmatched route. Here's what I'm doing to allow that:

(def routes (silk/routes [[:home [[]
                                  (data {:title "Home"})]]]))

(defn data-for [name]
  (assert (contains? (.-routes-map routes) name))
  (-> routes
      (.-routes-map)
      (get name)
      (.-pattern)
      :query
      (silk/match nil)))

(data-for :home) ;=> {:title "Home"}

Not particularly elegant, since I'm rummaging through your implementation details. I also see what @Deraen means about this not playing well with query string parameters. I think more direct support for this would be good.

Deraen commented 8 years ago

Not sure what you mean with "more direct" support, implementation wise the code is extending the same interface as what built-in matchers extends (vectors have implementation that checks path, maps have implementation to check query string etc.). But if we find a way to fix use with query strings, we could add the code to core.

tomconnors commented 8 years ago

By "more direct", I meant basically what you said; I think silk should support this in a way that doesn't interfere with query strings and that allows us to get the data for an unmatched route without relying on implementation details.

domkm commented 7 years ago

I'm closing as this is an old issue and I don't quite understand the need for getting arbitrary data from routes. As it's just data, I'd suggest @Deraen's solution or a small wrapper for Silk that keeps arbitrary data separately, keyed by route name. If I understand the issue, then the latter should easily allow accessing data for both matched and unmatched routes. If both of these solutions are insufficient, please comment and we'll reopen this issue. Sorry for my ridiculously slow response.