jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.33k stars 5.53k forks source link

inconsistency between `_.indexOf` with and without `isSorted` flag. #1078

Closed jdalton closed 11 years ago

jdalton commented 11 years ago

I know Underscore's stance on sparse arrays is to ignore the issue and remain inconsistent in different environments. However, I found a case where it's inconsistent in the same environment.

_.indexOf(Array(3), undefined); // -1 in modern enviros
_.indexOf(Array(3), undefined, true); // 0 in modern enviros
jashkenas commented 11 years ago

Do you have a suggested change to make for this as well?

jdalton commented 11 years ago

The inconsistency is caused by _.indexOf using native Array#indexOf and _.sortedIndex using a simple while-loop. There are other places like _.isEqual as well, so probably a pain to solve w/o changing Underscore's policy on consistency. This is more a heads up as some may have assumed the inconsistency was limited to older environments (I was surprised, at least, to find it in newer).

jashkenas commented 11 years ago

Cool then -- thanks for the heads up. As long as you aren't searching for undefined in sparse arrays, no worries.

jdalton commented 11 years ago

The inconsistency (be it sparse arrays, objects,or arguments object iteration) is kind of silly though. I'd love to hear the justification for why having different behavior in some environments or even within the same environment is worth it (the cost to devs debugging older environments / non-obvious issues in newer enviros).

jashkenas commented 11 years ago

Of course inconsistencies are undesirable -- although in cases like these (sparse arrays) where real-world uses of the library won't encounter them, they're sometimes acceptable, barely.

The reason is the same as always -- we want to use the native functions where possible, and use the straightforward implementation (the one that you would have written or used otherwise) where the native functions aren't available. That's the original idea for the baseline version of Underscore.

jdalton commented 11 years ago

Of course inconsistencies are undesirable -- although in cases like these (sparse arrays) where real-world uses of the library won't encounter them, they're sometimes acceptable, barely.

The problem is Underscore has a growing list of punted issues (ignoring them doesn't do devs any favors) where devs have run into inconsistencies in real-world use, esp when testing older enviros. I could understand if you said, it's worth it because Underscore gains performance or simplicity, but that's not the case.

The reason is the same as always -- we want to use the native functions where possible, and use the straightforward implementation (the one that you would have written or used otherwise) where the native functions aren't available. That's the original idea for the baseline version of Underscore.

Being stubborn at the expense of Underscore's users, based on 4 year old incorrect assumptions, seems off. Using simple loops wins consistency, performance, portability, and simplicity. I dig this quote from Alex Russell recent blog post:

the highest good you can do as an engineer is to make your fellow engineers more productive.

Using native iteration methods for the sake of using native methods, with no clear benefit and a real costs to devs, runs directly opposite to that.

jashkenas commented 11 years ago

It certainly is a damn shame that the basic native iteration / filtering / mapping array extras were never optimized in browsers -- but instead of standing on your usual soapbox ... feel free to be productive instead. Pass along a pull request that removes delegation to slow natives, and we can merge it on in.

jdalton commented 11 years ago

It certainly is a damn shame that the basic native iteration / filtering / mapping array extras were never optimized in browsers

Avoiding native iteration methods wins more than just performance, as stated above there's also gains in consistency, portability, and simplicity.

but instead of standing on your usual soapbox ... feel free to be productive instead.

I have been productive. I've led by example, spending the past year proving that it is possible to go this route.

Pass along a pull request that removes delegation to slow natives, and we can merge it on in.

This sounds like a perfect job for @knowtheory.