Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Maintain redirect options in session. Fix #221 #222

Merged
merged 2 commits into from

3 participants

Yasuharu Ozaki Nick Reed David Heinemeier Hansson
Yasuharu Ozaki

To reflect redirect_to options, keep those params in session to be set 'X-XHR-Referer' after redirected response.

Nick Reed
Collaborator

The client-side code expects X-XHR-Current-Location to be a path (/users/1), but the value of session[:_turbolinks_redirect_to] will be a full URI (http://example.com/users/1).

Also, if the link that was clicked had an anchor in it's href, that anchor is going to be appended onto the end of the redirected URL. So you could end up with two anchors: http://example.com/users/1#controller-anchor#link-anchor

Yasuharu Ozaki

the value of session[:_turbolinks_redirect_to] will be a full URI (http://example.com/users/1)

I know it, but does it make some issue?

anchor is going to be appended onto the end of the redirected URL

Thanks for feedback. I want to ask you about reflectRedirectedUrl.
Current core source code of it is

reflectRedirectedUrl = ->
    window.history.replaceState currentState, '', location + document.location.hash

made by fadab58.

Another anchor appending issue will be solved if the code is changed like

reflectRedirectedUrl = ->
    window.history.replaceState currentState, '', location

Why did you want to retain current document.location.hash even after redirected to new location?

Nick Reed
Collaborator

Regardless of whether or not it creates an issue, it's bad code. When the client compares X-XHR-Current-Location to document.location.pathname + document.location.search, it'll return false for the wrong reason.

The pull request you're referring to (#125) is one that you actually supported. To answer your question, we want to retain the anchor after a redirect because that's what the browser does.

Nick Reed
Collaborator

It appears that without Turbolinks, browsers handle the double anchor situation by preserving the redirect anchor and dropping the link anchor.

Yasuharu Ozaki

turbolinks.rb

def _compute_redirect_to_location_with_xhr_referer(options)
  session[:_turbolinks_redirect_to] =
    if options == :back && request.headers["X-XHR-Referer"]
       _compute_redirect_to_location_without_xhr_referer(request.headers["X-XHR-Referer"])
    else
      _compute_redirect_to_location_without_xhr_referer(options)
    end
end

def set_xhr_current_location
  response.headers['X-XHR-Current-Location'] = session.delete(:_turbolinks_redirect_to) || ''
end

turbolinks.js.coffee

reflectRedirectedUrl = ->
  location = xhr.getResponseHeader 'X-XHR-Current-Location'
  if location and location != ''
    window.history.replaceState currentState, '', location

solves issues in #110, #218, and #221.

I think it is good to change the name of set_xhr_current_location to like set_xhr_redirect_to, but could you check above code first?

Nick Reed
Collaborator

It doesn't preserve the anchor on a redirect.

Yasuharu Ozaki

Can you show your controller code?

Nick Reed
Collaborator
def edit
  redirect_to action: :new
end
Yasuharu Ozaki

I understand what you mean.

Added commit should fix anchor preserving issue. Could you check?

Nick Reed
Collaborator

This would be much, much simpler:

reflectRedirectedUrl = ->
  if (location = xhr.getResponseHeader 'X-XHR-Current-Location')?.length
    preservedHash = if removeHash(location) is location then document.location.hash else ''
    window.history.replaceState currentState, '', location + preservedHash
Yasuharu Ozaki yasuoza Maintain redirect options in session. Fix #221
To reflect redirect_to options, keep redirect url in session to be set
'X-XHR-Current-Location' in response header.
2aa978f
Yasuharu Ozaki

Thanks.
Squished.

Nick Reed
Collaborator

Like you said, we should probably change the name of X-XHR-Current-Location to something like X-XHR-Redirected. And do we even need to send the response header if there hasn't been a redirect? All it's doing is making the client have to check two conditions (the existence of the header and the length of it's value).

Yasuharu Ozaki

Yes, we should change the name like X-XHR-Redirected.

do we even need to send the response header if there hasn't been a redirect?

Without sending redirect_to location, how can we detect redirected location? Or Do you mean we should set redirected flag such as X-XHR-Redirected to 1 when redirected and X-XHR-Redirected-To to redirect location in header?

Nick Reed
Collaborator

No, I mean something like this:

def set_xhr_redirected
  redirected = session.delete :_turbolinks_redirect_to
  if redirected
    response.headers['X-XHR-Redirected'] = redirected
  end
end
reflectRedirectedUrl = ->
  if (location = xhr.getResponseHeader 'X-XHR-Redirected')?
    preservedHash = if removeHash(location) is location then document.location.hash else ''
    window.history.replaceState currentState, '', location + preservedHash
Yasuharu Ozaki

It will be shorter:

def set_xhr_redirected                                         
  response.headers['X-XHR-Redirected'] = session.delete(:_turbolinks_redirect_to) || '' 
end                                                                                           
reflectRedirectedUrl = ->
  if location = xhr.getResponseHeader 'X-XHR-Redirected'
    preservedHash = if removeHash(location) is location then document.location.hash else ''
    window.history.pushState { turbolinks: true, position: currentState.position + 1 }, '', url

I wrote above code once first changed to check whether location isn't blank to be more valid code.
What do you think?

Nick Reed
Collaborator

I think you're missing my point. There's no reason to send the header at all if it's just going to be an empty string.

My example could be cleaner. This is better:

def set_xhr_redirected
  if session[:_turbolinks_redirect_to]
    response.headers['X-XHR-Redirected'] = session.delete :_turbolinks_redirect_to
  end
end
Yasuharu Ozaki

So, how about this?
And I changed X-XHR-Redirected to X-XHR-Redirected-To because X-XHR-Redirected sounds like state, not location.

def set_xhr_redirected_to
  if session[:_turbolinks_redirect_to]
    response.headers['X-XHR-Redirected-To'] = session.delete :_turbolinks_redirect_to
  end
end
reflectRedirectedUrl = ->
  if location = xhr.getResponseHeader 'X-XHR-Redirected-To'
    preservedHash = if removeHash(location) is location then document.location.hash else ''
    window.history.pushState { turbolinks: true, position: currentState.position + 1 }, '', url
Nick Reed
Collaborator

It's fine, as long as you don't make the accidental mistake of using the last line of reflectNewUrl like your example shows.

Yasuharu Ozaki yasuoza Use X-XHR-Redirected-To instead of X-XHR-Current-Location
Set redirect location to X-XHR-Redirected-To header
only when server redirects user.
And use the location info in reflectRedirectedUrl.
f485478
Yasuharu Ozaki

Added a commit. Need to be squished?

Nick Reed
Collaborator

You can rebase it if you want.

I want to make sure we're not overlooking anything. @dhh do you see any problems with this?

David Heinemeier Hansson
Owner

Looks OK to me.

Nick Reed reed merged commit b7bfa1c into from
Nick Reed reed referenced this pull request from a commit
Nick Reed reed Reflect changes from PR #222 66467fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on May 9, 2013
  1. Yasuharu Ozaki

    Maintain redirect options in session. Fix #221

    yasuoza authored
    To reflect redirect_to options, keep redirect url in session to be set
    'X-XHR-Current-Location' in response header.
Commits on May 10, 2013
  1. Yasuharu Ozaki

    Use X-XHR-Redirected-To instead of X-XHR-Current-Location

    yasuoza authored
    Set redirect location to X-XHR-Redirected-To header
    only when server redirects user.
    And use the location info in reflectRedirectedUrl.
This page is out of date. Refresh to see the latest.
Showing with 15 additions and 21 deletions.
  1. +3 −12 lib/assets/javascripts/turbolinks.js.coffee
  2. +12 −9 lib/turbolinks.rb
15 lib/assets/javascripts/turbolinks.js.coffee
View
@@ -106,9 +106,9 @@ reflectNewUrl = (url) ->
window.history.pushState { turbolinks: true, position: currentState.position + 1 }, '', url
reflectRedirectedUrl = ->
- location = xhr.getResponseHeader 'X-XHR-Current-Location'
- if location and location isnt normalizePath(document.location.pathname) + document.location.search
- window.history.replaceState currentState, '', location + document.location.hash
+ if location = xhr.getResponseHeader 'X-XHR-Redirected-To'
+ preservedHash = if removeHash(location) is location then document.location.hash else ''
+ window.history.replaceState currentState, '', location + preservedHash
rememberCurrentUrl = ->
window.history.replaceState { turbolinks: true, position: Date.now() }, '', document.location.href
@@ -136,15 +136,6 @@ removeHash = (url) ->
link.href = url
link.href.replace link.hash, ''
-# Strips off trailing slash and ensures there is a leading slash
-normalizePath = (path) ->
- path = "/#{path}"
- path = path.replace /\/+/g, '/'
- path = path.replace /\/+$/, ''
- path = '/' if path is ''
- path
-
-
triggerEvent = (name) ->
event = document.createEvent 'Events'
event.initEvent name, true, true
21 lib/turbolinks.rb
View
@@ -8,15 +8,18 @@ module XHRHeaders
private
def _compute_redirect_to_location_with_xhr_referer(options)
- if options == :back && request.headers["X-XHR-Referer"]
- _compute_redirect_to_location_without_xhr_referer(request.headers["X-XHR-Referer"])
- else
- _compute_redirect_to_location_without_xhr_referer(options)
- end
+ session[:_turbolinks_redirect_to] =
+ if options == :back && request.headers["X-XHR-Referer"]
+ _compute_redirect_to_location_without_xhr_referer(request.headers["X-XHR-Referer"])
+ else
+ _compute_redirect_to_location_without_xhr_referer(options)
+ end
end
- def set_xhr_current_location
- response.headers['X-XHR-Current-Location'] = request.fullpath
+ def set_xhr_redirected_to
+ if session[:_turbolinks_redirect_to]
+ response.headers['X-XHR-Redirected-To'] = session.delete :_turbolinks_redirect_to
+ end
end
end
@@ -48,10 +51,10 @@ class Engine < ::Rails::Engine
initializer :turbolinks_xhr_headers do |config|
ActionController::Base.class_eval do
include XHRHeaders, Cookies, XDomainBlocker
- before_filter :set_xhr_current_location, :set_request_method_cookie
+ before_filter :set_xhr_redirected_to, :set_request_method_cookie
after_filter :abort_xdomain_redirect
end
-
+
ActionDispatch::Request.class_eval do
def referer
self.headers['X-XHR-Referer'] || super
Something went wrong with that request. Please try again.