fix behavior on redirect, no need for double requests #260

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
@lepture

lepture commented Jul 21, 2013

This patch fixes double requests for cross domain redirecting.

@chloerei

This comment has been minimized.

Show comment
Hide comment
@chloerei

chloerei Jul 21, 2013

processResponse()

Already check xhr.status, does not work?

processResponse()

Already check xhr.status, does not work?

This comment has been minimized.

Show comment
Hide comment
@lepture

lepture Jul 21, 2013

Owner

@chloerei the current implementation:

get '/redirect' do:
    return redirect('http://example.com')
  1. request '/redirect' with turbolinks
  2. server response with 403
  3. then turbolinks will request without turbolinks
  4. server response with 301/302
  5. location change
Owner

lepture replied Jul 21, 2013

@chloerei the current implementation:

get '/redirect' do:
    return redirect('http://example.com')
  1. request '/redirect' with turbolinks
  2. server response with 403
  3. then turbolinks will request without turbolinks
  4. server response with 301/302
  5. location change
@chloerei

This comment has been minimized.

Show comment
Hide comment
@chloerei

chloerei Jul 21, 2013

@lepture Got it, I suggest do like this:

if doc = processResponse()
  # ...
else
  document.location.href = xhr.getResponseHeader('Location') || url

@lepture Got it, I suggest do like this:

if doc = processResponse()
  # ...
else
  document.location.href = xhr.getResponseHeader('Location') || url
@lepture

This comment has been minimized.

Show comment
Hide comment
@lepture

lepture Jul 21, 2013

@chloerei I thought it should be before

triggerEvent 'page:receive'

lepture commented Jul 21, 2013

@chloerei I thought it should be before

triggerEvent 'page:receive'
@chloerei

This comment has been minimized.

Show comment
Hide comment
@chloerei

chloerei Jul 21, 2013

page:receive the page has been fetched from the server, but not yet parsed

I think trigger first is semantically. Although it's a redirect, not a page, but it's fetched from server, and not yet parsed.

page:receive the page has been fetched from the server, but not yet parsed

I think trigger first is semantically. Although it's a redirect, not a page, but it's fetched from server, and not yet parsed.

lepture added a commit to lepture/flask-turbolinks that referenced this pull request Jul 29, 2013

@reed reed closed this in d496f8b Oct 3, 2014

@reed

This comment has been minimized.

Show comment
Hide comment
@reed

reed Oct 3, 2014

Collaborator

This was a good idea, and I'm sorry it took SO long for me to notice it. I just committed a slightly more cautious implementation, but the functionality is the same.

Collaborator

reed commented Oct 3, 2014

This was a good idea, and I'm sorry it took SO long for me to notice it. I just committed a slightly more cautious implementation, but the functionality is the same.

@lepture lepture deleted the lepture:redirect branch Oct 5, 2014

sebastian-julius added a commit to blinkist/turbolinks that referenced this pull request Oct 20, 2014

Merge commit 'c5e4ca2e02547e3d0a0ef51bd879d6a41139444f'
* commit 'c5e4ca2e02547e3d0a0ef51bd879d6a41139444f': (52 commits)
  Fix typo
  Avoid double requests on cross origin redirects - Close #260
  v2.4.0
  Don't set request_method cookie for GET requests
  Change Turbolinks.events() array to Turbolinks.EVENTS hash
  Properly ignore empty hash links - Fix #407 again
  Handle HTML parsing bug in Safari 7.1 - Fix #408
  Check if Content-Type header exists before checking its value - Fix #410
  Ignore empty hash links when the current location ends in an empty hash - Fix #407
  expose event list via `Turbolinks.events()`
  Move event names into Turbolinks.EVENTS
  Provide target URL to page:before-change event - Close #357
  Maintain load order of scripts in the body - Fix #389
  Add Prototype compatibility with custom events - Fix #384
  Add page:before-unload event - Close #401
  Move cloning of link element inside the Link class to preserve ability to inspect surrounding elements
  Clarify page:update description in README - Fix #398
  Clone the link so attributes are not altered - Fix #399
  Include check for nil-session
  v2.3.0
  ...

Conflicts:
	lib/turbolinks.rb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment