Question about a change in the newer version of AddEvent

I've been using AddEvent for a couple of years now - the version that won that contest a while ago - and I'm very impressed with it. I recently checked out the site again and saw there was a new version, and I wanted to query one of the changes in it.

Specifically, in the first version of addEvent you constructed an event-handler wrapper function using the "function(e) { ...body of function... }" syntax. In the new version, you construct it using "new Function('e', '...body of function...')". What was the reason for the switch?

According to my reading (http://developer.mozilla.org/en/docs/Core_JavaScript_1.5_Reference:Global_Objec ts:Function#General), a function created using the Function() constructor is slower (at least in Gecko) because the function has to be interpreted every time it is called, whereas a function defined by "function() {}" is compiled at parse time.

Also, since t is the only variable external to this newly-created function, couldn't memory be saved by defining a function outside of addEvent which takes t as a second parameter, rather than defining this function as a closure every time that addEvent is called?

Yet another question, what's the rationale for duplicating the addEvent arguments with rO = o, rT = t, rF = f, rL = l for the sake of the unload handler? Is this a way of avoiding perpetuating the memory leak?

Could I get some independent confirmation that the pruned version of the function above does not introduce memory leaks?

Hi there!
I shifted the bulk of the Function() code to a regular function declaration because (at least) Gecko parses functions created with Function() every time they are called. I figure the less code it has to parse each time, the faster it runs; this caveat may not apply to IE anyway though, and would need rigorous timing tests to verify that it makes any difference.

As far as the variable duplication goes - these look to be extraneous, and probably increase memory usage since a closure stores all variables from the scope it was created in.

You mention you retained the unload handler because there were still leaks on large pages due to leftover properties. However, in the original function the unload cleanup is only applied to the first event handler, not to any further ones - so these were still being left behind when the page unloads (as was the o._evt container).

I put together a reduced testcase for memory leaks, which just added 500 unique closures as onclick handlers on the document. There were no perceptible memory leaks nor differences in memory behaviour between the original function and the one I posted above, with or without the unload handler in both cases.

I also tested with variants which had the Function() call replaced with a regular closure: these leaked obviously and consistently, with or without the unload handler. However if the unload handler was moved so that it applied to each event handler, not just the first one, then the leaks were much smaller.

I agree with you about extensions to the Object prototype - they make the otherwise useful (for...in) loop structure awkward and unreliable. However, I feel it's better to implement a sanity-check and curse the programmers who made it necessary, than to ignore it and allow unexpected behaviour that's difficult to debug (and which would require the programmer to implement the same solution anyway). Plus the check I added is smaller and probably faster than a hasOwnProperty() call.

Thus, the revised version:

var addEvent = function(o, t, f, l)
{
var d = 'addEventListener';
if (o[d]  && !l) return o[d](t, f, false);
if (!o._evts) o._evts = {};
if (!o._evts[t])
{
var n = 'on' + t;
o._evts[t]  = o[n] ? { b: o[n] } : {};
o[n] = new  Function('e', 'return addEvent._c(e||window.event, this, this._evts["' + t + '"])');
}
if (!f._i) f._i = addEvent._i++;
o._evts[t][f._i] = f;
if (t != 'unload')  addEvent(window, 'unload', function() {removeEvent(o, t, f, l)});
}

addEvent._i  = 1;
addEvent._c = function(e, o, a)
{
var r = true, i;
for (i in a) { if (a[i]._i) {o._f  = a[i]; r = o._f(e) != false &&  r; o._f = null;} }
return  r;
}

Good point about o._f - I'd suggest using delete on it instead, except that delete fails on certain cases in IE... I think because it cannot delete properties from the window object? (presumably because window is also the global object). Does this tally with your observations?

Not leaks, just leaving behind unused properties. But since one doesn't iterate the properties of a DOM node anyway (unless one likes trawling thousands of properties) then I suppose it makes no practical difference.

(huh, and I only just noticed that you're a fellow NZer!)

In fact why not just assign o[n] a regular incremented _i value, that way you can do:

var old = document.onclick;
addEvent(document, 'click',  stuff);
// ...other code...
removeEvent(document, 'click', old);

And hey, always good to meet another coder from Way Down Under :).

-  Angus.