Correction in the optimization of Function#bindAsEventListener
Reported by John-David Dalton | August 11th, 2008 @ 07:00 PM | in 1.6.0.4
Tobie has pointed out that the optimization of bindAsEventListener may not be needed and/or can at least be further optimized
bindAsEventListener: function() {
var __method = this, args = $A(arguments), object = args.shift();
// Avoid using Array#concat when only the context argument is given.
if (args.length) {
return function(event) {
return __method.apply(object, [event || window.event].concat(args));
};
}
return function(event) {
/* old code
return __method.apply(object, [event || window.event]);
*/
return __method.call(object, event || window.event); //<- better because it doesn't create an array each time and uses call for a single argument
};
},
Comments and changes to this ticket
-
John-David Dalton August 11th, 2008 @ 07:08 PM
- → State changed from “new” to “enhancement”
-
Juriy Zaytsev August 11th, 2008 @ 07:09 PM
- → State changed from “enhancement” to “new”
Ah, I missed this one : ) Let's add this change to methodize/wrap patch (which also changes
applytocallin the second branch) http://prototype.lighthouseapp.c... -
-
John-David Dalton August 12th, 2008 @ 06:17 PM
- → State changed from “new” to “enhancement”
-
Mark Caudill August 25th, 2008 @ 07:57 PM
Does this just need a patch (and testing) of the code provided by John?
-
Juriy Zaytsev August 25th, 2008 @ 09:31 PM
@Mark This is a performance patch, so tests are not needed (as long as all current function extensions tests pass)
-
Mark Caudill August 27th, 2008 @ 01:18 AM
- no changes were found...
-
Mark Caudill August 27th, 2008 @ 01:18 AM
- → Tag changed from “needs_patch needs_tests” to “patched”
Here's the patch... It tested out OK on Safari 3.1.2 (XP), IE7, Firefox 3.0.1 and Opera 9.52.
This looks like everything (on 1.6.0.3) is patched but some things need tests (extractScripts definitely).
-
Tobie Langel September 29th, 2008 @ 11:01 PM
- → Tag changed from “patched” to “patched tested”
- → Milestone changed from “1.6.0.3” to “1.6.0.4”
-
Tobie Langel October 3rd, 2008 @ 12:15 AM
- → State changed from “enhancement” to “invalid”
OK, I looked at this one a bit more. Function#bindAsEventListener's unique use is partial application (currying). For any other use, Function#bind is a better fit.
There's therefore no incentive to branch the code for a performance enhancement that should never be called if the API is used correctly.
I'm closing this as invalid (cause I couldn't find wontfix ;) ).
I however suggest that someone writes a patch for the upgrade helper wrapping Function#bindAsEventListener and logging a message advising to use Function#bind if there is no currying.
-
Juriy Zaytsev October 3rd, 2008 @ 01:56 AM
That's pragmatic : )
Ok, let's leave
bindAsEventListeneras it is for now, although I remember seeing it "misused" much more often that is is used "properly". Main reason must be the fact thatbindAsEventListenerjust sounds necessary to use when binding event listeners. Without reading documentation it might seem obvious to a beginner, that good oldbindis somehow not sufficient :)By adding couple more lines, we would fool-proof future use and give an immediate boost to existing code. We could still explain the proper usage, of course.
Nitpicking aside, this change was a part of a bigger patch - http://prototype.lighthouseapp.c...
We all know how ofter
wrap/methodizeare used internally, so I think it's important that we change those methods.bindAsEventListenerandcurryneed this to a lesser extent, but would bring some consistency (with no downsides, but code size)
Please Login or create a free account to add a new comment.
You can update this ticket by sending an email to from your email client. (help)
Create your profile
Help contribute to this project by taking a few moments to create your personal profile. Create your profile »
The Prototype JavaScript library.
Andrew Dupont