Fix page caching and lifecycle events

Fix #551. Partial replacement broke the page cache by caching body.outerHTML (losing all DOM state in the process) instead of the body element itself. This commit brings back the old behavior, with the following gotcha: when a partial replacement is performed, we remove the current page from the cache, since the body element will not change and there is no simple way for us to bring back the changed nodes. Additionally: - page:after-remove on body elements now triggers on cache eviction (since we don't want to lose DOM state) - to avoid triggering page:load on the same body elements more than once, a partial replacement now triggers the page:partial-load event
turbolinks · Jul 20, 2015 · 25f07dd · 25f07dd
1 parent 3c36267
commit 25f07dd
Show file tree

Hide file tree

Showing 5 changed files with 161 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -86,7 +86,11 @@
 
     *Kristian Plettenberg-Dussault*, *Thibaut Courouble*, *David Heinemeier Hansson*
 
-*   Add a `page:after-remove` event, triggered after a node (stored in `event.data`) is removed from the DOM, to give user scripts the opportunity to clean up references to these nodes and avoid memory leaks.
+*   Trigger `page:partial-load` instead `page:load` on partial replacement (`Turbolinks.visit()` or `Turbolinks.replace()` with `change` option)
+
+    *Thibaut Courouble*
+
+*   Add a `page:after-remove` event, triggered after an element (stored in `event.data`) is removed from the DOM (partial replacement), or a body element is evicted from the cache, to give user scripts the opportunity to clean up references to these elements and avoid memory leaks.
 
     This event replaces the `page:expire` event for cleaning up cached pages.
 
