statgen / locuszoom

A Javascript/d3 embeddable plugin for interactively visualizing statistical genetic data from customizable sources.
https://statgen.github.io/locuszoom/
MIT License
156 stars 29 forks source link

Adding `onUpdate` function causes `undefined` to be added `onUpdateFunctions` #64

Closed mssand-b closed 8 years ago

mssand-b commented 8 years ago

Observed this behavior today: I created a function f() {} (literally did nothing), then called locusZoomPlot.onUpdate(f). That worked fine; however, the second call to onUpdate() (without argument, so therefore intending to run all of the stored functions) would yield a Uncaught TypeError: this.onUpdateFunctions[func] is not a function.

I replaced the onUpdate function with the following:

LocusZoom.Instance.prototype.onUpdate = function(func){
    debugger;
    console.log('this called');
    console.log('func is', func, typeof func == 'undefined'); 
    console.log('currently', this.onUpdateFunctions);
    if (typeof func == "undefined" && this.onUpdateFunctions.length){
        console.log('these conditions met');
        for (func in this.onUpdateFunctions){
            console.log('func iterator', func);
            this.onUpdateFunctions[func]();
            console.log('in iterator', this.onUpdateFunctions);
        }
    } else if (typeof func == "function") {
        this.onUpdateFunctions.push(func);
    }
    console.log('now', this.onUpdateFunctions);
};

Using this, I could see that f got added correctly. However, when onUpdate() was called the first time, this.onUpdateFunctions became [function function, undefined: undefined]. Further inspection showed that when onUpdateFunctions was called the first time without argument, it would iterate over the following values: 0, shuffle, swap, heapSort. 0 makes sense, as an array indice. The last 3 are surprising--this.onUpdateFunctions["shuffle"]() and this.onUpdateFunctions["heapSort]()" don't seem to do anything, but this.onUpdateFunctions["swap"]() seems to be adding the undefined to onUpdateFunctions. Not entirely sure where these are coming from--I assume it has something to do with the prototype methods for arrays in javascript, but some quick research didn't seem to show anything.

The fix seems to be swapping out the iteration like so:

this.onUpdateFunctions.forEach(function(funcToRun) {
     funcToRun();
});

I'll make up a PR here in a minute.

tl;dr: the way JS handles iteration over an array doesn't always seem to work correctly, which results in undefined getting added to onUpdateFunctions

Frencil commented 8 years ago

Addressed with ebb0858.