Merge pull request #575 from Thibaut/cache

Fix page caching and lifecycle events

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*
 

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)

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>

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) ->