Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a smarter route match algorithm. #46

Merged
merged 1 commit into from
Feb 22, 2015

Conversation

jamesplease
Copy link
Contributor

The current sorting algorithm works-ish, but it's a bit odd. Firstly, it is imperfect. Secondly, it is rather complicated.

This PR takes advantage of a symmetry between route sorting and the way we represent numberz as symbols. Whenever you write numbers in base10, subsequent additions to the left increase the size of the number by an order of magnitude, thus ensuring that the number is larger. In other words, any number written as xyz is larger than any other number written as ab assuming x > 0, a > 0. Such is the case with URLs and specificity; URLs with more segments are always more specific (think x/y/z vs a/b). This is why the specificity is created as a string, and built while we loop. Afterward, it's converted to a number.

When comparing x/y/z against a/b/c in this system, the individual values of a...z determine the contribution to the size of that number for that magnitude. This is what lets us compare y against b independent of the other segments, which is what we want, and is the solution to the edge case in the comment.

Note: this implementation is very lol right now because I didn't want to refactor the code too much. I just wanted to make a bare minimum number of changes to show that this algorithm passes the tests. If it were implemented, there would likely be some more shuffling of code to make it less weird.

Also there should be more tests for those edge cases this new algorithm supports, and also updates to the comments.

if (a.types.statics !== b.types.statics) { return b.types.statics - a.types.statics; }

return 0;
return b.specificity.val - a.specificity.val;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tkellen
Copy link

tkellen commented Feb 6, 2015

💃

@tkellen
Copy link

tkellen commented Feb 18, 2015

ping @rwjblue @wycats @machty

@jamesplease
Copy link
Contributor Author

Another benefit to this approach is that we could sort the routes. If we do that then we could implement binary-search to locate the correct route, making the algorithm O(lg n) instead of O(n), or, in other words, much faster as the number of routes grows large.

There are a few other optimizations that could be made, too. This is the first step tho'.

@stefanpenner
Copy link
Collaborator

👍

@machty
Copy link
Contributor

machty commented Feb 18, 2015

@jmeas This looks pretty good; can you add a test case that would be breaking without this code change?

@jamesplease
Copy link
Contributor Author

Will-do tomorrow, @machty.

@trek
Copy link
Collaborator

trek commented Feb 20, 2015

Also there should be more tests for those edge cases this new algorithm supports, and also updates to the comments.

👍 This is one of those very clever solutions that will puzzle future generations without a helpful hint about what is going on

@jamesplease
Copy link
Contributor Author

Will-do tomorrow, @machty.

🙊 Sorry, I've been super busy this week on a project @ work. I should have time this weekend.

@stefanpenner
Copy link
Collaborator

Sorry, I've been super busy this week on a project @ work. I should have time this weekend.

famous last words :trollface:

but would love to get this in :)

@drogus
Copy link

drogus commented Feb 21, 2015

I really like it, it solves my issue described in #42. This is neat, especially that it can go into Ember 1.x. If you want some more tests, I committed 2 cases that caused me problems: https://github.com/drogus/route-recognizer/tree/route-priority-tests

That said, I still think that eventually automatic sorting should be removed and routes should be matched in an order that they're added. It is simpler and easier to document.

@jamesplease
Copy link
Contributor Author

That said, I still think that eventually automatic sorting should be removed and routes should be matched in an order that they're added. It is simpler and easier to document.

Backbone does this, and it causes some frustration. Although static params and named params still generally match when you want, the behavior of splats is drastically different. They go from being match-last to match-first, which makes them harder to use, and maybe even less useful.

The most common use case for splats, I think, are 404 routes, so this necessitates that you register your 404 route last (or, in the style angular routers, give developers a special hook when no route is found).

I'm confident that there's a way to document this algorithm in a way that people understand how it works and appreciate its usefulness.

Alternatively (this might not be very Ember-y, idk) it could be an option that can be turned off? Dunno if that's a good idea tho'

@drogus
Copy link

drogus commented Feb 21, 2015

Backbone does this, and it causes some frustration

Could you provide an example on where it would lead to frustration? I'm curious when it could cause problems.

The only problem with automatic sorting I have is that if you encounter a case, which works differently than you expect, you can't fix it in any way. After thinking this through again, however, I can't think of any case that wouldn't work with the new algorithm. It could potentially cause problems if support for regexps was added, but I'm not sure if this is something that is even planned. Also, if the algorithm defaults to order in which routes are defined in case specifity is the same, it shouldn't be too much of problem with regexps, too.

// We take advantage of the fact that route specificity
// mirrors the way we write numbers as strings. For more on
// how this works, refer to:
// https://github.com/tildeio/route-recognizer/pull/46
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opted out of writing a paragraph about how this works. Lemme know if you think I should do otherwise.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the information immediately at hand is better. Write the paragraph. Also: "the way we write numbers as strings" who is "we"? Do you actually mean "the way numbers and strings have a numerical value in ASCII"? or "the way JavaScript typecasts strings into numbers for comparisons", or something else?

@jamesplease
Copy link
Contributor Author

Comments updated, and I added two test cases that the old algorithm incorrectly resolved.

If there are more tests you'd like to add, just let me know what they are and I can add 'em!

@jamesplease
Copy link
Contributor Author

Updated @trek.