@@ -150,7 +154,7 @@
     Turbolinks.visit url, scroll: false
     ```
 
-*   Attach affected nodes to the `page:before-unload`, `page:change` and `page:load` events (in `event.data`).
+*   Attach affected elements to the `page:before-unload`, `page:change`, `page:load` and `page:partial-load` events (in `event.data`).
 
     *Thibaut Courouble*
 

diff --git a/lib/assets/javascripts/turbolinks.js.coffee b/lib/assets/javascripts/turbolinks.js.coffee
@@ -18,15 +18,20 @@ EVENTS =
   CHANGE:         'page:change'
   UPDATE:         'page:update'
   LOAD:           'page:load'
+  PARTIAL_LOAD:   'page:partial-load'
   RESTORE:        'page:restore'
   BEFORE_UNLOAD:  'page:before-unload'
   AFTER_REMOVE:   'page:after-remove'
 
 fetch = (url, options = {}) ->
   url = new ComponentUrl url
 
+  if options.change or options.keep
+    removeCurrentPageFromCache()
+  else
+    cacheCurrentPage()
+
   rememberReferer()
-  cacheCurrentPage()
   progressBar?.start()
 
   if transitionCacheEnabled and !options.change and cachedPage = transitionCacheFor(url.absolute)
@@ -67,12 +72,13 @@ fetchReplacement = (url, options) ->
     if doc = processResponse()
       reflectNewUrl url
       reflectRedirectedUrl()
-      loadedNodes = changePage doc, options
+      loadedNodes = changePage extractTitleAndBody(doc)..., options
       if options.showProgressBar
         progressBar?.done()
       manuallyTriggerHashChangeForFirefox()
       updateScrollPosition(options.scroll)
-      triggerEvent EVENTS.LOAD, loadedNodes
+      triggerEvent (if options.change then EVENTS.PARTIAL_LOAD else EVENTS.LOAD), loadedNodes
+      constrainPageCacheTo(cacheSize)
     else
       progressBar?.done()
       document.location.href = crossOriginRedirect() or url.absolute
@@ -92,7 +98,7 @@ fetchReplacement = (url, options) ->
 
 fetchHistory = (cachedPage, options = {}) ->
   xhr?.abort()
-  changePage createDocument(cachedPage.body), title: cachedPage.title, runScripts: false
+  changePage cachedPage.title, cachedPage.body, null, runScripts: false
   progressBar?.done()
   updateScrollPosition(options.scroll)
   triggerEvent EVENTS.RESTORE
@@ -102,14 +108,15 @@ cacheCurrentPage = ->
 
   pageCache[currentStateUrl.absolute] =
     url:                      currentStateUrl.relative,
-    body:                     document.body.outerHTML,
+    body:                     document.body,
     title:                    document.title,
     positionY:                window.pageYOffset,
     positionX:                window.pageXOffset,
     cachedAt:                 new Date().getTime(),
     transitionCacheDisabled:  document.querySelector('[data-no-transition-cache]')?
 
-  constrainPageCacheTo cacheSize
+removeCurrentPageFromCache = ->
+  delete pageCache[new ComponentUrl(currentState.url).absolute]
 
 pagesCached = (size = cacheSize) ->
   cacheSize = parseInt(size) if /^[\d]+$/.test size
@@ -122,15 +129,15 @@ constrainPageCacheTo = (limit) ->
   .sort (a, b) -> b - a
 
   for key in pageCacheKeys when pageCache[key].cachedAt <= cacheTimesRecentFirst[limit]
+    onNodeRemoved(pageCache[key].body)
     delete pageCache[key]
 
 replace = (html, options = {}) ->
-  loadedNodes = changePage createDocument(html), options
-  triggerEvent EVENTS.LOAD, loadedNodes
+  loadedNodes = changePage extractTitleAndBody(createDocument(html))..., options
+  triggerEvent (if options.change then EVENTS.PARTIAL_LOAD else EVENTS.LOAD), loadedNodes
 
-changePage = (doc, options) ->
-  [extractedTitle, targetBody, csrfToken] = extractTitleAndBody(doc)
-  title = options.title ? extractedTitle
+changePage = (title, body, csrfToken, options) ->
+  title = options.title ? title
   currentBody = document.body
 
   if options.change
@@ -143,18 +150,17 @@ changePage = (doc, options) ->
   document.title = title if title isnt false
 
   if options.change
-    changedNodes = swapNodes(targetBody, nodesToChange, keep: false)
+    changedNodes = swapNodes(body, nodesToChange, keep: false)
   else
     unless options.flush
       nodesToKeep = findNodes(currentBody, '[data-turbolinks-permanent]')
       nodesToKeep.push(findNodesMatchingKeys(currentBody, options.keep)...) if options.keep
-      swapNodes(targetBody, nodesToKeep, keep: true)
+      swapNodes(body, nodesToKeep, keep: true)
 
-    document.body = targetBody
-    onNodeRemoved(currentBody)
+    document.body = body
     CSRFToken.update csrfToken if csrfToken?
     setAutofocusElement()
-    changedNodes = [targetBody]
+    changedNodes = [body]
 
   scriptsToRun = if options.runScripts is false then 'script[data-turbolinks-eval="always"]' else 'script:not([data-turbolinks-eval="false"])'
   executeScriptTags(scriptsToRun)

diff --git a/test/javascript/iframe3.html b/test/javascript/iframe3.html
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<html>
+<head>
+  <meta charset="utf-8">
+  <title>title 3</title>
+  <meta content="authenticity_token" name="csrf-param">
+  <meta content="token3" name="csrf-token">
+  <script src="/js/jquery.js"></script>
+  <script src="/js/turbolinks.js"></script>
+</head>
+<body>
+
+</body>
+</html>
diff --git a/test/javascript/turbolinks_replace_test.coffee b/test/javascript/turbolinks_replace_test.coffee
@@ -38,7 +38,7 @@ suite 'Turbolinks.replace()', ->
     body = @$('body')
     permanent = @$('#permanent')
     permanent.addEventListener 'click', -> done()
-    beforeUnloadFired = afterRemoveFired = false
+    beforeUnloadFired = partialLoadFired = false
     @document.addEventListener 'page:before-unload', =>
       assert.isUndefined @window.j
       assert.notOk @$('#new-div')
@@ -48,14 +48,11 @@ suite 'Turbolinks.replace()', ->
       assert.equal @document.title, 'title'
       assert.equal @$('body'), body
       beforeUnloadFired = true
-    @document.addEventListener 'page:after-remove', (event) =>
-      assert.isNull event.data.parentNode
-      assert.equal event.data, body
-      assert.notEqual permanent, event.data.querySelector('#permanent')
-      afterRemoveFired = true
+    @document.addEventListener 'page:partial-load', (event) =>
+      partialLoadFired = true
     @document.addEventListener 'page:load', (event) =>
       assert.ok beforeUnloadFired
-      assert.ok afterRemoveFired
+      assert.notOk partialLoadFired
       assert.deepEqual event.data, [@document.body]
       assert.equal @window.j, 1
       assert.isUndefined @window.headScript
@@ -86,18 +83,15 @@ suite 'Turbolinks.replace()', ->
       </html>
     """
     body = @$('body')
-    beforeUnloadFired = afterRemoveFired = false
+    beforeUnloadFired = partialLoadFired = false
     @document.addEventListener 'page:before-unload', =>
       assert.equal @$('#permanent').textContent, 'permanent content'
       beforeUnloadFired = true
-    @document.addEventListener 'page:after-remove', (event) =>
-      assert.isNull event.data.parentNode
-      assert.equal event.data, body
-      assert.ok event.data.querySelector('#permanent')
-      afterRemoveFired = true
+    @document.addEventListener 'page:partial-load', (event) =>
+      partialLoadFired = true
     @document.addEventListener 'page:change', =>
       assert.ok beforeUnloadFired
-      assert.ok afterRemoveFired
+      assert.notOk partialLoadFired
       assert.equal @$('#permanent').textContent, 'new content'
       done()
     @Turbolinks.replace(doc, flush: true)
@@ -118,18 +112,15 @@ suite 'Turbolinks.replace()', ->
     body = @$('body')
     div = @$('#div')
     div.addEventListener 'click', -> done()
-    beforeUnloadFired = afterRemoveFired = false
+    beforeUnloadFired = partialLoadFired = false
     @document.addEventListener 'page:before-unload', =>
       assert.equal @$('#div').textContent, 'div content'
       beforeUnloadFired = true
-    @document.addEventListener 'page:after-remove', (event) =>
-      assert.isNull event.data.parentNode
-      assert.equal event.data, body
-      assert.notEqual body, event.data.querySelector('#div')
-      afterRemoveFired = true
+    @document.addEventListener 'page:partial-load', (event) =>
+      partialLoadFired = true
     @document.addEventListener 'page:change', =>
       assert.ok beforeUnloadFired
-      assert.ok afterRemoveFired
+      assert.notOk partialLoadFired
       assert.equal @$('#div').textContent, 'div content'
       assert.equal @$('#div'), div # :keep nodes are transferred
       @$('#div').click() # event listeners on :keep nodes should not be lost
@@ -159,7 +150,7 @@ suite 'Turbolinks.replace()', ->
     body = @$('body')
     change = @$('#change')
     temporary = @$('#temporary')
-    beforeUnloadFired = false
+    beforeUnloadFired = loadFired = false
     @document.addEventListener 'page:before-unload', =>
       assert.equal @window.i, 1
       assert.equal @$('#change').textContent, 'change content'
@@ -172,6 +163,8 @@ suite 'Turbolinks.replace()', ->
       assert.isNull event.data.parentNode
       assert.equal event.data, afterRemoveNodes.shift()
     @document.addEventListener 'page:load', (event) =>
+      loadFired = true
+    @document.addEventListener 'page:partial-load', (event) =>
       assert.ok beforeUnloadFired
       assert.equal afterRemoveNodes.length, 0
       assert.deepEqual event.data, [@$('#temporary'), @$('#change'), @$('[id="change:key"]')]
@@ -190,7 +183,10 @@ suite 'Turbolinks.replace()', ->
       assert.notEqual @$('#temporary'), temporary # temporary nodes are cloned
       assert.notEqual @$('#change'), change # changed nodes are cloned
       assert.equal @$('body'), body
-      done()
+      setTimeout =>
+        assert.notOk loadFired
+        done()
+      , 0
     @Turbolinks.replace(doc, change: ['change'])
 
   test "with :change and html fragment", (done) ->