From bf6812b1413a85d137360e28309ac3a53ba75328 Mon Sep 17 00:00:00 2001 From: Pat Cavit Date: Fri, 30 Mar 2012 23:46:24 -0700 Subject: [PATCH] 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. --- src/app/js/router.js | 24 +++++++++++-- src/app/tests/router-test.js | 67 ++++++++++++++++++++++++++++++++++-- 2 files changed, 86 insertions(+), 5 deletions(-) diff --git a/src/app/js/router.js b/src/app/js/router.js index 5039fd0c934..35903380979 100644 --- a/src/app/js/router.js +++ b/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 + '$'); diff --git a/src/app/tests/router-test.js b/src/app/tests/router-test.js index 1abf0f5b734..ea5bfe87684 100644 --- a/src/app/tests/router-test.js +++ b/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,