stefanpenner added a commit that referenced this pull request Feb 22, 2015
Implement a smarter route match algorithm.
@stefanpenner stefanpenner merged commit 52796e1 into tildeio:master Feb 22, 2015
@stefanpenner
Copy link
Collaborator

👍

@stefanpenner
Copy link
Collaborator

@machty we need to decide what the version bump is needed. I would consider this a bugfix, although it may be somewhat breaking for people.

@jamesplease
Copy link
Contributor Author

👯

@wycats
Copy link
Contributor

wycats commented Feb 22, 2015

I'm happy with this change, but I still feel strongly (based on my days in Rails) that "first insertion" rules produce very unexpected results. This becomes especially problematic when you are writing abstractions that generate routes that end up colliding (because of static vs dynamic segments or fallback routes)

@machty
Copy link
Contributor

machty commented Feb 22, 2015

First insertion might also be problematic for lazily-loaded routes.

jamiebuilds referenced this pull request in jamiebuilds/rfcs Jun 7, 2015
@jamesplease
Copy link
Contributor Author

Because there's some interesting conversation here about the limitations of first insertion algorithms, I figured I would play devil's advocate and share an alternative that works well for the other major type of router that I know of (Angular's UI-Router & the routers it has inspired). These routers a first insertion algorithm, but two additional features make it as predictable as Ember's algorithm.

The first feature is regex support on dynamic segments. So you could, for instance, write:

"/user/{id:[0-9a-fA-F]{1,8}}"

and it would only match hex values.

This feature is useful because it makes it allows you the same flexibility when adding another route called, say, "/user/profile". By increasing the specificity of what the dynamic segment itself matches, the end result is functionally equivalent to a match by specificity algorithm as seen in Ember.

The other feature is what I call a 404 abstraction. A 404 abstraction is one of either:

  1. An event that the router emits when no route is matched, such that you can do...

    router.on('notfound', cb);

  2. A special route that is always activated when no other route is matched (generally determined by name)

    router.register({ name: 'notfound', enter: cb });

I think these two features solve the most common problems with first insertion rules, but maybe not all of them.

Also, writing Regexes can get pretty ugly pretty quickly, but one could argue that it's more powerful than Ember's system. I'd be interested to see how often that power is actually needed, though. I know, for one, that no application I have written has required it, but that's a negligible subset of all of the apps out there.

As for the 404 abstraction, I can see how it might be easier for a new developer to learn. Given that it's a distinct concept, rather than clever use of an algorithm, it tends to appear in its own section in the Guides and/or API. It's also a bit more expressive than using a splat, which makes code more readable, I think. But from a strictly logically perspective it is an unnecessary concept when match by specificity is used.

Both systems work well enough, I think, to adequately avoid frustrating the developer. A system that does not work, though, is one that does not match by specificity nor one that provides regex in the dynamic segments. Backbone is an example of this. If you want to use regex, the whole route string itself must be regex, which is pretty lame, I think.

Just some thoughts I've had while researching routerz ☁️

rwjblue added a commit to rwjblue/ember.js that referenced this pull request Apr 20, 2016
Fixes a number of bugs:

* [tildeio/route-recognizer#46](tildeio/route-recognizer#46) Implement a smarter route match algorithm.
  Fixes a number of route priority issues, specifically one around glob
  route priority matching.
* [tildeio/route-recognizer#62](tildeio/route-recognizer#62)
  Handle malformed URI error. Prevents errors from being thrown (and
  halting the boot of an app) when a malformed queryParam is present.
* General performance tweaks in:
  * tildeio/route-recognizer#76 Add benchmark
    suite.
  * tildeio/route-recognizer#75 Pre-allocate
    array lengths.
  * tildeio/route-recognizer#77 Cache results of
    `State.get`.
  * tildeio/route-recognizer#78 Route result
    should be of a fixed size.
  * tildeio/route-recognizer#79 Removes closures
    around `eachChar`.
  * tildeio/route-recognizer#80 Fix State and
    `charSpec` shaping.

Compare view:

tildeio/route-recognizer@v0.1.5...v0.1.10
mixonic added a commit that referenced this pull request Aug 17, 2016
Revert specificity changes in PR #46
toddjordan pushed a commit to toddjordan/ember.js that referenced this pull request Sep 9, 2016
Fixes a number of bugs:

* [tildeio/route-recognizer#46](tildeio/route-recognizer#46) Implement a smarter route match algorithm.
  Fixes a number of route priority issues, specifically one around glob
  route priority matching.
* [tildeio/route-recognizer#62](tildeio/route-recognizer#62)
  Handle malformed URI error. Prevents errors from being thrown (and
  halting the boot of an app) when a malformed queryParam is present.
* General performance tweaks in:
  * tildeio/route-recognizer#76 Add benchmark
    suite.
  * tildeio/route-recognizer#75 Pre-allocate
    array lengths.
  * tildeio/route-recognizer#77 Cache results of
    `State.get`.
  * tildeio/route-recognizer#78 Route result
    should be of a fixed size.
  * tildeio/route-recognizer#79 Removes closures
    around `eachChar`.
  * tildeio/route-recognizer#80 Fix State and
    `charSpec` shaping.

Compare view:

tildeio/route-recognizer@v0.1.5...v0.1.10
@jamesplease jamesplease deleted the algorithm-pls branch May 31, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants