Bug Tracker

Modify

Ticket #2616 (closed enhancement: fixed)

Opened 6 years ago

Last modified 2 years ago

A better jQuery.map

Reported by: flesler Owned by: danheberden
Priority: high Milestone: 1.6
Component: core Version: 1.5
Keywords: Cc:
Blocking: Blocked by:

Description

Hi, today I realized jQuery.map doesn't support hashes(associative arrays). I'm surprised, this is a behavior I'd assume it'd be included.

I added the changes to make this possible. In addition, added a third, optional, argument that is the return object. So it's possible to map on a existing object.

I made it so flexible, that one can map from an array to a hash, and the other way around as well. This is mostly useful to collect the keys or values from a hash, or to map an array of elements to an object(like setArray). It will handle array-like objects, and will treat them as arrays

I optimized the array concatenation, so now it's only done once, in the end, and now, there's no need for array-wrapping the returned data.

I think the length of the code remains more or less the same, so this should be doable.

I tested all the cases in IE6, FF2, Safari 3, and Opera 9.

I hope this helps

Attachments

map.diff Download (1.3 KB) - added by flesler 6 years ago.
Proposal
map-tests.diff Download (1.1 KB) - added by flesler 6 years ago.
The tests for the test suite
map[5753].diff Download (1.1 KB) - added by flesler 6 years ago.
Updated to match against [5753]

Change History

Changed 6 years ago by flesler

Proposal

comment:1 Changed 6 years ago by flesler

Tried all these cases in IE6, FF2, Opera 9.25 and Safari 3 and they all worked well.

var keys = $.map( {a:1,b:2}, function( v, k ){
	return k;
}, [ ] );
alert(keys);

var values = $.map( {a:1,b:2}, function( v, k ){
	return v;
}, [ ] );
alert(values);

var mapped = $.map( document.getElementsByTagName('script'), function( v, k ){
	return v;
}, {length:0} );
alert(mapped);

var flat = $.map( Array(8), function( v, k ){
	return k % 2 ? k : [k,k,k];//try mixing array and regular returns
});
alert(flat);

comment:2 Changed 6 years ago by joern

Could you convert that to the testsuite, too?

Changed 6 years ago by flesler

The tests for the test suite

comment:3 Changed 6 years ago by joern

  • need changed from Review to Commit

Commited test in [5342].

comment:4 Changed 6 years ago by joern

Your patch must be tested against the benchmark before applying it, as map is a core function of jQuery. Its raw speed has higher priority then being useful for other cases. If we can do both, even better.

comment:5 Changed 6 years ago by flesler

Shouldn't you/we accept the change before adding the tests to the test suite ? Or it will fail for others that haven't applied the proposed diff.

comment:6 Changed 6 years ago by flesler

I meant you/we add the test, not accept. I didn't mean to say it's on me to accept :)

Changed 6 years ago by flesler

Updated to match against [5753]

comment:18 Changed 4 years ago by SlexAxton

  • Owner set to flesler
  • Priority changed from major to low
  • Status changed from new to pending
  • Milestone changed from 1.2.4 to 1.5

Is this still something you guys think should make it into core? It's been a little while...

comment:19 Changed 3 years ago by trac-o-bot

  • Status changed from pending to closed

Automatically closed due to 14 days of inactivity.

comment:20 Changed 3 years ago by jordan@…

I think the performance his is too much as per joern's comment:  http://jsfiddle.net/jboesch26/mmhwZ/4/

comment:21 Changed 3 years ago by jordan@…

I've made some changes to make sure this doesn't affect performance that much:  https://github.com/jquery/jquery/pull/238

comment:22 Changed 3 years ago by rwaldron

#8328 is a duplicate of this ticket.

comment:23 Changed 3 years ago by rwaldron

I've spent some time with the newest patch and perf comparing it to the existing, I'm indifferent about the perf. My issue is this...

// Provided test...

var keys = jQuery.map( {a:1,b:2}, function( v, k ){
	return k;
});

equals( keys.join(""), "ab", "Map the keys from a hash to an array" );


// But this is totally bonkers:
var keys = jQuery.map( {a:1,b:2, length: 2 }, function( v, k ){
	return k;
});

equals( keys.join(""), "ab", "Map the keys from a hash to an array, has a length property" );

// > "01" 
// expected: "ab"

comment:24 Changed 3 years ago by rwaldron

  • Status changed from closed to reopened
  • Milestone 1.5 deleted

comment:25 Changed 3 years ago by rwaldron

  • Keywords needsreview added; array map removed
  • Version changed from 1.2.3 to 1.5
  • Milestone set to 1.next

Reopening this for the current review

comment:26 Changed 3 years ago by jboesch

Updated pull request can be found here:  https://github.com/jquery/jquery/pull/252

comment:27 Changed 3 years ago by rwaldron

  • Status changed from reopened to pending

Did you look at the weird test results I posted previously?

comment:28 Changed 3 years ago by trac-o-bot

  • Status changed from pending to closed
  • Resolution set to invalid

Because we get so many tickets, we often need to return them to the initial reporter for more information. If that person does not reply within 14 days, the ticket will automatically be closed, and that has happened in this case. If you still are interested in pursuing this issue, feel free to add a comment with the requested information and we will be happy to reopen the ticket if it is still valid. Thanks!

comment:29 Changed 3 years ago by rwaldron

  • Status changed from closed to reopened
  • Resolution invalid deleted

comment:30 Changed 3 years ago by rwaldron

  • Keywords needsreview removed
  • Priority changed from low to high
  • Milestone changed from 1.next to 1.6

comment:31 Changed 3 years ago by danheberden

  • Owner changed from flesler to danheberden
  • Status changed from reopened to assigned

comment:32 Changed 3 years ago by john

  • Status changed from assigned to closed
  • Resolution set to fixed

Landed.

comment:33 Changed 3 years ago by yk

Why map to the object creates an array? I do not quite understand the history of this ticket, but the original patch was different behavior (map object to object) and I think it's more logical.

comment:34 Changed 3 years ago by danheberden

For an end use standpoint, you're right - but internally, worrying about something else being returned from map besides an array is out of scope for the patch. Even then, the patch isn't designed to be full featured by any means - just a simple way to parse out keys or values into an array.

That's not to say someone can't make a plugin to offer the object to object map - not sure how that'd look recursively though..

Please follow the  bug reporting guidlines and use  jsFiddle when providing test cases and demonstrations instead of pasting the code in the ticket.

View

Add a comment

Modify Ticket

Action
as closed
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.