Clean up jQuery data on node removal

turbolinks · Apr 4, 2015 · 04068f1 · 04068f1
1 parent b8d43ac
commit 04068f1
Show file tree

Hide file tree

Showing 8 changed files with 28 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -70,12 +70,16 @@
 
     *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 and data related to these nodes (e.g. `jQuery.fn.remove()`), and avoid memory leaks.
+*   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.
 
     This event replaces the `page:expire` event for cleaning up cached pages.
 
     *Thibaut Courouble*, *Drew Martin*
 
+*   Clean up jQuery event handlers automatically when the `page:after-remove` event is fired (fix memory leak).
+
+    *Thibaut Courouble*, *Drew Martin*
+
 *   Fix `URI::InvalidURIError` when `X-XHR-Referer` is invalid and the request performs a redirection.
 
     *Thibaut Courouble*

diff --git a/README.md b/README.md
@@ -35,7 +35,7 @@ With Turbolinks pages will change without a full reload, so you can't rely on `D
 * `page:fetch` starting to fetch a new target page
 * `page:receive` the page has been fetched from the server, but not yet parsed
 * `page:before-unload` the page has been parsed and is about to be changed
-* `page:after-remove` a node (stored in `event.data`) has been removed from the DOM and should be cleaned up (e.g. with `jQuery.fn.remove()`)
+* `page:after-remove` a node (stored in `event.data`) has been removed from the DOM and should be cleaned up *(jQuery data is cleaned up automatically)*
 * `page:change` the page has been changed to the new version (and on DOMContentLoaded)
 * `page:update` is triggered alongside both page:change and jQuery's ajaxSuccess (if jQuery is available - otherwise you can manually trigger it when calling XMLHttpRequest in your own code)
 * `page:load` is fired at the end of the loading process.

diff --git a/lib/assets/javascripts/turbolinks.js.coffee b/lib/assets/javascripts/turbolinks.js.coffee
@@ -143,7 +143,7 @@ changePage = (doc, options) ->
       swapNodes(targetBody, nodesToBeKept, keep: true)
 
     existingBody = document.documentElement.replaceChild(targetBody, document.body)
-    triggerEvent(EVENTS.AFTER_REMOVE, existingBody)
+    onNodeRemoved(existingBody)
     CSRFToken.update csrfToken if csrfToken?
     setAutofocusElement()
 
@@ -176,9 +176,14 @@ swapNodes = (targetBody, existingNodes, options) ->
       else
         targetNode = targetNode.cloneNode(true)
         existingNode.parentNode.replaceChild(targetNode, existingNode)
-        triggerEvent(EVENTS.AFTER_REMOVE, existingNode)
+        onNodeRemoved(existingNode)
   return
 
+onNodeRemoved = (node) ->
+  if typeof jQuery isnt 'undefined'
+    jQuery(node).remove()
+  triggerEvent(EVENTS.AFTER_REMOVE, node)
+
 executeScriptTags = ->
   scripts = document.body.querySelectorAll 'script:not([data-turbolinks-eval="false"])'
   for script in scripts when script.type in ['', 'text/javascript']

diff --git a/package.json b/package.json
@@ -8,6 +8,7 @@
   },
   "devDependencies": {
     "mocha": "~2.2.0",
-    "chai": "~2.2.0"
+    "chai": "~2.2.0",
+    "jquery": "~2.1.0"
   }
 }
diff --git a/test/config.ru b/test/config.ru
@@ -7,6 +7,7 @@ Assets = Sprockets::Environment.new do |env|
   env.append_path File.join(Root, "lib", "assets", "javascripts")
   env.append_path File.join(Root, "node_modules", "mocha")
   env.append_path File.join(Root, "node_modules", "chai")
+  env.append_path File.join(Root, "node_modules", "jquery", "dist")
   env.append_path File.join(Root, "test", "javascript")
 end
 

diff --git a/test/javascript/iframe.html b/test/javascript/iframe.html
@@ -11,6 +11,7 @@
       window.setTimeout = function(fn) { return originalSetTimeout(fn, 0); };
     })();
   </script>
+  <script src="/js/jquery.js"></script>
   <script src="/js/turbolinks.js"></script>
 </head>
 <body>

diff --git a/test/javascript/iframe2.html b/test/javascript/iframe2.html
@@ -11,6 +11,7 @@
       window.setTimeout = function(fn) { return originalSetTimeout(fn, 0); };
     })();
   </script>
+  <script src="/js/jquery.js"></script>
   <script src="/js/turbolinks.js"></script>
   <script>var headScript = true</script>
 </head>

diff --git a/test/javascript/turbolinks_visit_test.coffee b/test/javascript/turbolinks_visit_test.coffee
@@ -200,3 +200,13 @@ suite 'Turbolinks.visit()', ->
         @window.document.querySelector('#permanent').click()
     @Turbolinks.pagesCached(0)
     @Turbolinks.visit('iframe2.html')
+
+  test "jquery cleanup", (done) ->
+    body = @document.body
+    @window.jQuery(body).on 'click', ->
+      done new Error("jQuery event wasn't cleaned up")
+      done = null
+    @document.addEventListener 'page:load', ->
+      body.click()
+      setTimeout (-> done?()), 0
+    @Turbolinks.visit('iframe2.html')