Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix pushState history on dispatch with redirect #536

Merged
merged 2 commits into from

6 participants

@kristianpd
Owner

Problem

If a redirect and as a result, a second dispatch occurs during the dispatch of an action, the final state of the navigator reflects the outside dispatch rather than the inside one.

For example:

class FooController extends Batman.Controller
  beforeFilter: ->
     redirect '/show'

  index: ->
  show: ->

A dispatch to the index action will result in a redirect to the show action. When pushing a new state to the navigator, the pushState to the window history occurs after the dispatch. In short, the following problem occurs:

# navigator
push: (params) ->
  path = @dispatch(params)
    # dispatch calls index
    # before filter fires
    # redirect to show occurs
    # another push state occurs during dispatch of show action
    # state is set to /show
  @pushState(null, '', path)
    # state is set back to /index
  path

Fix

Fix Navigator#push and Navigator#replace to only push and replace the last dispatch path in the case of a redirect occurring during a dispatch event.

Keeps track of a simple instance variable @_lastRedirect to compare against the expected path. If they differ, a redirect occurred deeper down and we don't want to push the outer path

ping @hornairs

@smathy I believe this may fix the issue you were encountering

@travisbot

This pull request passes (merged ed60ce1 into 4bad808).

@airhorns
Collaborator

@kristianpd can you explain in a bit more detail the issue this fixes so that @jamesmacaulay can get in on the code review too? It looks good to me but I want a second opinion.

@kristianpd
Owner

@hornairs @jamesmacaulay I've updated the PR description to include more information

@smathy

Nice, this does fix my problem :thumbsup:

@jamesmacaulay
Collaborator

This makes sense, good work. Just add regression tests please, such that if you undid your changes to any of these 3 methods (dispatch, push, replace) then we'd get a failing test.

@travisbot

This pull request passes (merged 81b1f18 into 4bad808).

@jamesmacaulay
Collaborator

LGTM :+1:

@kristianpd kristianpd merged commit d937046 into master
@loopj

This commit seems to break "forward" functionality in browser history in some cases.

I'll try to get back to you with a reproducible example, but it might just be safer to revert this commit, especially since f10c718 probably solves this problem.

@kristianpd what do you think?

