justmoon / node-extend

Simple function to extend objects
MIT License
341 stars 68 forks source link

Arrays are not replaced on deep copies #19

Closed inator closed 10 years ago

inator commented 10 years ago

I imagine that this is inherited from the jQuery implementation of extend, but I'm experiencing the exact same behavior described here:

http://forum.jquery.com/topic/jquery-extend-modifies-but-not-replaces-array-properties-on-deep-copy

In my opinion, the inclusion of array extending may be more problematic then helpful. Would you consider a deviation on this from what the jQuery folks chose to do?

EDIT: Been giving this some thought, and perhaps consistency is more important in a "port." I can certainly live with the current approach knowing that the overall behavior matches up with jQuery. Still, I'd be interested in your thoughts.

ljharb commented 10 years ago

Arrays are objects too, and although I can certainly see the usefulness of replacing them, I think that "deep extend" should apply to all objects. In addition, I think consistency with jQuery is important here.

Either way, $.extend/_.extend, and all of their brethren, including this module, will soon be obsolete for shallow copies, now that Object.assign is in ES6.

I can certainly see the value of an Object.assign wrapper that both deeply copies objects, and replaces arrays rather than merges them - but I think it should be a separate module, not configured through a flag (the deep param imo was a mistake - it should have been $.extend and $.deepExtend from the start)

inator commented 10 years ago

Indeed, we shouldn't discriminate. Arrays are objects too! I don't know if you saw my edit but I arrived at the same conclusion shortly after posting the issue. I almost closed it but elected not to so that I could get your thoughts. Thanks!

On Jul 2, 2014, at 6:49 PM, Jordan Harband notifications@github.com wrote:

Arrays are objects too, and although I can certainly see the usefulness of replacing them, I think that "deep extend" should apply to all objects. In addition, I think consistency with jQuery is important here.

Either way, $.extend/_.extend, and all of their brethren, including this module, will soon be obsolete for shallow copies, now that Object.assign is in ES6.

I can certainly see the value of an Object.assign wrapper that both deeply copies objects, and replaces arrays rather than merges them - but I think it should be a separate module, not configured through a flag (the deep param imo was a mistake - it should have been $.extend and $.deepExtend from the start)

— Reply to this email directly or view it on GitHub.

inator commented 10 years ago

BTW - I agree with the $.extend and $.deepExtend approach. That's the way it should have been.

On Jul 2, 2014, at 7:42 PM, Dale Shaw dale@xiatron.com wrote:

Indeed, we shouldn't discriminate. Arrays are objects too! I don't know if you saw my edit but I arrived at the same conclusion shortly after posting the issue. I almost closed it but elected not to so that I could get your thoughts. Thanks!

On Jul 2, 2014, at 6:49 PM, Jordan Harband notifications@github.com wrote:

Arrays are objects too, and although I can certainly see the usefulness of replacing them, I think that "deep extend" should apply to all objects. In addition, I think consistency with jQuery is important here.

Either way, $.extend/_.extend, and all of their brethren, including this module, will soon be obsolete for shallow copies, now that Object.assign is in ES6.

I can certainly see the value of an Object.assign wrapper that both deeply copies objects, and replaces arrays rather than merges them - but I think it should be a separate module, not configured through a flag (the deep param imo was a mistake - it should have been $.extend and $.deepExtend from the start)

— Reply to this email directly or view it on GitHub.

ljharb commented 10 years ago

Thanks for the input! Ideas are always welcome :-)