Skip to content

Loading…

fixes #1003 - History is started before `navigate` #1010

Merged
merged 1 commit into from

2 participants

@braddunbar
Collaborator

If history has not been started before the first call to navigate (changing the iframe hash) the next checkUrl sees the hash as blank. By starting history earlier in start this is avoided.

Also, historyStarted was moved to Backbone.History.started. It is impossible to check it's value otherwise.

The test case is only useful in browsers that need the iframe.

@jashkenas
Owner

Looks almost perfect. But shouldn't that be Backbone.history.started? With the lowercase h

@braddunbar
Collaborator

I don't think so. historyStarted was used for all instances of Backbone.History. It denotes that any instance has been started, not just the current one.

var history = new Backbone.History();
history.start();
Backbone.history.start(); // should throw
@jashkenas
Owner

You're quite right.

@jashkenas jashkenas merged commit 16b37e7 into jashkenas:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 15, 2012
  1. @braddunbar
This page is out of date. Refresh to see the latest.
Showing with 42 additions and 21 deletions.
  1. +8 −7 backbone.js
  2. +34 −14 test/router.js
View
15 backbone.js
@@ -851,7 +851,7 @@
// Handles cross-browser history management, based on URL fragments. If the
// browser does not support `onhashchange`, falls back to polling.
- Backbone.History = function() {
+ var History = Backbone.History = function() {
this.handlers = [];
_.bindAll(this, 'checkUrl');
};
@@ -863,10 +863,10 @@
var isExplorer = /msie [\w.]+/;
// Has the history handling already been started?
- var historyStarted = false;
+ History.started = false;
// Set up all inheritable **Backbone.History** properties and methods.
- _.extend(Backbone.History.prototype, Backbone.Events, {
+ _.extend(History.prototype, Backbone.Events, {
// The default interval to poll for hash changes, if necessary, is
// twenty times a second.
@@ -892,10 +892,11 @@
// Start the hash change handling, returning `true` if the current URL matches
// an existing route, and `false` otherwise.
start: function(options) {
+ if (History.started) throw new Error("Backbone.history has already been started");
+ History.started = true;
// Figure out the initial configuration. Do we need an iframe?
// Is pushState desired ... is it available?
- if (historyStarted) throw new Error("Backbone.history has already been started");
this.options = _.extend({}, {root: '/'}, this.options, options);
this._wantsHashChange = this.options.hashChange !== false;
this._wantsPushState = !!this.options.pushState;
@@ -903,6 +904,7 @@
var fragment = this.getFragment();
var docMode = document.documentMode;
var oldIE = (isExplorer.exec(navigator.userAgent.toLowerCase()) && (!docMode || docMode <= 7));
+
if (oldIE) {
this.iframe = $('<iframe src="javascript:0" tabindex="-1" />').hide().appendTo('body')[0].contentWindow;
this.navigate(fragment);
@@ -921,7 +923,6 @@
// Determine if we need to change the base url, for a pushState link
// opened by a non-pushState browser.
this.fragment = fragment;
- historyStarted = true;
var loc = window.location;
var atRoot = loc.pathname == this.options.root;
@@ -950,7 +951,7 @@
stop: function() {
$(window).unbind('popstate', this.checkUrl).unbind('hashchange', this.checkUrl);
clearInterval(this._checkUrlInterval);
- historyStarted = false;
+ History.started = false;
},
// Add a route to be tested when the fragment changes. Routes added later
@@ -991,7 +992,7 @@
// route callback be fired (not usually desirable), or `replace: true`, if
// you wish to modify the current URL without adding an entry to the history.
navigate: function(fragment, options) {
- if (!historyStarted) return false;
+ if (!History.started) return false;
if (!options || options === true) options = {trigger: options};
var frag = (fragment || '').replace(routeStripper, '');
if (this.fragment == frag || this.fragment == decodeURIComponent(frag)) return;
View
48 test/router.js
@@ -1,6 +1,32 @@
$(document).ready(function() {
- module("Backbone.Router");
+ var router = null;
+ var lsatRoute = null;
+ var lastArgs = [];
+
+ function onRoute(router, route, args) {
+ lastRoute = route;
+ lastArgs = args;
+ }
+
+ module("Backbone.Router", {
+
+ setup: function() {
+ Backbone.history = null;
+ router = new Router({testing: 101});
+ Backbone.history.interval = 9;
+ Backbone.history.start({pushState: false});
+ lastRoute = null;
+ lastArgs = [];
+ Backbone.history.on('route', onRoute);
+ },
+
+ teardown: function() {
+ Backbone.history.stop();
+ Backbone.history.off('route', onRoute);
+ }
+
+ });
var Router = Backbone.Router.extend({
@@ -58,19 +84,6 @@ $(document).ready(function() {
});
- Backbone.history = null;
- var router = new Router({testing: 101});
-
- Backbone.history.interval = 9;
- Backbone.history.start({pushState: false});
-
- var lastRoute = null;
- var lastArgs = [];
- Backbone.history.bind('route', function(router, route, args) {
- lastRoute = route;
- lastArgs = args;
- });
-
test("Router: initialize", function() {
equal(router.testing, 101);
});
@@ -208,4 +221,11 @@ $(document).ready(function() {
equal(history.getFragment('/root/foo'), 'foo');
});
+ test("#1003 - History is started before navigate is called", function() {
+ var history = new Backbone.History();
+ history.navigate = function(){ ok(Backbone.History.started); };
+ Backbone.history.stop();
+ history.start();
+ });
+
});
Something went wrong with that request. Please try again.