Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Revert unbinding of all event delegates #19

Merged
merged 1 commit into from

3 participants

@rstacruz
Collaborator

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 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].

@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
@glebm

This breaks #8

@glebm

#8 is [B] by the way.
A way to sort of fix both is to override on to ignore duplicate bindings on document

Note that although doing [B] might seem strange, it is actually quite common (i.e. happens in a lot of codebases that I've seen).

@rstacruz
Collaborator

Even if there's a patch to override on to disallow duplicates, this will still break the behavior of rails_ujs.

Despite [B] existing in many codebases, [A] still the technically-superior solution as there would be less code invoked on every page load, and less code needed from Turbolinks or jQuery.Turbolinks to work. [A] exists in many codebases as well.

@glebm

What I am talking about is overriding on + removing off completely (like in this pull request). Such hack would make both [A] and [B] work! (Overall, it is more important that [A] works of course.)

/cc @kossnocorp

@rstacruz
Collaborator

Ah, my mistake. How can you reliably handle duplicates, though? Let's say:

var onload = function() {
  $(document).on('click', 'a.button', function() {
    console.log("Clicked!");
  });
};

// Call it twice:
onload();
onload();

The handler (the anonymous function) on the two calls will be different instances of Function. To illustrate:

// Let's say we monkey-patched $.fn.on with something like this:
var handlers = [];
function addHandler(handler) {
  for (var i = 0; i < handlers.length; ++i) {
    if (handler === handlers[i]) {
      console.log("Duplicate found!");
    }
  }
  handlers.push(handler);
}

// Then let's simulate a $(...):
var onload = function() {
  addHandler(function() { console.log("Hello"); });
};

// And call it twice:
onload(); onload();

This example will not yield "Duplicate found!" because the functions created in the two onload() calls are different.

@rstacruz
Collaborator

Looking at your code, it looks like the same handler === handlers[i] comparison is used. I tried injecting a console.log in there to intercept when it happens:

originalOn = $.fn.on
$.fn.on = (types, selector, data, fn) ->
  if selector == document && fn
    for type in types when fn
      handlers = $._data(@, 'handlers')[type] || []
      for h in handlers when h == fn
        console.log "Double-binding prevented!" # <-- here
        return @
  originalOn.apply(@, arguments)

# Simulate calling the onload handler twice
for i in [1..2]
  $(document).on 'click', 'a.button', ->
    alert "Clicked"

...as expected, it doesn't yield "Double-binding prevented!" for the same reason. I also tried other ways of comparison (handlers.indexOf(h)).

There will never be a comparison that will work except:

for h in handlers when h.toString() is fn.toString()

...but personally, I'm not comfortable with this approach because Function.prototype.toString behavior is not standardized. While it works on Webkit and Gecko, there's no guarantee it'll work elsewhere.

@kossnocorp kossnocorp merged commit a608907 into from
@kossnocorp
Owner

Thank you a lot!

I'm gonna release it as v1.0.0-rc2.

@rstacruz can you also describe problem with [B] and recommended solution in README?

@rstacruz rstacruz deleted the branch
@rstacruz
Collaborator

Also, some relevant reading: https://coderwall.com/p/ypzfdw :-)

@rstacruz rstacruz referenced this pull request from a commit in rstacruz/jquery.turbolinks
@rstacruz rstacruz Document caveats of binding document events
This describes the side-effects of jQuery Turbolinks in setups wherein events are bound to $(document).

Related: #8, #19.
008aba4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 24, 2013
  1. @rstacruz

    Revert unbinding of all event delegates.

    rstacruz authored
    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/
This page is out of date. Refresh to see the latest.
Showing with 0 additions and 19 deletions.
  1. +0 −18 spec/jquery.turbolinks_spec.coffee
  2. +0 −1  src/jquery.turbolinks.coffee
View
18 spec/jquery.turbolinks_spec.coffee
@@ -46,24 +46,6 @@ describe '$ Turbolinks', ->
callback1.should.have.been.calledWith($)
- it '''
- should remove all events delegated to
- document after trigger fetch
- ''', ->
- id = getUniqId()
- selector = '#' + id
- addEl = ->
- $('body').empty()
- $('<div>').attr(id: id).appendTo('body')
-
- addEl()
- $(document).on('event_name', selector, callback1)
- $(selector).trigger('event_name')
- $(document).trigger('page:fetch')
- addEl()
- $(selector).trigger('event_name')
- callback1.should.have.been.calledOnce
-
describe '$.setReadyEvent', ->
beforeEach ->
View
1  src/jquery.turbolinks.coffee
@@ -24,7 +24,6 @@ turbolinksReady = ->
# Fetch event handler
fetch = ->
- $(document).off(undefined, '**')
$.isReady = false
# Bind `ready` to DOM ready event
Something went wrong with that request. Please try again.