compassus / compassus

A routing library for Om Next.
https://compassus.github.io/compassus/doc/1.0.0-alpha3/
Eclipse Public License 1.0
110 stars 18 forks source link

`compassus.core/index-route` should return the class #6

Closed anmonteiro closed 7 years ago

anmonteiro commented 8 years ago

Currently (compassus.core/index-route Component) will return:

(with-meta {:class Component} {:compassus.core/index-route true})

Such implementation results in a route configuration map in which all values are not Om Next classes (the index route will be a map, as shown above).

This is not desirable for people who need to programmatically access the route configuration map, expecting a map of {:keyword OmNextClass}.

One possible solution is to mutate the React class, e.g.:

(set! (. class -compassus$index-route) true)

A disadvantage of such approach is that it changes the class forever. However, I don't foresee users reusing these top-level route components in several applications with other component as the index (which could cause conflicts in deciding which is actually the index).

We could also solve this by adding a way to get at the route->component map that we store in the application config. However, this doesn't really solve the issue of having a route config map from keyword to class.

cmcfarlen commented 8 years ago

This is interesting. I luckily haven't run into this only because I haven't happened to try to lookup my index-route in the resulting map (I do look up other routes). I have also found myself wanting a current-component function from compassus. Currently I have implemented my own that calls current-route and then looks the result up in the same map of routes I give to compassus. I don't like this because I have to have a reference to that map. Perhaps you could solve both of these issues by providing a route-component function that takes the app and a route key and returns the component?

anmonteiro commented 8 years ago

What I don't like about a route-component function is also the same reason that led me to open this issue in the first place.

One of the initial requirements for Compassus was a data-first representation of routes. Currently, calling index-route means getting back something that isn't what the user has passed in. This is really what I'd like to fix here, which would allow people to store their routes in a var and be certain that that's exactly what Compassus is storing internally.

anmonteiro commented 7 years ago

Yet another alternative would be to setup the index route under a :index or :index-route key in either the app configuration map or the routes map.

Example:

(compassus/application
  {:routes {:index Home
            :about About}
   :index-route :index})

or:

(compassus/application
  {:routes {:index Home
            :about About
            :index-route :index}})

This feels more data-driven to me than the current approach of using the index-route function, and has the added benefit of preserving a map of {:keyword OmNextClass}.

anmonteiro commented 7 years ago

fixed 0a956a1904e41ff611d673352ac78609c46b65d3