Side navigation
#7752 closed bug (invalid)
Opened December 11, 2010 06:44PM UTC
Closed December 13, 2010 06:56PM UTC
Passing 1 function to toggle-event should be treated like .click(). Currently there are side effects.
| Reported by: | patrickwhalen | Owned by: | |
|---|---|---|---|
| Priority: | low | Milestone: | 1.6 |
| Component: | event | Version: | 1.4.4 |
| Keywords: | Cc: | ||
| Blocked by: | Blocking: |
Description
**This will likely not be considered a bug, since it involves an improper usage of the API. As such, I'm filing this as an "enhancement". I thought it merited consideration since it involves possible unexpected code execution and a potential infinite loop.**
If you incorrectly pass 1 function to the event version of
.toggle(), it seems to fire the handler automatically during setup. In other words, no actual event is required.
#!js
$('div').toggle(function() {
console.log( 'the handler fired' ); // Fires once on load.
});
http://jsfiddle.net/patrick_dw/jGwQA/
In addition, if an Error is thrown in the handler, the handler will continue to fire infinitely.
#!js
$('div').toggle(function() {
throw( 'some error' ); // Fires infinitely
});
http://jsfiddle.net/patrick_dw/jGwQA/1/
Again, passing 1 function is not a supported usage of the method. But given the potential outcome, I thought it may be appropriate to treat a toggle() with only one function as a normal click() handler.
In the
toggle()method:
https://github.com/jquery/jquery/blob/master/src/event.js#L971
...its implementation may be as simple as:
#!js
if( args.length === 1 && typeof fn === 'function' ) {
return this.click( fn );
}
Attachments (0)
Change History (5)
Changed December 11, 2010 06:49PM UTC by comment:1
Changed December 11, 2010 09:10PM UTC by comment:2
| component: | unfiled → event |
|---|---|
| priority: | undecided → low |
I don't know, jQuery, like a lot of other things, breaks when you pass invalid arguments to methods. This is one way that libraries and languages tell you that you are doing something wrong, so I don't think this is particularly urgent. By the same token, it's a pretty simple and reasonable fix - just use jQuery.isFunction instead. Do other folks think this is worth doing?
Changed December 11, 2010 09:10PM UTC by comment:3
| type: | enhancement → bug |
|---|
Changed December 12, 2010 02:32PM UTC by comment:4
| _comment0: | Replying to [comment:2 ajpiano]: \ > I don't know, jQuery, like a lot of other things, breaks when you pass invalid arguments to methods. \ \ True. Taking another look, I would bet that some folks out there pass a single function as the sole parameter when using {{{toggle()}}} as its "toggle effect" version (not the click handler version). Making it default to {{{click()}}} would break code unnecessarily. \ \ When I filed the original report, I had forgotten about the effect version of {{{toggle()}}}, but what really prompted it was the infinite loop. \ \ Doing another test, I see that the actual cause of the infinite loop is throwing an error in the {{{animate()}}} callback. \ \ {{{ \ #!js \ $('div').animate({width:100},function(){ \ throw('e'); \ }); \ }}} \ \ http://jsfiddle.net/patrick_dw/jGwQA/2/ \ \ Depending on what is taking place in the callback, it may be difficult to anticipate an error being thrown. Given an unanticipated error, the outcome could be devastating to the application. \ \ But then, I don't know if there's a simple fix for this outside of a `try/catch` around the callback, which may not be desired. → 1292164433929977 |
|---|
Replying to [comment:2 ajpiano]:
I don't know, jQuery, like a lot of other things, breaks when you pass invalid arguments to methods.
True. Taking another look, I would bet that some folks out there pass a single function as the sole parameter when using
toggle()as its "toggle effect" version (not the click handler version). Making it default to
click()would break code unnecessarily.
When I filed the original report, I had forgotten about the effect version of
toggle(), but what really prompted it was the infinite loop.
Doing another test, I see that the actual cause of the infinite loop is throwing an error in the
animate()callback.
#!js
$('div').animate({width:100},function(){
throw('e'); // Fires infinitely
});
http://jsfiddle.net/patrick_dw/jGwQA/2/
Depending on what is taking place in the callback, it may be difficult to anticipate an error being thrown. Given an unanticipated error, the outcome could be devastating to the application.
But then, I don't know if there's a simple fix for this outside of a try/catch around the callback, which may not be desired.
Changed December 13, 2010 06:56PM UTC by comment:5
| _comment0: | At this point, this ticket certainly no longer pertains to the original proposed enhancement, which we seem to have determined is probably not worth it, so I'm gonna go ahead and close it... \ \ The issue about the uncaught exceptions in animation callbacks can be tracked in 5684. → 1292266980852747 |
|---|---|
| resolution: | → invalid |
| status: | new → closed |
At this point, this ticket certainly no longer pertains to the original proposed enhancement, which we seem to have determined is probably not worth it, so I'm gonna go ahead and close it...
The issue about the uncaught exceptions in animation callbacks can be tracked in #5684.
Now I see that the issue is the double duty of
. As such, the enhancement is **invalid**, though the infinite loop aspect may be of interest.