Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

When root option omits traling slash, Backbone.History.navigate and Backbone.History.start set URL with a slash between the root and route fragment when using pushState #1505

Merged
merged 3 commits into from

3 participants

@jrolfs

When the root option on Backbone.History omits a trailing slash, e.g: /root, the URL being pushed did not include a slash between the root fragment and route fragment so the result URL would look like /rootfragment. This patch fixes that case in both navigate() and start() (when changing URL from a hashed URL) so that the URL looks like /root/fragment.

This is related to #1414.

@jrolfs jrolfs Fixed Backbone.History.navigate and Backbone.History.start so any url…
… changes include a slash between the root and route fragment when using pushState
86cffe9
@braddunbar
Collaborator

Hi @jrolfs! Thanks for the patch! One note, I think that normalizing (adding a trailing slash to) root in History#start will be much easier than trying to deal with a possible lack of trailing slash.

test/router.js
@@ -318,4 +318,34 @@ $(document).ready(function() {
strictEqual(Backbone.history.fragment, 'x');
});
+ test("Router: insert slash before fragment when root fragment has no trailing slash", 2, function() {
+ Backbone.history.stop();
@braddunbar Collaborator

I think it's probably best to create a new history object here.

@jrolfs
jrolfs added a note

I considered that. Better safe than sorry I guess ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jrolfs

That does make sense. I guess then my only concern is line 1045 in History#start because if root is normalized the atRoot check won't work properly for the case when location.pathname omits the trailing slash on the root URL which would cause an infinite redirect loop if pushState is enabled and the browser doesn't support it. I'm more than happy to go the normalize route, minding that, and file another pull request.

@jrolfs

Sorry, to be clear I think that redirect loop only happens in IE where the return doesn't seem to prevent the browser from redirecting. Either way the atRoot check needs to account for the possibility that the URL may omit the trailing slash.

@braddunbar
Collaborator

You're right about atRoot, though I think that it should handle a presence or lack of trailing slash consistently.

Also, no need for another pull request, just push to the same branch and we'll continue to discuss here. :)

@jrolfs jrolfs Normalizing root url on Backbone.History.start, consistently matching…
… root url for atRoot in History.start regardless of trailing slash
3d6b20c
@jrolfs

Switched over to the normalize approach and fixed the unit test.

@braddunbar braddunbar merged commit 596b12b into from
@braddunbar
Collaborator

Merged above. Thanks @jrolfs!

@ddaanet

I think that was a bad idea.

My use case is using Backbone.history on a search page. I am not writing a whole application with Backbone, but I use the convenience provided by View and History to incrementally improve a web app that was written before JSON was even a thing. So I use a single route and all my route fragment is made solely of a query string.

So my root looks like "/search" and my locations look like "https://example.com/search?a=x&b=y"

Bottom line, I now need to

  1. manually perform the hash-based to pushState-based route conversion before calling History#start.
  2. assign to Backbone.history.root after calling History#start to prevent History#navigate from adding an ugly trailing slash to my location path.
@braddunbar
Collaborator

Hi @ddaanet! I don't quite follow you about manually doing the conversion, it should be handled by History. Can you post an example of this with some instructions?

As far as the trailing slash goes, we've been back and forth about it and either way makes someone upset. I'm not sure we should change it again.

@ddaanet

Backbone does the location conversion after normalizing the route to add a trailing slash. So it turns /foo#?bar=baz into /foo/?bar=baz. I understand this is the intended behaviour, but that makes me unhappy. I want /foo?bar=baz.

So I end up with this code:

    var root = '/candidate/list',
        hasPushState = window.history && window.history.pushState,
        location = window.location;
    /* XXX: Prevent Backbone 1.1.2 from adding a trailing slash to root
    when converting a hash-based route to a pushState-based route. */
    if (hasPushState && !location.search && location.hash) {
      window.history.replaceState(
        {}, document.title, root + Backbone.history.getHash());
    }
    Backbone.history.start({pushState: true, root: root});
    /* Override Backbone.history.start adding a trailing slash to root. */
    Backbone.history.root = root;

I do not think you should remove the root normalization now, since it would break compatibility. But I would be nice if there was a new option to disable it, so life would be easier when using a query-string only route.

@braddunbar
Collaborator

@ddaanet Ah! I see now. This is a result of allowing search params but not updating the logic for checking for the root. I've addressed this in #3357. Thanks for letting us know!

@ddaanet

@braddunbar Wow! That's seriously awesome. This is so awesome I'm going to reward you with another URL conversion issue I had to work around. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 26, 2012
  1. @jrolfs

    Fixed Backbone.History.navigate and Backbone.History.start so any url…

    jrolfs authored
    … changes include a slash between the root and route fragment when using pushState
  2. @jrolfs

    Normalizing root url on Backbone.History.start, consistently matching…

    jrolfs authored
    … root url for atRoot in History.start regardless of trailing slash
  3. @jrolfs
This page is out of date. Refresh to see the latest.
Showing with 56 additions and 1 deletion.
  1. +5 −1 backbone.js
  2. +51 −0 test/router.js
View
6 backbone.js
@@ -1023,6 +1023,9 @@
var docMode = document.documentMode;
var oldIE = (isExplorer.exec(navigator.userAgent.toLowerCase()) && (!docMode || docMode <= 7));
+ // Normalize root to always include trailing slash
+ if (!trailingSlash.test(this.options.root)) this.options.root = this.options.root + '/';
+
if (oldIE && this._wantsHashChange) {
this.iframe = Backbone.$('<iframe src="javascript:0" tabindex="-1" />').hide().appendTo('body')[0].contentWindow;
this.navigate(fragment);
@@ -1042,7 +1045,8 @@
// opened by a non-pushState browser.
this.fragment = fragment;
var loc = this.location;
- var atRoot = (loc.pathname === this.options.root) && !loc.search;
+ var atRoot = (loc.pathname.replace(trailingSlash, '') === this.options.root.replace(trailingSlash, '')) &&
+ !loc.search;
// If we've started off with a route from a `pushState`-enabled browser,
// but we're currently in a browser that doesn't support it...
View
51 test/router.js
@@ -318,4 +318,55 @@ $(document).ready(function() {
strictEqual(Backbone.history.fragment, 'x');
});
+ test("Router: insert slash before fragment when root fragment has no trailing slash", 3, function() {
+ Backbone.history.stop();
+ location.replace('http://example.com/root');
+ Backbone.history = new Backbone.History({
+ location: location,
+ history: {
+ pushState: function(state, title, url) {
+ strictEqual(url, '/root/fragment');
+ }
+ }
+ });
+ Backbone.history.start({
+ pushState: true,
+ root: '/root',
+ hashChange: false
+ });
+ Backbone.history.navigate('fragment');
+
+ Backbone.history.stop();
+ location.replace('http://example.com/root#fragment');
+ location.protocol = 'http:';
+ location.host = 'example.com';
+ Backbone.history = new Backbone.History({
+ location: location,
+ history: {
+ pushState: function(state, title, url) {},
+ replaceState: function(state, title, url) {
+ strictEqual(url, 'http://example.com/root/fragment');
+ }
+ }
+ });
+ Backbone.history.start({
+ pushState: true,
+ root: '/root'
+ });
+
+ Backbone.history.stop();
+ location.replace('http://example.com/root');
+ var backboneHistory = new Backbone.History({
+ location: location
+ });
+ backboneHistory.loadUrl = function() {
+ ok(true);
+ };
+ Backbone.history = backboneHistory;
+ Backbone.history.start({
+ pushState: true,
+ root: '/root'
+ });
+ });
+
});
Something went wrong with that request. Please try again.