Skip to content

Commit

Permalink
Revert 56ab65a
Browse files Browse the repository at this point in the history
This patch causes regressions in real Ember code. Two reasons this might
be the case:

* This code causes specificity on a previous route to stomp on a later
  route. It expects the return value from `currentState.put` is always
  fresh, and thus that specificity can be set. In reality the return
  state object may be reused, so setting a new specificity would stomp
  an existing one.
* This code does not take into account the depth of a segment added from
  a different route.
  • Loading branch information
mixonic committed Aug 17, 2016
1 parent 99ac97d commit 97f3bf6
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 56 deletions.
61 changes: 28 additions & 33 deletions lib/route-recognizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,59 +100,35 @@ EpsilonSegment.prototype = {
// The `names` will be populated with the paramter name for each dynamic/star
// segment. `shouldDecodes` will be populated with a boolean for each dyanamic/star
// segment, indicating whether it should be decoded during recognition.
function parse(route, names, specificity, shouldDecodes) {
function parse(route, names, types, shouldDecodes) {
// normalize route as not starting with a "/". Recognition will
// also normalize.
if (route.charAt(0) === "/") { route = route.substr(1); }

var segments = route.split("/");
var results = new Array(segments.length);

// A routes has specificity determined by the order that its different segments
// appear in. This system mirrors how the magnitude of numbers written as strings
// works.
// Consider a number written as: "abc". An example would be "200". Any other number written
// "xyz" will be smaller than "abc" so long as `a > x`. For instance, "199" is smaller
// then "200", even though "y" and "z" (which are both 9) are larger than "0" (the value
// of (`b` and `c`). This is because the leading symbol, "2", is larger than the other
// leading symbol, "1".
// The rule is that symbols to the left carry more weight than symbols to the right
// when a number is written out as a string. In the above strings, the leading digit
// represents how many 100's are in the number, and it carries more weight than the middle
// number which represents how many 10's are in the number.
// This system of number magnitude works well for route specificity, too. A route written as
// `a/b/c` will be more specific than `x/y/z` as long as `a` is more specific than
// `x`, irrespective of the other parts.
// Because of this similarity, we assign each type of segment a number value written as a
// string. We can find the specificity of compound routes by concatenating these strings
// together, from left to right. After we have looped through all of the segments,
// we convert the string to a number.
specificity.val = '';

for (var i=0; i<segments.length; i++) {
var segment = segments[i], match;

if (match = segment.match(/^:([^\/]+)$/)) {
results[i] = new DynamicSegment(match[1]);
names.push(match[1]);
shouldDecodes.push(true);
specificity.val += '3';
types.dynamics++;
} else if (match = segment.match(/^\*([^\/]+)$/)) {
results[i] = new StarSegment(match[1]);
names.push(match[1]);
shouldDecodes.push(false);
specificity.val += '1';
types.stars++;
} else if(segment === "") {
results[i] = new EpsilonSegment();
specificity.val += '2';
} else {
results[i] = new StaticSegment(segment);
specificity.val += '4';
types.statics++;
}
}

specificity.val = +specificity.val;

return results;
}

Expand Down Expand Up @@ -246,10 +222,29 @@ State.prototype = {
}
};

// Sort the routes by specificity
// This is a somewhat naive strategy, but should work in a lot of cases
// A better strategy would properly resolve /posts/:id/new and /posts/edit/:id.
//
// This strategy generally prefers more static and less dynamic matching.
// Specifically, it
//
// * prefers fewer stars to more, then
// * prefers using stars for less of the match to more, then
// * prefers fewer dynamic segments to more, then
// * prefers more static segments to more
function sortSolutions(states) {
return states.sort(function(a, b) {
return b.specificity.val - a.specificity.val;
if (a.types.stars !== b.types.stars) { return a.types.stars - b.types.stars; }

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

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

return 0;
});
}

Expand Down Expand Up @@ -337,15 +332,15 @@ var RouteRecognizer = function() {
RouteRecognizer.prototype = {
add: function(routes, options) {
var currentState = this.rootState, regex = "^",
specificity = {},
types = { statics: 0, dynamics: 0, stars: 0 },
handlers = new Array(routes.length), allSegments = [], name;

var isEmpty = true;

for (var i=0; i<routes.length; i++) {
var route = routes[i], names = [], shouldDecodes = [];

var segments = parse(route.path, names, specificity, shouldDecodes);
var segments = parse(route.path, names, types, shouldDecodes);

allSegments = allSegments.concat(segments);

Expand Down Expand Up @@ -375,7 +370,7 @@ RouteRecognizer.prototype = {

currentState.handlers = handlers;
currentState.regex = new RegExp(regex + "$");
currentState.specificity = specificity;
currentState.types = types;

if (name = options && options.as) {
this.names[name] = {
Expand Down
23 changes: 0 additions & 23 deletions tests/recognizer-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -569,29 +569,6 @@ test("Prefers single dynamic segments over stars", function() {
resultsMatch(router.recognize("/foo/bar/suffix"), [{ handler: handler2, params: { star: "bar", dynamic: "suffix" }, isDynamic: true }]);
});

test("Prefers more specific routes over less specific routes", function() {
var handler1 = { handler: 1 };
var handler2 = { handler: 2 };
var router = new RouteRecognizer();

router.add([{ path: "/foo/:dynamic/baz", handler: handler1 }]);
router.add([{ path: "/foo/bar/:dynamic", handler: handler2 }]);

resultsMatch(router.recognize("/foo/bar/baz"), [{ handler: handler2, params: { dynamic: "baz" }, isDynamic: true }]);
resultsMatch(router.recognize("/foo/3/baz"), [{ handler: handler1, params: { dynamic: "3" }, isDynamic: true }]);
});

test("Prefers more specific routes with stars over less specific dynamic routes", function() {
var handler1 = { handler: 1 };
var handler2 = { handler: 2 };
var router = new RouteRecognizer();

router.add([{ path: "/foo/*star", handler: handler1 }]);
router.add([{ path: "/:dynamicOne/:dynamicTwo", handler: handler2 }]);

resultsMatch(router.recognize("/foo/bar"), [{ handler: handler1, params: { star: "bar" }, isDynamic: true }]);
});

test("Handle star routes last when there are trailing `/` routes.", function() {
var handler1 = { handler: 1 };
var handler2 = { handler: 2 };
Expand Down

0 comments on commit 97f3bf6

Please sign in to comment.