Looks like your commit addresses the same issue. Were you able to get a reproducible example? It wouldn't hurt to get a test for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
26 lib/batman.js
@@ -9166,20 +9166,36 @@
};
Navigator.prototype.dispatch = function(params) {
- return this.cachedPath = this.app.get('dispatcher').dispatch(params);
+ this.cachedPath = this.app.get('dispatcher').dispatch(params);
+ if (this._lastRedirect) {
+ this.cachedPath = this._lastRedirect;
+ }
+ return this.cachedPath;
};
Navigator.prototype.push = function(params) {
- var path;
+ var path, pathFromParams, _base;
+ pathFromParams = typeof (_base = this.app.get('dispatcher')).pathFromParams === "function" ? _base.pathFromParams(params) : void 0;
+ if (pathFromParams) {
+ this._lastRedirect = pathFromParams;
+ }
path = this.dispatch(params);
- this.pushState(null, '', path);
+ if (!this._lastRedirect || this._lastRedirect === path) {
+ this.pushState(null, '', path);
+ }
return path;
};
Navigator.prototype.replace = function(params) {
- var path;
+ var path, pathFromParams, _base;
+ pathFromParams = typeof (_base = this.app.get('dispatcher')).pathFromParams === "function" ? _base.pathFromParams(params) : void 0;
+ if (pathFromParams) {
+ this._lastRedirect = pathFromParams;
+ }
path = this.dispatch(params);
- this.replaceState(null, '', path);
+ if (!this._lastRedirect || this._lastRedirect === path) {
+ this.replaceState(null, '', path);
+ }
return path;
};
View
26 lib/dist/batman.node.js
@@ -9166,20 +9166,36 @@
};
Navigator.prototype.dispatch = function(params) {
- return this.cachedPath = this.app.get('dispatcher').dispatch(params);
+ this.cachedPath = this.app.get('dispatcher').dispatch(params);
+ if (this._lastRedirect) {
+ this.cachedPath = this._lastRedirect;
+ }
+ return this.cachedPath;
};
Navigator.prototype.push = function(params) {
- var path;
+ var path, pathFromParams, _base;
+ pathFromParams = typeof (_base = this.app.get('dispatcher')).pathFromParams === "function" ? _base.pathFromParams(params) : void 0;
+ if (pathFromParams) {
+ this._lastRedirect = pathFromParams;
+ }
path = this.dispatch(params);
- this.pushState(null, '', path);
+ if (!this._lastRedirect || this._lastRedirect === path) {
+ this.pushState(null, '', path);
+ }
return path;
};
Navigator.prototype.replace = function(params) {
- var path;
+ var path, pathFromParams, _base;
+ pathFromParams = typeof (_base = this.app.get('dispatcher')).pathFromParams === "function" ? _base.pathFromParams(params) : void 0;
+ if (pathFromParams) {
+ this._lastRedirect = pathFromParams;
+ }
path = this.dispatch(params);
- this.replaceState(null, '', path);
+ if (!this._lastRedirect || this._lastRedirect === path) {
+ this.replaceState(null, '', path);
+ }
return path;
};
View
10 src/routing/navigator.coffee
@@ -26,13 +26,19 @@ class Batman.Navigator
handleCurrentLocation: => @handleLocation(window.location)
dispatch: (params) ->
@cachedPath = @app.get('dispatcher').dispatch(params)
+ @cachedPath = @_lastRedirect if @_lastRedirect
+ @cachedPath
push: (params) ->
+ pathFromParams = @app.get('dispatcher').pathFromParams?(params)
+ @_lastRedirect = pathFromParams if pathFromParams
path = @dispatch(params)
- @pushState(null, '', path)
+ @pushState(null, '', path) if !@_lastRedirect or @_lastRedirect is path
path
replace: (params) ->
+ pathFromParams = @app.get('dispatcher').pathFromParams?(params)
+ @_lastRedirect = pathFromParams if pathFromParams
path = @dispatch(params)
- @replaceState(null, '', path)
+ @replaceState(null, '', path) if !@_lastRedirect or @_lastRedirect is path
path
redirect: @::push
normalizePath: (segments...) ->
View
33 tests/batman/navigator/navigator_test.coffee
@@ -1,7 +1,6 @@
QUnit.module 'Batman.Navigator'
setup: ->
-
test "normalizePath(segments...) joins the segments with slashes, prepends a slash if necessary, and removes final trailing slashes", ->
equal Batman.Navigator.normalizePath(''), '/'
equal Batman.Navigator.normalizePath('','foo','','bar'), '/foo/bar'
@@ -11,3 +10,35 @@ test "normalizePath(segments...) joins the segments with slashes, prepends a sla
equal Batman.Navigator.normalizePath('foo','bar','baz'), '/foo/bar/baz'
equal Batman.Navigator.normalizePath('foo','//bar/baz/'), '/foo//bar/baz'
equal Batman.Navigator.normalizePath('foo','bar/baz//'), '/foo/bar/baz'
+
+test "push with dispatch that includes nested push only pushes inner state", ->
+ navigator = new Batman.Navigator
+ navigator.app = new Batman.Object
+ dispatcher:
+ pathFromParams: (params) -> params
+ dispatch: (params) ->
+ navigator.push '/redirected' if params is '/foo'
+ params
+
+ pushSpy = navigator.pushState = createSpy()
+ navigator.push '/foo'
+
+ ok pushSpy.callCount, 1
+ deepEqual pushSpy.lastCallArguments, [null, '', '/redirected']
+
+test "replace with dispatch that includes nested replace only replaces inner state", ->
+ navigator = new Batman.Navigator
+ navigator.app = new Batman.Object
+ dispatcher:
+ pathFromParams: (params) -> params
+ dispatch: (params) ->
+ navigator.replace '/redirected' if params is '/foo'
+ params
+
+ replaceSpy = navigator.replaceState = createSpy()
+ navigator.replace '/foo'
+
+ ok replaceSpy.callCount, 1
+ deepEqual replaceSpy.lastCallArguments, [null, '', '/redirected']
+
+
Something went wrong with that request. Please try again.