Issue with rails_ujs #12

Closed
glebm opened this Issue Dec 16, 2012 · 10 comments

Projects

None yet

3 participants

@glebm
glebm commented Dec 16, 2012

Delete link (data-method=delete data-confirm="Yes?") + rails ujs + turbolink'd page (after page change) = fail
Likely related to how rails_ujs binds events

@kossnocorp
Owner

This problem because of https://github.com/kossnocorp/jquery.turbolinks/blob/master/src/jquery.turbolinks.coffee#L27 and this: #8. Any ideas how to fix it?

@kossnocorp
Owner

Look like I need to hack $.fn.on method to prevent double handlers on document.

@glebm
glebm commented Jan 16, 2013

Are you thinking of preventing double handlers by checking if a function has already been binded?
Something like this? (not tested)

diff --git a/src/jquery.turbolinks.coffee b/src/jquery.turbolinks.coffee
index 405e608..0def112 100644
--- a/src/jquery.turbolinks.coffee
+++ b/src/jquery.turbolinks.coffee
@@ -24,7 +24,6 @@ turbolinksReady = ->

 # Fetch event handler
 fetch = ->
-  $(document).off(undefined, '**')
   $.isReady = false

 # Bind `ready` to DOM ready event
@@ -47,6 +46,14 @@ $.setFetchEvent = (event) ->
     .off('.turbolinks-fetch')
     .on(event + '.turbolinks-fetch', fetch)

+originalOn = $.fn.on
+$.fn.on = (types, selector, data, fn) ->
+  if selector == document && fn
+    for type in types when fn
+      handlers = $._data(@, 'handlers')[type] || []
+      return @ for h in handlers when h == fn
+  originalOn.apply(@, arguments)
+
 # Bind `ready` to Tubolinks page:load
 $.setReadyEvent('page:load')
@glebm glebm added a commit to glebm/jquery.turbolinks that referenced this issue Jan 16, 2013
@glebm glebm Fix for #12 9445382
@glebm glebm added a commit to glebm/jquery.turbolinks that referenced this issue Jan 16, 2013
@glebm glebm Fix for #12 3d59109
@glebm
glebm commented Jan 16, 2013

Whoever wants to test if this prevents double submits, you can use:

gem 'jquery-turbolinks', github: 'glebm/jquery.turbolinks', branch: 'refs-12' 
@rstacruz
Collaborator

-1 on this... changing jQuery behavior (supressing double-binding) seems like a very bad idea.

If events are double-fired, they are most likely bound twice. If they are bound twice, they're most likely executed twice because you have your script tags in <body>, not <head>.

Try moving all your scripts to <head>, and ensuring that your load order is jquery.js -> jquery.turbolinks.js -> other scripts -> turbolinks.js.

@glebm
glebm commented Jan 18, 2013

Double-firing in the referred case happens because it's binded in jQuery(->) which gets re-evaluated every page:change

@rstacruz
Collaborator

Rails-ujs doesn't work inside a jQuery(->) block. It hooks events via delegate ($(document).delegate('[data-method]', function() {...} )), not binding to the elements directly.

I suspect that you only need change your load order to this:

<html>
<head>
  <script src='jquery.js'></script>
  <script src='jquery.turbolinks.js'></script>
  <script src='jquery_ujs.js'></script>
  <!-- other scripts here -->
  <script src='turbolinks.js'></script>
</head>
<body>
  ...
</body>
</html>

Note that it's inside <head> and not inside the body.

@glebm
glebm commented Jan 19, 2013

I was referring to #8. That issue is the reason jquery.turbolinks started clearing the document events on fetch:

$(document).off(undefined, '**')

Which is why your code above will stop working after the first fetch.

My patch removes the off, but adds a duplication check on on

@rstacruz
Collaborator

Why is the duplicate check of on needed?

If you bind your delegates (eg, $(document).on('click', 'button', ..)) outside a $(document).ready wrapper, double-binding shouldn't be a problem.

@rstacruz rstacruz added a commit to rstacruz/jquery.turbolinks that referenced this issue Jan 24, 2013
@rstacruz rstacruz Revert unbinding of all event delegates.
This reverts the change introduced in #8. This fixes #14 and #12.

The code `$(document).off(undefined, '**')` breaks 3rd-party
script behavior where events are delegated outside a `document.ready`
wrapper, like Rails's jquery_ujs.

For instance, having code that looks like this will fail after
navigating to a second page:

    // [A]
    $(document).on('click', 'button', function() { ... })

...this is exactly how scripts like jquery_ujs binds its events, and is
prescribed in jQuery's documentation for [fn.live].

To get around it, some people may misguidedly wrap it in a
`document.ready` wrapper like so. This is inefficient: events will then
be bound and un-bound repeatedly as you navigate through pages.

    // [B]
    $(function() {
      $(document).on('click', 'button', function() { ... })
    });

Therefore, [A] should be the recommended solution. This fix makes [A]
work again, but breaks the behavior of [B].

[fn.live]: http://api.jquery.com/live/
9b0661e
@rstacruz
Collaborator

I think this can be closed now with 1.0.0-rc2. :)

@kossnocorp kossnocorp closed this Feb 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment