Inconsistent route handling in static vs. dynamic routes.

I've got some controllers that can handle multiple routes, usually a static and a dynamic variant of the same route, such as library and library/:category. Since both routes navigate to the same page, I use the same controller and ideally, I'd like to use the same handler for both routes as well. If the optional parameter is not defined by the url, I can just fall back to a default value instead.

At the moment, this is rather flakey because the handler receives the same amount of arguments, but different data depending if the route was static or dynamic:
  • enyo.Router._handleStatic executes this._execHandler(context, handler, [path], route);.
  • enyo.Router._handleDynamic executes this._execHandler(context, handler, matches, route);.
As you can see, in the case of static routes, the route's handler gets called with one argument containing the matched static path. Yet in the case of dynamic routes, the route's handler gets called with one or more arguments each containing a parameter extracted from the url, but no copies of the full path.

Further complicating thing, the handler also gets no notification of which route has been triggered, making it impossible to distinguish if the static or dynamic route has been triggered.

I would like to propose returning both the entire matched path, and the extracted parameters from the matched route. Perhaps even a hash of key-value pairs for matched parameters in dynamic routes. This way, it's much easier to determine what's going on.

I'll be implementing something like this for my current project, but if it's something you'd like to see in enyo core, I can create a pull-request.

Comments

  • Well, that is an unfortunate difference in the handling of routes. A pull request would be welcomed. Can you do all that in a backwards compatible way?
  • I can make a pull-request, but I'm not sure if it will be possible to maintain full backward compatibility.

    I could add the path as 1st argument, maintaining compatibility only with the static handler.

    Alternatively, I could add the path as last argument, maintaining compatibility with the dynamic handler, but if the same handler has to handle multiple variations of the same route, this makes it unpredictable which argument is the path.

    Or I can drop backwards compatibility, and return a path, and a hash with key-value pairs. This is probably the most flexible solution - allowing a single handler for multiple route variations with optional parameters and such, but not backwards compatible. The break would be very easy to deal with though.
  • If you can tell me which you prefer, I'll create a pull-request.
  • Might be a good time to break it and make it right. Let me put on my thinking cap and let you know how we'd prefer for it to be.
  • Sure, I'll await the verdict.
  • Got any news yet on which way you want to go with this?
  • No, still debating this. We all agree that router needs love and much better documentation (and samples).
  • I can give the router some extra love besides this one update. I've got a refactored version of the current router that's entirely backwards compatible, but unlike the current implementation you can extend it to make super-kinds. It also allows you to define a custom token regexp and takes care of uri-decoding url parameters.
  • That would be nice to look at. Do you have some samples and documentation along with it?
  • I don't have documentation at the moment, but I don't think it would be very difficult to set up an example. I'll throw together an example.
  • Just a small upate, I'll try to get an example together this week. Kinda busy with some deadlines, so could take a little while.
  • Not a problem at all. We appreciate the work you're doing and we're also quite busy, too!
  • edited April 2015
    My router was built for enyo 2.4. I worked on it last night and it's now more in line with the 2.5 router, including updated comments and such.

    Main changes are as follows:
    • Almost all private values are now properties of the enyo.Router kind.
    • listeners array is now a static property on the enyo.Router kind.
    • hashDidChange method is now a static method on the enyo.Router kind.
    • The regex for extracting token values is now a property (and can thus be easily modified)
    • The regex for extracting token values accepts a few more characters, enabling use of url encoded parameters.
    • Token values are automatically decoded using decodeURIComponent.
    • Handlers now all receive the same arguments for static and dynamic routes:
      • 1st argument, the path of the matched route
      • 2nd argument, an optional key-value hash of the parameters extracted from the url.
    These changes allow the creation of router sub-kinds, and add some more flexibility to the values you can pass through the url.

    I've put the current code on pastebin, maybe you could take a look and see if it's heading in the right direction or if you'd like to see some changes before I make a pull-request: http://pastebin.com/WLF8H1yq
  • Hey Roy,

    I was wondering if you've had time to take a look at the current code :)

    --
    Ruben
  • Not yet! Probably not this week.
  • Okay, no problem, thanx for keeping me updated. I hope you'll find a little time next week then :smile:
  • I found another slight error in the router. The docs state that enyo.Router.addHistory will add the location to the lowest position in the stack if the supplied index is out of bounds. Looking at the source, however, it appears the router simply ignores the location and does not add it at all.

    What would be the preferred behaviour? Aribtrarily adding an out-of-bounds location to the 'lowest position' of the stack as described in the docs does not seem very intuitive. However, silently failing is probably even worse.

    I could add a fix to my updated router, let me know what the preferred behaviour should be. I think I personally would prefer either:
    - If the index is too high, add it at the highest point and if the index is too low, add it at the current lowest point.
    Or:
    - Throw an error.
Sign In or Register to comment.