Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

indexOf utility and Array comprehensions need tweaking for sparse arrays and fallbacks. #1771

Closed
jdalton opened this Issue · 7 comments

3 participants

@jdalton

The indexOf utility fallback doesn't follow spec enough in regards to sparse arrays.
Instead of hasOwnProperty the in operator should be used.

See this and this.

Also Array comprehensions should be mod'ed to fit with this change and to align with [JavaScript 1.7](https://developer.mozilla.org/en/New_in_JavaScript_1.7#Array_comprehensions_(Merge_into_Array_comprehensions\)) and ES.Next proposals.

@michaelficarra
Collaborator

Agreed. @satyr's implementation looks good to me, but with one concern: I'm wondering if we should use the Strict Equality Comparison Algorithm like indexOf or an egal comparison. Since it's an operator of our own design, I think we have the freedom to use egal, so I'd prefer that. NaN in [NaN] producing false and -0 in [0] producing true is a little unexpected.

@michaelficarra
Collaborator

Input from @jashkenas is needed here.

@jashkenas
Owner

I think @satyr's implementation looks good. I think I would expect triple equals semantics ... as nothing else in the language has egal semantics, and you'd expect in to work analogously to is.

@michaelficarra michaelficarra closed this issue from a commit
@michaelficarra michaelficarra fixes #1771: indexOf utility and comprehensions should use `in` seman…
…tics

not hasOwnProperty tests as in the indexOf utility or no test as in the
comprehensions
44bf8c2
@jashkenas jashkenas reopened this
@jashkenas
Owner

Support for sparse arrays in IE isn't worth the tradeoff of adding an extra in -- CoffeeScript comprehensions are supposed to compile to vanilla for loops. Closing as a wontfix.

@jashkenas jashkenas closed this
@jdalton

By not having the indexOf utility follow spec you have inconsistent behavior for IE < 9 (and any other browser without native indexOf) vs browsers with native Array#indexOf.

The array comprehension issue isn't an IE issue.
It's to align with ES.Next and existing JS comprehension implementations.

@jdalton

The cost in the indexOf utility is probably a wash as you would be removing the incorrect hasOwnProperty check and replacing it with the in check.

@jashkenas
Owner

Yes -- the in change is fine, and has been committed (although I don't see it listed here yet), but adding an in to every iteration through a comprehension -- which is what the pull request was doing, isn't a change we want to make.

@TrevorBurnham TrevorBurnham referenced this issue from a commit in TrevorBurnham/coffee-script
@jashkenas Fixes #1771: Fixing the indexOf shim. 846306f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.