Is setting element.id as a side effect bad practice?

Question

I'm writing a little cached function in a plugin / library. It takes a HTMLElement and returns a Decorator.

return function _cache(elem) {
    if (elem.id === "") {
        elem.id = PLUGIN_NAME + "_" + uid++;
    }
    if (cache[elem.id] === void 0) {
        cache[elem.id] = _factory(elem);
    }
    return cache[elem.id];
}

Here I'm storing some expensive operation in a cache by the id of the HTMLElement. This is a O(1) lookup but it uses the "bad practice" of setting elem.id and having a side effect.

The alternative would be O(N) lookup on the cache

return function _cache(elem) {
    for (var i = 0, ii = cache.length; i++) {
        var o = cache[i];
        if (o.elem == elem) return o.data;
    }
    var ret = _factory(elem);
    cache.push({ elem: elem, data: ret });
    return ret;
}

But this means that my cached expensive method doesn't have any side effects on the HTMLElement.

Question:

Is this "side effect" innocent and is it worth doing for the optimization on my decorator?

Real Code:

Gist of plugin template where I use this snippet

Edit:

I'm clearly too tired and forgot data-foo exists. Here's how it should be implemented

var attr = "data-" + PLUGIN_NAME + "-cache";

return function _cache(elem) {
    var val = elem.getAttribute(attr);
    if (val === null || val === "") {
        val = PLUGIN_NAME +  "_" + uid++;
        elem.setAttribute(attr, val);
    }
    if (cache[val] === undefined) {
        cache[val] = _factory(elem);
    }
    return cache[val];
}

Answer 1

Instead of using the id, use data-x - that's what it was created for.

id has a specific meaning, is confusing to see it automagically generated (even if properly documented, which is nearly never.) You're also risking a slight chance of override.

Answer 2

Is this "side effect" innocent

No, clearly. Is it going to interact well with other scripts on the page? Don't know... that depends what it's for and what other kinds of scripts you expect it to be combined with. You can never make a ‘plugin’ that won't ever fail when interacting with other plugins and scripts, but by keeping the side-effects to a minimum you can at least try to minimise it.

Note that id is not a unique identifier. Although there should be only one element with a given ID in a document at one particular time, (a) multiple elements might be created with the same ID and inserted into the document sequentially (perhaps one element replacing another with the same ID), and (b) people still do use duplicate IDs even though it's wrong. Either would cause your cache to collect old, no-longer-used elements and return them inappropriately.

It is unfortunate that there is no JavaScript function to get a scalar/hashable unique identifier for an arbitrary object; the only way to obtain object identity is to ===-compare against other objects.

Another common way forward is to add an arbitrary new property to the node (‘expando’ in IE terms), with a randomised really-unique ID. Expandos aren't guaranteed by standard to work, but it has worked in all browsers back to day one and is commonly used.

This is how for example jQuery identifies elements uniquely, and if you are writing a plugin for jQuery you might try taking advantage of that—jQuery.expando holds the name of the arbitrary expando property being used for this purpose... or, sticking within the documented featureset, data() could be used to add your own metadata to the element including another unique ID of your own.

Expandos do have some unpleasant side-effects including accidentally treating them as attributes in IE<9 (which can't tell the difference between properties and attribute), but if you're using jQuery anyway you probably don't have anything to lose.

is it worth doing for the optimization on my decorator?

Depends how many you're expecting to have on a page. Comparing each item to each other item is an O(n²) operation; tolerable (and probably preferable given the side-effects) if n is low, but quickly getting unmanageable as n grows.