feat(extend): optionally deep-extend when last parameter is true
#10519
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
| @@ -333,7 +333,7 @@ function setHashKey(obj, h) { | |||
| * Extends the destination object `dst` by copying own enumerable properties from the `src` object(s) | |||
| * to `dst`. You can specify multiple `src` objects. If you want to preserve original objects, you can do so | |||
| * by passing an empty object as the target: `var object = angular.extend({}, object1, object2)`. | |||
| * Note: Keep in mind that `angular.extend` does not support recursive merge (deep copy). | |||
| * Note: Pass `true` in as the last argument to perform a recursive merge (deep copy). | |||
There was a problem hiding this comment.
We ought to have a @param tag for this but I am not sure how that fits with the variable argument src.
There was a problem hiding this comment.
yeah, it's not super clear how that would work... Maybe you could hack dgeni to process arguments that end with ... (or begin with ...) as a spread of parameters or something
There was a problem hiding this comment.
Bloody autocorrect renames "dgeni" to "degree" -.-
|
there is something odd in having an optional parameter after a variable number of parameters... why not put this optional parameter second? |
| }); | ||
|
|
||
|
|
||
| it('should create an empty array if destination property is a non-object and source property is an array when deep-extending', function() { |
There was a problem hiding this comment.
'should create a new array ...'?
|
Apart from the minor docs/comments notes I made this looks good to me. |
It would be lovely to do it that way, but it would be a breaking change... This is what I didn't like about the PR in the first place, but it doesn't seem "that bad" really |
|
Why would it be a breaking change?
|
| } | ||
| } | ||
| } | ||
|
|
||
| setHashKey(dst, h); | ||
| if (isDeep !== 0xFACECAFE) setHashKey(dst, h); |
There was a problem hiding this comment.
We should probably still attach a hashkey if h is defined.
There was a problem hiding this comment.
yeah, I think that makes sense, will get rid of FACECAFE =)
There was a problem hiding this comment.
Ich bin bis 05.01.2015 abwesend.
Ich werde vertreten durch Herrn T. Landgraf und Herrn D. Neander.
Hinweis: Dies ist eine automatische Antwort auf Ihre Nachricht "Re:
[angular.js] feat(extend): optionally deep-extend when last parameter is
true (#10519)" gesendet am 20.12.2014 03:58:31.
Diese ist die einzige Benachrichtigung, die Sie empfangen werden, während
diese Person abwesend ist.=
|
I rather have |
angular.merge() is like angular.extend(), but will "deeply" copy object properties from source objects.
|
@gdi2290 I've implemented your suggestion --- It's slightly more code but it doesn't really make a big difference, so I'm fine with either one or neither |
Previously, hashKeys were copied from source objects and subsequently deleted or replaced with the original. Now, there is no need to delete properties (which can be expensive), and no need to track the initial hashKey value.
|
@caitp thanks, in the merge tests you should probably remove the third argument merge(dst, src, true); |
|
heh missed a few --- but they won't matter anyway, it's good to have them even to verify it won't throw on primitives :> |
pkozlowski-opensource
modified the milestones:
1.4.0-beta.1 / 1.3.10,
1.4.0-beta.2 / 1.3.11
petebacondarwin
modified the milestones:
1.4.0-beta.3 / 1.3.12,
1.4.0-beta.4 / 1.3.13,
1.4.0-beta.5 / 1.3.14
|
I'll tweak the tests and land this today. |
…merges 'deeply' Closes angular#10507 Closes angular#10519
…merges 'deeply' Closes angular#10507 Closes angular#10519
Same as #10507 but with my review comments addressed and jscs fixed.
Someone quickly review this so we can land it :<