Bug Tracker

Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#9750 closed bug (duplicate)

jQuery.map( arguments, fn ) can fail in Safari

Reported by: spicyj Owned by:
Priority: low Milestone: 1.next
Component: core Version: 1.6.2
Keywords: Cc:
Blocked by: Blocking:

Description

The "isArray" heuristic looks at elems[ 0 ] and elems[ elems.length - 1 ]; if they are both truthy; it assumes that elems is an array. In the case that one of the two elements is falsey, map will treat it as an object instead of an array.

Normally, that's pretty much okay, but in Safari, for ( key in arguments ) seems to yield no values. This page demonstrates the bug:

http://jsfiddle.net/qeSxC/

By testing the two end values for undefined instead of truthiness, everything is fixed. Patch and a test on GitHub:

https://github.com/spicyj/jquery/commit/204844c8d7acd97d20b3add013156b1a2cc58c3c

Change History (8)

comment:1 Changed 8 years ago by spicyj

comment:2 Changed 8 years ago by Rick Waldron

Component: unfiledcore
Priority: undecidedlow
Resolution: wontfix
Status: newclosed

Without arguing the semantics of jQuery.map's first argument, you're using it like "each" which is incorrect - jQuery.map should not be used for side-effects. jQuery.map is expected to return a mutated copy of the original array or object argument. I've reduced the test case further to show the correct usage: http://jsfiddle.net/rwaldron/FU7Fv/

comment:3 Changed 8 years ago by spicyj

My bug stands. Screenshot from your fiddle:

http://uploads.hipchat.com/6574/26709/u3mhjt5gjugeigk/upload.png

(In my original script, I was using jQuery.map properly but changed it to run console.log for that demo.)

comment:4 Changed 8 years ago by spicyj

If you prefer to relying on jQuery.map's each-like functionality, you can use this patch instead which uses map "properly" in the test case:

https://github.com/spicyj/jquery/commit/81e0e8cd42da0dc93d2d69cd8bed6915105d1d0b

comment:5 Changed 8 years ago by Rick Waldron

Resolution: wontfix
Status: closedreopened

Reopening for further discussion

comment:6 Changed 8 years ago by Robert Katić

This would be a duplicate of #9025 (that is wrongly declared as a duplicate) that already has a patch.

comment:7 Changed 8 years ago by addyosmani

Resolution: duplicate
Status: reopenedclosed

comment:8 Changed 8 years ago by addyosmani

Duplicate of #9025.

Note: See TracTickets for help on using tickets.