crossfilter / reductio

Reductio: Crossfilter grouping
Other
250 stars 42 forks source link

The last alias function is clobbering all the previous alias functions #27

Closed imacneill closed 8 years ago

imacneill commented 8 years ago

Hi, Thanks for putting this library together. While using it, I think that I've found a bug in the way the alias functions are mapped.

Unfortunately, I'm pretty new to javascript and not a software engineer (though I am a scientist), so I hope I can describe this adequately.

Let's say I set up an alias: reducer.alias({a : function() {return "a";}, //for discussion call this function a {b : function() {return "b";}); //for discussion call this function b

Later when I use the reducer, I have some object: Object a : function() // unfortunately, this is function b b : function() // function b as it is supposed to be

But the problem is the pipes got screwed up and now 'a' calls function b (instead of function a), and 'b' calls function b.

If I reorder my alias, then both 'a' and 'b' call function a. e.g. reducer.alias({b : function() {return "b";}, {a : function() {return "a";} //for discussion call this function a ); //for discussion call this function b

I (with the help of a coworker) managed to track the issue to the reductio_alias variable assignment (https://github.com/esjewett/reductio/blob/master/reductio.js#L339-L349). It is due to the scoping of the prop variable inside the for loop: var reductio_alias = { initial: function(prior, path, obj) { return function (p) { if(prior) p = prior(p); for(var prop in obj) { path(p)[prop] = function() { return objprop; }; // prop inside this function is // not the same as outside the // function at the time the // function gets call } return p; }; } };

In case the comment above is too cryptic, 'path(p)[prop]=' is evaluated correctly, because prop is defined and changing within the scope of the for loop. This is why we get an object in the end with all of the proper keys. However, 'objprop' inside the function doesn't get evaluated until the function is called later on. At this point in all cases, because we are out of the scope of the for loop, prop is equal to the last value that it held when the for loop ended. Thus all keys will call the same function (or something like that...)

Anyway, I solved the issue by wrapping the anonymous function in a named function that takes the prop as an argument. It all seems to work now.

var reductio_alias = { initial: function(prior, path, obj) { return function (p) { if(prior) p = prior(p); function buildAliasFunction(key){ return function(){ return objkey; }; } for(var prop in obj) { path(p)[prop] = buildAliasFunction(prop); } return p; }; } };

I hope this helps, even if you choose to solve this a different way.

I am also opening a pull request for your convenience in case you choose this solution.

imacneill commented 8 years ago

Oh man... I should have previewed. It completely ruined the formatting. Sorry that it's harder to read. This is the first time I opened an issue on github.

esjewett commented 8 years ago

Oh, well, that's not good! Thanks for finding this! I'll comment further on the pull request.

esjewett commented 8 years ago

Sorry for the delay. Published 0.5.3 incorporating this fix.