Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

Fix some bugs in Y.Router

Router.hasRoute was failing to match routes that contained query params. Since Router only really deals with paths this seems kinda broken. Added Y.Router.removeQuery & .hasRoute now uses it. ._getPath should probably use it too but I'm not 100% convinced that's the right choice.

Router was generating bad regexes for :foo if it was the last thing in the regex. It would also end up matching hash/query params. Changed the regex so it stops at "/", "?", or "#".

Added test for regex generation and threw a ton of paths at it. There's probably some wild edge cases I didn't think of but this seems like a good start.
  • Loading branch information...
commit bf6812b1413a85d137360e28309ac3a53ba75328 1 parent bf25d7b
@tivac authored
Showing with 86 additions and 5 deletions.
  1. +22 −2 src/app/js/router.js
  2. +64 −3 src/app/tests/router-test.js
View
24 src/app/js/router.js
@@ -232,7 +232,9 @@ Y.Router = Y.extend(Router, Y.Base, {
return false;
}
- return !!this.match(this.removeRoot(url)).length;
+ url = this.removeQuery(this.removeRoot(url));
+
+ return !!this.match(url).length;
},
/**
@@ -289,6 +291,24 @@ Y.Router = Y.extend(Router, Y.Base, {
return url.charAt(0) === '/' ? url : '/' + url;
},
+
+ /**
+ Removes any query parameters from the end of the _url_ (if they exist) and
+ returns the result.
+
+ @method removeQuery
+ @param {String} url URL.
+ @return {String} Queryless path.
+ **/
+ removeQuery: function (url) {
+ var containsQuery = url.indexOf('?');
+
+ if (containsQuery > -1) {
+ url = url.substring(0, containsQuery);
+ }
+
+ return url;
+ },
/**
Replaces the current browser history entry with a new one, and dispatches to
@@ -661,7 +681,7 @@ Y.Router = Y.extend(Router, Y.Base, {
}
keys.push(key);
- return operator === '*' ? '(.*?)' : '([^/]*)';
+ return operator === '*' ? '(.*?)' : '([^/\\?\\#]*)';
});
return new RegExp('^' + path + '$');
View
67 src/app/tests/router-test.js
@@ -197,7 +197,7 @@ routerSuite.add(new Y.Test.Case({
Y.config.throwFail = this.throwFail;
delete this.throwFail;
},
-
+
'route() should add a route': function () {
var router = this.router = new Y.Router();
@@ -246,9 +246,12 @@ routerSuite.add(new Y.Test.Case({
Assert.isTrue(router.hasRoute('/foo'));
Assert.isTrue(router.hasRoute('/bar'));
+ Assert.isTrue(router.hasRoute('/bar?a=b'));
+ Assert.isTrue(router.hasRoute('/baz?a=b'));
Assert.isFalse(router.hasRoute('/baz/quux'));
+ Assert.isFalse(router.hasRoute('/baz/quux?a=b'));
},
-
+
'hasRoute() should support full URLs': function () {
var router = this.router = new Y.Router(),
loc = win && win.location,
@@ -267,7 +270,7 @@ routerSuite.add(new Y.Test.Case({
// Scheme-relative URL.
Assert.isTrue(router.hasRoute('//' + loc.host + '/foo'));
},
-
+
'hasRoute() should always return `false` for URLs with different origins': function () {
var router = this.router = new Y.Router(),
origin = 'http://something.really.random.com';
@@ -518,6 +521,63 @@ routerSuite.add(new Y.Test.Case({
});
router._dispatch('/foo', {}, src);
+ },
+
+ '_getRegex() should return regexes that do not match too much' : function() {
+ var router = this.router = new Y.Router(),
+ check = function(path, url) {
+ return router._getRegex(path, []).test(url);
+ };
+
+ Assert.isTrue( check("/*", "/"));
+ Assert.isTrue( check("/*", "/foo"));
+ Assert.isTrue( check("/*", "/foo?a=b"));
+ Assert.isTrue( check("/*", "/foo#a"));
+ Assert.isTrue( check("/*", "/foo/bar"));
+
+ Assert.isTrue( check("/*foo", "/"));
+ Assert.isTrue( check("/*foo", "/foo"));
+ Assert.isTrue( check("/*foo", "/foo?a=b"));
+ Assert.isTrue( check("/*foo", "/foo#a"));
+ Assert.isTrue( check("/*foo", "/foo/bar"));
+
+ Assert.isTrue( check("/", "/"));
+ Assert.isFalse(check("/", "/foo"));
+ Assert.isFalse(check("/", "/foo/bar"));
+ Assert.isFalse(check("/", "/foo?bar"));
+ Assert.isFalse(check("/", "/foo#bar"));
+
+ Assert.isTrue( check("/foo", "/foo"));
+ Assert.isFalse(check("/foo", "/"));
+ Assert.isFalse(check("/foo", "/foo/bar"));
+ Assert.isFalse(check("/foo", "/foo?bar"));
+ Assert.isFalse(check("/foo", "/foo#bar"));
+
+ Assert.isTrue( check("/foo/bar", "/foo/bar"));
+ Assert.isFalse(check("/foo/bar", "/"));
+ Assert.isFalse(check("/foo/bar", "/foo"));
+ Assert.isFalse(check("/foo/bar", "/foo?bar"));
+ Assert.isFalse(check("/foo/bar", "/foo#bar"));
+
+ Assert.isTrue( check("/:foo", "/foo"));
+ Assert.isTrue( check("/:foo", "/bar"));
+ Assert.isFalse(check("/:foo", "/baz/quux"));
+ Assert.isFalse(check("/:foo", "/bar?a=b"));
+ Assert.isFalse(check("/:foo", "/bar#a"));
+
+ Assert.isTrue( check("/foo/:foo", "/foo/bar"));
+ Assert.isTrue( check("/foo/:foo", "/foo/bar"));
+ Assert.isFalse(check("/foo/:foo", "/baz/quux"));
+ Assert.isFalse(check("/foo/:foo", "/foo/bar?a=b"));
+ Assert.isFalse(check("/foo/:foo", "/foo/bar#a"));
+
+ Assert.isTrue( check("/:foo/bar", "/foo/bar"));
+ Assert.isTrue( check("/:foo/bar", "/bar/bar"));
+ Assert.isFalse(check("/:foo/bar", "/baz"));
+ Assert.isFalse(check("/:foo/bar", "/bar"));
+ Assert.isFalse(check("/:foo/bar", "/baz/quux"));
+ Assert.isFalse(check("/:foo/bar", "/foo/bar?a=b"));
+ Assert.isFalse(check("/:foo/bar", "/foo/bar#a"));
}
}));
@@ -677,6 +737,7 @@ routerSuite.add(new Y.Test.Case({
Assert.areSame(2, calls);
},
+
'routes containing a "*" should match the segments which follow it': function () {
var calls = 0,
Please sign in to comment.
Something went wrong with that request. Please try again.