Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Always use the absolute URL when caching a page and looking up a cached page #334

Closed
wants to merge 2 commits into from

3 participants

@k1w1

This avoids having two versions of a page cached, one with a relative URL and one with an absolute URL.

I have been experimenting with this on aha.io and it fixes issue #333. I don't know the Turbolinks code well enough to know if there are other implications of this change.

@k1w1 k1w1 Always use the absolute URL when caching a page and looking up a cach…
…ed page. This avoids having two versions of a page cached, one with a relative URL and one with an absolute URL.
481fed7
@smidwap

As @reed mentioned in #336, this won't quite work. If @reed's refactor for some reason doesn't move forward, the essence of the fix is this:

Step 1. Add a function absoluteUrl that transforms a url to an absolute one

absoluteUrl = (url) ->
  (link = document.createElement 'a').href = url
  link.href

Step 2. Use absoluteUrl in the places where you changed to using document.location.href

@smidwap

@k1w1 did you happen to delete your comment? I received it by email, but I don't see it on GitHub any longer. I'll include it at the bottom for reference.

Can you describe the problem more specifically? I'm having troubles reproducing. What is the setup for your redirect? Is it a link or a form? Why does it redirect at one point but then not redirect later?

And to be clear, this is not a problem with absoluteUrl(...) but with currentState.url, correct?


I am seeing a problem with @smidwap's approach. I modified cacheCurrentPage to be:

cacheCurrentPage = function() {
pageCache[absoluteUrl(currentState.url)] = {

The problem is that after a redirect, currentState.url contains the original URL, not the destination of the redirect. This means that the current page is cached to the wrong URL, leaving a stale version of the page in the cache which is used when the user navigates back to this page normally (not through the redirect).

@k1w1

Sorry, I deleted the comment because I realized that I might be misunderstanding how turbolinks is supposed to work. In this particular case it is possible to get to the same URL via a redirect, and also from a link. I am not 100% sure yet whether it is working or not. I will post again if I can more clearly reproduce a problem.

@smidwap

Sounds good. I don't use redirects with Turbolinks often. Instead, I try to use Turbolinks.visit(...) in js responses. May not fit every use case, though.

@smidwap

@reed do you think it'd be worth merging this PR to get the bug fixed and consider your refactor separately? I know this issue affects my app as well, so it'd be nice to get it straightened out.

@reed
Collaborator

No, I don't think it's worth it. It only fixes part of the problem. My solution, as far as I can tell, covers all of it. It ensures absolute urls are used not just for cache keys but in state objects as well. This solution completely ignores the cache lookup in the history change handler. Which means that if, for example, all you use are relative urls in your app, pages will be cached by absolute url but every state object will use a relative one. Anytime you hit the back or forward button, it will use the state object's relative url to find the page in the cache and it will fail. So merging this PR would actually create a bigger issue than the one it's trying to fix.

All I'm waiting on is someone besides me to confirm that my PR works and fixes the issue. If either one of you can do that, I'll pull it in.

@reed reed closed this in 417a9d5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 24, 2014
  1. @k1w1

    Always use the absolute URL when caching a page and looking up a cach…

    k1w1 authored
    …ed page. This avoids having two versions of a page cached, one with a relative URL and one with an absolute URL.
Commits on Feb 5, 2014
  1. @k1w1
This page is out of date. Refresh to see the latest.
View
7 lib/assets/javascripts/turbolinks.js.coffee
@@ -17,7 +17,7 @@ fetch = (url) ->
cacheCurrentPage()
reflectNewUrl url
- if transitionCacheEnabled and cachedPage = transitionCacheFor(url)
+ if transitionCacheEnabled and cachedPage = transitionCacheFor(absoluteUrl(url))
fetchHistory cachedPage
fetchReplacement url
else
@@ -63,7 +63,7 @@ fetchHistory = (cachedPage) ->
cacheCurrentPage = ->
- pageCache[currentState.url] =
+ pageCache[absoluteUrl(currentState.url)] =
url: document.location.href,
body: document.body,
title: document.title,
@@ -139,6 +139,9 @@ resetScrollPosition = ->
else
window.scrollTo 0, 0
+absoluteUrl = (url) ->
+ (link = document.createElement 'a').href = url
+ link.href
# Intention revealing function alias
removeHashForIE10compatiblity = (url) ->
View
2  turbolinks.gemspec
@@ -1,6 +1,6 @@
Gem::Specification.new do |s|
s.name = 'turbolinks'
- s.version = '2.2.0'
+ s.version = '2.2.1'
s.author = 'David Heinemeier Hansson'
s.email = 'david@loudthinking.com'
s.license = 'MIT'
Something went wrong with that request. Please try again.