Buggy Array#map, Array#every, and friends don't ensure the `list` param is an object #197

Closed
jdalton opened this Issue Jan 4, 2014 · 15 comments

Comments

Projects
None yet
2 participants
Contributor

jdalton commented Jan 4, 2014

Related to #182 it applies to more than just Array#forEach.

Owner

ljharb commented Jan 4, 2014

Good point - let's ensure that check is done (In a DRY fashion) for each of the Array# methods that take a context param

@ghost ghost assigned ljharb Jan 4, 2014

Owner

ljharb commented Jan 4, 2014

Where are you testing this? In node 0.6.21, I'm seeing only Array#map also having this bug - every/some/filter/reduce all seem to work properly.

Contributor

jdalton commented Jan 4, 2014

I tested in Node 0.6.8 (I used nave):

Array.prototype.every.call('hi', function(v, i, o) { console.log(v, i, typeof o); });
// logs => h, 0, string
Contributor

jdalton commented Jan 4, 2014

It's probably a good idea to check any method that passes the object to its callback.

Owner

ljharb commented Jan 4, 2014

Gotcha - pretty much the only version of node that anyone ever supports is the latest minor - ie, 0.6.21 is all that matters in 0.6.x, and anything 0.6.x before that should be immediately gotten rid of. However, I have a PR ready that will cover them anyways.

Contributor

jdalton commented Jan 4, 2014

fwiw Node 0.6.21 (installed it via nave too) also console logs the incorrect value:

Array.prototype.every.call('hi', function(v, i, o) { console.log(v, i, typeof o); });
// logs => h, 0, string
Owner

ljharb commented Jan 4, 2014

huh - let me recheck my tests in #199, they might not be correct.

Owner

ljharb commented Jan 4, 2014

Ah - looks like the es5 shim already handles this case for everything but filter and map in node 0.6. My tests will still check for it, just in case.

Contributor

jdalton commented Jan 4, 2014

Ah - looks like the es5 shim already handles this case

After your patch yap, before though methods like Array#every didn't.

Owner

ljharb commented Jan 5, 2014

Incorrect - with my tests and without my fix, on node 0.6.8 and 0.6.21, the tests passed for everything but filter and map.

On master, i run node, then require('es5-shim'), then Array.prototype.every.call('hi', function(v, i, o) { console.log(v, i, typeof o); }); and you can clearly see that the logged type is "object". If you don't explicitly require es5-shim, then you're testing stock node, not the level the shim has brought it to.

@ljharb ljharb closed this in #199 Jan 5, 2014

ljharb added a commit that referenced this issue Jan 5, 2014

Merge pull request #199 from ljharb/check_boxing_in_other_array_methods
Check object boxing of list argument in other array methods. Fixes #197
Contributor

jdalton commented Jan 5, 2014

Here's a screencast of what I'm seeing:
http://f.cl.ly/items/1z2i1d3N362R3B2B2N1C/array_methods.mp4

Owner

ljharb commented Jan 5, 2014

wow, ok, i did manage to reproduce that - however on the latest master including that PR, it works. try that again?

Contributor

jdalton commented Jan 5, 2014

wow, ok, i did manage to reproduce that

\o/

however on the latest master including that PR, it works. try that again?

I'd hope the PR did fix it.

Owner

ljharb commented Jan 5, 2014

:-D Thanks for your diligence and persistence!

Contributor

jdalton commented Jan 5, 2014

Thanks for jumping in. You produce patches faster than I've seen devs do in a while. It makes my head spin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment