Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Fix #10588. For now, event voyeurism only merits the look of disappro… 
…val.

If you are using `.data("events")` we would like to know how we can provide a documented interface that satisfies the need.
  • Loading branch information
@dmethvin
dmethvin committed Oct 26, 2011
1 parent 35bc30c commit 24e416d
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions src/data.js
View file
Expand Up @@ -101,9 +101,8 @@ jQuery.extend({
thisCache[ jQuery.camelCase( name ) ] = data;
}

// TODO: This is a hack for 1.5 ONLY. It will be removed in 1.6. Users should
// not attempt to inspect the internal events object using jQuery.data, as this
// internal data object is undocumented and subject to change.
// Users should not attempt to inspect the internal events object using jQuery.data,
// it is undocumented and subject to change. But does anyone listen? No.
if ( name === "events" && !thisCache[name] ) {
return thisCache[ internalKey ] && thisCache[ internalKey ].events;
}
Expand Down

38 comments on commit 24e416d

@gnarf
Copy link
Member

@gnarf gnarf commented on 24e416d Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I have used this when debugging... $($0).data("events") is useful from chrome console...

http://jqbug.com/10589 is a ticket to discuss how we should replace/remove this in the future...

@gnarf
Copy link
Member

@gnarf gnarf commented on 24e416d Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be as clear as possible:

Don't let discovering that this exists be a reason to start using it, we still plan on making it disappear someday...

@dcherman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it for debugging, or at times for re-ordering already bound events if I need to bind something but have it fire first like this:

el.data( 'events' ).keydown.unshift( el.data( 'events' ).keydown.pop() );

@ryanneufeld
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps taking it out would break useful plugins like this: http://firequery.binaryage.com/

@SlexAxton
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as everyone else. Debugging, and the occasional crazy hack.

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dcherman, that is very likely to break in 1.7 because we now store delegated and direct events in the same array element, with delegated events at the front of the array. There is a .delegateCount property on the array that would be very confused by that code if you had mixed direct/delegated events.

@ryanneufeld, if a plugin wants to dissect internal jQuery data for debugging it's welcome to do so, but yeah it is likely to break on major changes to internal data and they'll need to rewrite stuff in order to display it properly again.

@aaronpowell
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to agree with everyone else, I've used it for debugging and working out what handlers exist (when a handler doesn't fire as expected)

@oroce
Copy link

@oroce oroce commented on 24e416d Oct 26, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've used it for debugging too (for some crazy bugs in opera).

@aFarkas
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using Firequery for debugging, which will break with the change. The information about the mixed and the delegated events is really usefull, too, One of my plugins is using 'stopImmediatePropagation' on a standard event. And for this event, I have to make sure, that my event handler is the first (to stop all others), so I'm doing similiar unshift action like dcherman.

How would you propose a solution, using jQuery 1.7? (In my case the event is bound 'lazy' to the element)

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aFarkas, Firequery can easily reverse engineer the structure; I'd encourage them to do so and can provide whatever help they need. For debugging I have no problem describing the data structures. I'm more concerned about ending up needing to formally support something a few people have reverse engineered but we never formally documented.

As for out-firsting the other event handlers, it would really depend on what you were trying to do. If there are no other delegated handlers on the element, you could use the fact that delegated handlers run first to your advantage in this situation: http://jsfiddle.net/dmethvin/rxHjd/

I have no idea how the "*" selector performs for that case but if you use it I am sure you'll find out. :)

@dcherman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just copying something here that I said in IRC, but that's just an example of a use case I had, but there are some very obvious workarounds I can implement (read: fix my event order ).

That said, there's still an evil way of doing it using Array.splice to take the delegateCount into consideration, but that's probably just as bad of an idea as mucking with the events array was in the first place :)

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point I should make here that might not be clear from the code. Our goal is to remove the "events" data name from the public data object, so it won't collide accidentally with names that users may choose and send jQuery events into a tailspin. You can still get to the internal data structure via jQuery._data(elem, "events") but the underscore is a rickety bridge with a scary troll under it that you must cross at your own risk. If you're only using this for debugging, convert to the underscorey version and don't worry about it.

