Skip to content

Conversation

onovy
Copy link

@onovy onovy commented Apr 10, 2014

Function merge now allows only 2 arrays for merging. This patch allow to have more than 2 arrays for merging, same as function extend allows more then 2 plain objects for merge.

http://bugs.jquery.com/ticket/14982

firstLen = first.length;

for ( ; i < arguments.length ; i++ ) {
// Only deal with non-null/undefined values
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before als non-null/undefined values were copied

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

non-null params?
$.merge([1, 2], null);

null as values are still copied:
$.merge([1, 2], [null]);
-> [1,2,null]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh right... your are skipping the arguments passed to the function not the values of the array...

@staabm
Copy link
Contributor

staabm commented Apr 10, 2014

Have you already opened a jquery ticket on the bugtracker for this request? Usually they decide which feature will be integrated their before actually implementing it.

@onovy
Copy link
Author

onovy commented Apr 10, 2014

no, implementation was simple so i decided to implement it a try pull request it.

should i open ticked for it now?

@staabm
Copy link
Contributor

staabm commented Apr 10, 2014

yes, ticket is a must have.

@onovy
Copy link
Author

onovy commented Apr 10, 2014

@dmethvin
Copy link
Member

@onovy What is the use case for this? In general our utility functions should be kept as simple as possible and address the internal needs of the jQuery library. More complex needs can be addressed through a plugin. Would adding this feature allow us to simplify jQuery itself? If so can you identify the places where that would happen?

@onovy
Copy link
Author

onovy commented Apr 10, 2014

I think utility from jQuery is not only for jQuery core, but for jQuery users too. New pattern simplifies usage, because it's same as extend, which many users expects. Of course it's usable in jQuery Core.

@dmethvin
Copy link
Member

We don't generally add features prospectively, without a use case that shows some benefit for more users or for our internal needs. That it is theoretically usable in core is not the same as showing places where we could benefit from multi-array merge().

Most users with a real Array would probably be better off with Array#concat() from a performance standpoint I'd think, unless there are a high number of merges going on and garbage collection comes into play.

@onovy
Copy link
Author

onovy commented Apr 10, 2014

general use case is same as .extend, where is difference?

if most users uses Array#concat, why there is .merge in jQuery? Because it alters first array and don't create new array. Exactly same as .extend, but for Arrays and not Plain Objects.

@dmethvin
Copy link
Member

I don't see any significant benefit to adding this, but am still open to doing so if it could be used to good advantage in jQuery's code base.

@dmethvin
Copy link
Member

Closed, plugin.

@dmethvin dmethvin closed this Apr 14, 2014
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants