Dispatching on empty string

[shaunlebron]

I think the following dispatch should go into the "catch all" route, but it is going to the "users" route:

(defroute "/users/:id" [id]
  (println "User" id))

(defroute "*" []
  (println "catch all"))

(secretary/dispatch! "") ; => prints "User nil"

I suppose the use-case here is to catch the default home route, but I don't see the ability here.

[noprompt]

Interesting. Well, that's definitely a bug.

[joelash]

While working on fixing this, I came across an interesting question about the expected behavior here. Let's say we had the following routes.

(defroute "/" []
  (println "Home"))

(defroute "/users/:id" [id]
  (println "User" id))

(defroute "*" []
  (println "catch all"))

(secretary/dispatch! "") ; => should print "catch all" or "Home"?

Thoughts?

[shaunlebron]

Oh, good catch. I think "" should be caught by "/" instead of "*". Reason being, the urls below should take the same route on a client side SPA at example.com.

http://example.com/#
http://example.com/#/

[ddellacosta]

So, I'm finally getting around to cleaning up some old code and upgrading secretary. The upgrade (from 1.1.0 to 1.2.1) broke our site, and I traced it to this particular change: our routes are all in the form of /a#foo/a/b/c (etc.). Right now I've got a hacky workaround in place using with-redefs on the uri-with-leading-slash (which is extra ugly since it is a private fn) but it would be nice if this was at least customizable. Thoughts?

UPDATE: looks like the work @noprompt is doing here addresses this via a protocol? I agree that this could/should be handled by those using the library, so seems reasonable to use my hack for now and update when 2.0 is ready.

[noprompt]

@ddellacosta Hey Dave, we've been bitten by the same bug in the past but it didn't surface for us until I had been deep into the work for the next major version. Even though the branch hasn't been merged yet, we've used it in production and it works. The only reason it hasn't been merged yet is I simply haven't had enough time to update the README; that's it! Most of the information you would need to use it though is in the tests and the comments in that issue.

Since I know there are this branch really needs to get merged and the next major version out the door, I will try very hard to find time this week to do it.

[ddellacosta]

@noprompt hey Joel, no worries of course, what I've got now should be totally workable for the time being. I'll hang on until that gets merged in, and of course don't feel any pressure to get things out the door from me! I appreciate all your work on this great lib.

(EDIT: and will check out the code in that PR in the meantime to familiarize myself with how 2.0 will work.)