@gnarf
Copy link
Member

@gnarf gnarf commented on 24e416d Oct 27, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A prototypal simple debugging tool:

$.fn.getEvents = function() {
    return this[ 0 ] && $._data( this[ 0 ], "events" );
}

@gustaff-weldon
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it for debugging, ex. in IE :) where I can't use firequery.

@shaneriley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I use it to check if an event exists on an element. https://gist.github.com/1250910

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaneriley, in production code? Do you have some examples?

Do you really need the namespace support? A $.fn.hasEvents can be really small without it and doesn't need to loop through the events:

http://jsfiddle.net/dmethvin/nHdgH/

@shaneriley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed it to check for the presence of particular namespaced events to determine if a "plugin" was loaded and initialized before taking action.

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For that particular case I think it would be better to have the plugin set a data property on its frame element to indicate it's initialized. That definitely would be faster and less fragile than iterating through the internal events object.

@shaneriley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would require modifying plugins to do that, and given plugins are updated regularly it's likely that the edits will be lost with a plugin update.

In any event, it would be nice to still be able to check events attached to a particular DOM element.

@dcherman
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaneriley: No need for that, just make a wrapper for the plugins you need to save state on.

Something like https://gist.github.com/1319733

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although that wouldn't work if the plugin wasn't truly ready until it did some sort of asynchronous initialization such as an ajax. However, in that case the plugin should have some sort of callback or status indication that it is truly ready, and that could be used to determine readiness rather than trying to infer it by looking at attached events. If the plugin isn't ready immediately and doesn't have some sort of external flag to indicate its readiness, it has a serious design issue that needs to be addressed.

@shaneriley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example:
I have an anchor that is supposed to open a modal via a plugin when clicked and the anchor has a second event simply bound to the click event with no namespace. Elsewhere on the page, I want to open the same modal but there's a possibility that certain actions that place the click event on the anchor haven't transpired. I can easily check that by looking for a namespaced event on the anchor, and if it exists, trigger the namespaced event only.

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wrote a plugin which is based on data("events") : https://github.com/sebastien-p/jquery.hasEventListener. So I guess I will just have to use _data instead. Not a big deal.

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastien-p, using _data() will be a start, but since the internal organization of the data structure has changed you'll need to make other changes as well.

From the plugin's readme:

Some people do something like $("#some_element").unbind("click").bind("click", function () {}); which works but is unclean and calls the unbind method even when not needed.

I am not sure how .unbind("click") is more unclean than inspecting an undocumented internal data structure with a 450-line plugin. If that code had a namespace for the event I think it would be just fine. Or the code can just track that it's already done initialization and doesn't need to re-attach an event handler. Or it could use delegated events. Any of those are simpler, smaller, faster, and more robust than using the plugin.

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmethvin : The thing is that we can't control every third-party plugins that other people wrote which sometimes contain very bad code that can lead to, for example, memory leaks. That's why I (and some other guys as we can find some related questions on websites like stackoverflow) think that a hasEvent method could be useful. So, I wrote this plugin. Writing something like elem.hasEvent("delegate!type.namespace") || elem.parents("selector").delegate("selector", "type.namespace", handler) is - in my opinion - cleaner (but longer and slower, I know) than elem.parents("selector").undelegate("selector", "type.namespace").delegate("selector", "type.namespace", handler). I agree that such a plugin is most of the time an overkill solution to the common problems we - as jQuery developers - face off. Anyway, I would love to see a better solution for both cases as part of a future jQuery API but, for now, I think that people who see any usefulness to those existing "hasEvent-like" plugins should be able to rely on them, don't they ?

@gnarf
Copy link
Member

@gnarf gnarf commented on 24e416d Oct 27, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastien-p The catch here being that changes to the internal storage of jQuery events has and will evolve from version to version. Plugins like yours will need to be carefully examined every time a new version of jQuery is released to stay up to date with those internal storage structures. There is a reason why we store the event data in our "private" data cache, mucking with it is dangerous and will almost surely lead to very hard to trace bugs as the library evolves.

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnarf37 : So, what about an official "hasEvent-like" support in a near future ? :) This could be as useful for debugging as for other use cases people will think of for sure.

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elem.hasEvent("delegate!type.namespace") || elem.parents("selector").delegate("selector", "type.namespace", handler)

So this is trying to avoid a double-attach of the event handler? How do you know that the hasEvent you detected is handler?

Is it fair to say that (other than debugging) the need for looking at the events array is primarily because jQuery doesn't ignore duplicate attaches of the same handler? Again, it seems like most code should be able to know whether it has initialized itself or not, but I am trying to understand this situation since I haven't been in it myself. And there are situations where you might want to attach the same handler, but with different data arguments or closure data.

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ dmethvin : you also can do elem.hasEvent("delegate!type.namespace", handler) || elem.parents("selector").delegate("selector", "type.namespace", handler). Because I'm not that good at english it is kind of hard for me to explain myself like I would like to, sorry. I am not saying that the events datas should be public but the fact is I think (and I am not the only one) that a "hasEvent-like" method could sometimes be useful (why do other languages have it ?). And I am pretty sure that hiding events will break a lot of existing plugins, not only mine.

@rwaldron
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastien-p:

The thing is that we can't control every third-party plugins that other people wrote which sometimes contain very bad code that can lead to

Please understand that this is exactly the same blocking issue that your plugin creates for jQuery Core

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwldrn : oh, why ? I came here to talk about this, can't call it "blocking" because I am not complaining at all... or maybe I misspoke, sorry.

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastien-p, nothing is being hidden. We are trying to protect the data structure from accidentally being clobbered. Note my message above:

One point I should make here that might not be clear from the code. Our goal is to remove the "events" data name from the public data object, so it won't collide accidentally with names that users may choose and send jQuery events into a tailspin. You can still get to the internal data structure via jQuery._data(elem, "events") but the underscore is a rickety bridge with a scary troll under it that you must cross at your own risk. If you're only using this for debugging, convert to the underscorey version and don't worry about it.

@gnarf
Copy link
Member

@gnarf gnarf commented on 24e416d Oct 27, 2011

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastien-p - Blocking in the sense that we can't actually remove the data("events") code until we evaluate where/why people are using it, and decide if we should be providing better alternatives.

hasEvent might be a use case we could provide support for, but I'm siding with @dmethvin here. I think there will always be a better alternative design than peeking at attached event handlers. This is evidenced by the fact that the pure DOM events have no similar method. NOTE: I still feel that the debugging clause has its uses, but you shouldn't ever need a hasEvent in production code.

@sebastien-p
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally understand what you guys are saying and I agree. I am sure a nice solution will came out of this thread, as always :)

@shaneriley
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of curiosity, does that mean that the detach method will no longer retain events if you move them from a data object?

@dmethvin
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shaneriley, there shoudn't be any changes to the documented API behavior from this change.

@julianjelfs
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a javascript event aggregator component in our application so that unrelated components can publish and subscribe as means of maintaining a loose coupling between the components. We use jQuery custom event binding under the hood to achieve this. There are some rare circumstances where a component must only subscribe to an event if nothing else has subscribed to it. So we implemented a sort of isSubscribed function on our event aggregator. Under the hood this delves into the data("events") to work out the answer. I did this in the full understanding that it was dodgy and that I was running the risk of it breaking but it works for me at the moment and satisfies a requirement. If there were some sort of hasEvent thing, it would also solve my problem. I'm pretty sure I could work out some other way to do it, but for me it's a +1 for hasEvent.

@dmethvin
Copy link
Member Author

@dmethvin dmethvin commented on 24e416d Nov 2, 2011 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.