Resolve issue where globs would be prioritized over empty segments

[jamesplease]

Fixes #82 , supersedes #83

---

This PR is fixing a lil mistake I made when I updated the route sorting algorithm in #46.

This change might seem arbitrary, so let me explain the mistake I made, how this fixes it, and a caveat. That way, it can be merged with confidence ✊

LET'S GET TO IT

What was wrong

When I wrote this algorithm, I used these definitions:

Epsilon segment = empty segment (0 characters)
Glob segment = empty or non-empty segment = (0 or more characters)

The sorting algorithm that I'm using gives routes a numeric value to sort them by. Larger numbers are more specific. Trying to give these two segment types a single value is not straightforward, because glob segments have two interesting states – they're either empty or they're not – and we may want to take those into account separately. In other words, we may want to say that a non-empty glob has some specificity A, and that an empty glob has some other specificity B.

It's my opinion the proper fix, from a strictly mathematical sense, would be to do that. This PR doesn't do that because it would be a lot of code changes, but I think the small change I made here accomplishes the same thing.

I admit when I wrote this algorithm originally I didn't understand when Ember would produce Epsilon segments (I have a better understanding now, I think). So my initial approach was to see if I could assume something that would ignore the glob conditional and get all the tests passing.

The assumption I made is that we won't need to care about that conditional, and that instead, glob segments could always be considered more specific than epsilon segments. This made sense to me because they have the ability to be non-empty.

If that seems str8 whack to you, let me present a metaphor. Imagine there are two boxes that you couldn't see inside, and you are told to decide which one is heavier. It seems reasonable to say that the one that might be heavier is heavier, if you were forced to just say one way or the other, without using an "if" statement in your answer (like "if A is also empty, then they're equal, otherwise...").

When I made that assumption, the tests passed and I thought that was good enough. But clearly it wasn't 😛

In summary, glob segments were considered "heavier" than epsilon segments, so in all cases they would win out against an empty segment, all else being equal. Therefore, an empty URL would match a glob and not an epsilon route.

How this fixes it

The exact change swaps their weight in the code, making epsilon segments have weight 2, and globs have weight 1 (which you can see in the diff).

This might seem weird, because we're now saying that the empty box is always heavier than the box that might have stuff in it. But it works because matching doesn't solely depend on that number. It also takes into account whether the URL is empty or not.

Tests might help make sense of that. Consider two URLs, "" and "hello", being matched against two routes, "" and "*notFound".

Non-empty URL: "hello"

• ✔️ It won't match the Epsilon route because it's non-empty
• ✔️ It will match the glob, which has specificity 1
• ✔️ Static or dynamic segments are more specific than globs always, so if we registered a hello or :hello route, those would take priority.

Empty URL: ""

• ✔️ It will match the epsilon segment, which carries a weight of 2
• ✔️ It will also match the glob, which carries a weight of 1
• ✔️ The router returns a match for the epsilon segment, because 2 > 1

(this test was been added to the test suite in the PR)

From before, I mentioned that this PR achieves the same functionality as giving glob segments conditional weights. After seeing these tests, that might make more sense. One might say we're getting that conditional weight from the fact that empty segments don't match non-empty URLs.

Caveats

The point of all of this is to move route-recognizer toward a matching algorithm that works based solely on specificity, and not the order that the routes are added.

Using positional notation offers a tremendous improvement over where this lib was beforehand, I think, but it's still not 100% complete yet.

One edge case I can think of involves multiple epsilon segments in a row. If you register two epsilon segments before one epsilon segment, the two epsilon segments always match. Swap the order, and the single epsilon segment matches.

There's likely a way update the matcher to take this into account, but it may also highlight a flaw in the idea of epsilon segments. Does Ember ever need to allow a user to add multiple segments in a row?

My understanding is that the answer is no. If that's the case, an "easy" fix would be to throw an error if a user tried to add multiple epsilon segments in a row. Another idea, which is what I'm doing in a WIP router that I'm building, is to hardcode the idea of an "index" route into the router itself, rather than trying to give them a representation in the URL string. It works great! The same system can be used to implement load/error states in a pretty elegant way. I also think it makes sense to separate those special states from the URLs themselves from a theoretical perspective.

Afaik multiple epsilon segments in a row isn't documented anywhere, so maybe it's unlikely that this caveat will affect anyone? But more testing should definitely be done!

@workmanw I'm curious to see if this fixes your problem!

[workmanw]

@jmeas Yeap! This fixes the issue in my example repo and in our app. I'm not sure that my app is a great smoke test, but this fix doesn't have any negative impacts on our app. All the tests pass.

Thank you very much. I can tell by your write up the complexity of this issue and I certainly have a great appreciation for the route recognizer now. 🍻

[jamesplease]

Awesome! Glad to know that your issue is resolved ✌

[rwjblue]

This looks good to me, thank you for the thorough explanation (and the hard work on the refactor + fix).

@machty / @mmun - Do either of you have a sec to review this?

[igorT]

Looks good to me as well