feat(angular.extend) - Simple non-breaking change to support deep extend #10507

Closed
wants to merge 4 commits into
from

Projects

None yet

7 participants

@prettycode

People use angular.extend() as a utility function, whether or not it's designed for that purpose. It can easily be extended to support recursive merging with very very minor additions. Developers coming to Angular expect extend() to support this.

@googlebot

CLAs look good, thanks!

@googlebot googlebot added the cla: yes label Dec 17, 2014
@prettycode

Is there a way to rerun the job that failed? It's a timeout, not an explicit (test) failure.

@lgalfaso
Member
git commit --amend --date "`date -R`"
git push -f
@lgalfaso
Member

also, please squash all the commits into one

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
*
* @param {Object} dst Destination object.
* @param {...Object} src Source object(s).
* @returns {Object} Reference to `dst`.
*/
function extend(dst) {
- var h = dst.$$hashKey;
+ var h = dst.$$hashKey,
+ argsLength = arguments.length,
+ isDeep = (argsLength >= 3) && (arguments[argsLength - 1] === true);
@caitp
caitp Dec 17, 2014 Contributor

nit: each of these declarations should have its own var for clarity

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
- for (var i = 1, ii = arguments.length; i < ii; i++) {
+ if (isDeep) {
+ delete arguments[argsLength - 1];
@caitp
caitp Dec 17, 2014 Contributor

declare ii before the loop, decrement it if isDeep

@caitp
caitp Dec 17, 2014 Contributor

in fact you know what, argsLength is declared already, so just use that instead :)

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
- for (var i = 1, ii = arguments.length; i < ii; i++) {
+ if (isDeep) {
+ delete arguments[argsLength - 1];
+ }
+
+ for (var i = 1, ii = argsLength; i < ii; i++) {
@caitp
caitp Dec 17, 2014 Contributor

no need for ii here, i < argsLength is fine

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
- var key = keys[j];
- dst[key] = obj[key];
+ var key = keys[j],
+ src = obj[key];
+ dst[key] = isDeep ? extend(dst[key], src) : src;
@caitp
caitp Dec 17, 2014 Contributor

recursion doesn't make a lot of sense if src is a type that can't have properties

@caitp caitp commented on an outdated diff Dec 17, 2014
test/AngularSpec.js
@@ -223,6 +223,18 @@ describe('angular', function() {
// make sure we retain the old key
expect(hashKey(dst)).toEqual(h);
});
+
+ it('should perform deep extend when last argument is true', function() {
+ var src = { foo: { bar: 'foobar' }},
+ dst = { foo: { bazz: 'foobazz' }};
+ extend(dst, src, true);
@caitp
caitp Dec 17, 2014 Contributor

add a test case where dst has a primitive (non-falsy) foo property --- also a test where it has a null foo property

@caitp
caitp Dec 17, 2014 Contributor

also add some tests where properties have null and undefined values :>

@caitp
caitp Dec 17, 2014 Contributor

Please add the tests I've been asking for, and address all of the comments I've brought up --- then we can land this.

You can avoid the complicated squash in the future by just doing git commit --amend rather than adding new commits

@lgalfaso
Member

simple question, why is it that angular.extends({}, angular.copy(foo)) not good enough? Is this just the fact that you have to also call angular.copy?

@caitp
Contributor
caitp commented Dec 17, 2014

I think config = angular.extends({}, angular.copy(defaultConfig), angular.copy(config)); is a bit more verbose than it really needs to be.

I don't see a huge problem with adding this, not to say that there aren't things I dislike about it.

@caitp
Contributor
caitp commented Dec 17, 2014

Oh, I should add that config = angular.extends({}, angular.copy(defaultConfig), angular.copy(config)); is semantically different from config = angular.extends({}, defaultConfig, config, true);

@caitp
Contributor
caitp commented Dec 17, 2014

@petebacondarwin what's your opinion? Normally we just tell people to "use lodash", but it's not a very big change, I don't really see the harm in this one

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
- dst[key] = obj[key];
+ var src = obj[key];
+
+ if (isDeep && Object.keys(src).length) {
@caitp
caitp Dec 17, 2014 Contributor

If the value is null or undefined, Object.keys() will throw here... Don't worry about checking the type I guess, it's fine

@caitp caitp commented on the diff Dec 17, 2014
src/Angular.js
- for (var i = 1, ii = arguments.length; i < ii; i++) {
+ if (isDeep) {
+ delete arguments[argsLength - 1];
+ }
+
+ for (var i = 1; i < argsLength; i++) {
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
@caitp
caitp Dec 17, 2014 Contributor

That said, I really think we should only be taking this path if isObject(obj) || isFunction(obj) --- it doesn't really make sense otherwise

@caitp
caitp Dec 17, 2014 Contributor

For instance:

Object.keys(4); // will return the own-enumerable-property-keys of `new Number(4);` --- that's not very useful! It won't have any own properties.
@caitp
caitp Dec 17, 2014 Contributor

Also, in ES5 it will throw for all non-objects, which is not very useful! So WebKit/JSC would throw here anyways. Newer browsers will take the ES6 path and convert the primitive ToObject, and do the wrong stuff with it.

@prettycode

I'm afraid I may be in over my head here. Tried to squash the multiple commit (sorry) into one via this, but I'm still seeing multiple commits:

git rebase -i HEAD~8
git push -f

@caitp
Contributor
caitp commented Dec 17, 2014

@prettycode what do you see when you run git rebase -i HEAD~8? you should get an editor screen, and you'll want to replace the "pick" on the left side of the last 7 commits with "f" to squash it. Careful not to squash too many =)

@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
- for (var i = 1, ii = arguments.length; i < ii; i++) {
+ if (isDeep) {
+ delete arguments[argsLength - 1];
+ }
+
+ for (var i = 1; i < argsLength; i++) {
var obj = arguments[i];
if (obj) {
@caitp
caitp Dec 17, 2014 Contributor

You misunderstood me in that last commit --- I mean on line 353, the condition should not be if (obj), it should be if (isObject(obj) || isFunction(obj))

@caitp
caitp Dec 17, 2014 Contributor

Make this change, please.

@prettycode prettycode Simple non-breaking change to support deep extend.
9f7f80b
@prettycode

Thanks for your help, caitp! Sorry for my inexperience with git. Updating tests and the if(obj) check you referenced (the comment I misunderstood).

@petebacondarwin
Member

@caitp - Oh go on then :-)

@prettycode prettycode Check property is object before deep-extending.
c291c59
@caitp caitp commented on an outdated diff Dec 17, 2014
src/Angular.js
- for (var i = 1, ii = arguments.length; i < ii; i++) {
+ if (isDeep) {
+ delete arguments[argsLength - 1];
@caitp
caitp Dec 17, 2014 Contributor
if (isDeep) --argsLength;

We don't need to delete anything, it's slow, and deleting the argument doesn't really do anything meaningful (it doesn't change the length value). I said this in like the first 3 comments =)

@prettycode prettycode Updating test to be more thorough.
d257ae0
@caitp caitp commented on the diff Dec 17, 2014
src/Angular.js
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
- dst[key] = obj[key];
+ var src = obj[key];
+
+ if (isDeep && isObject(dst[key])) {
@caitp
caitp Dec 17, 2014 Contributor

If dst[key] is anything other than an object, we really need to replace it with an object --- I think copy() will use an Array if the source is an array, too.

@prettycode prettycode Removal of delete per user caitp.
e49ed06
@caitp caitp commented on the diff Dec 17, 2014
src/Angular.js
var obj = arguments[i];
if (obj) {
var keys = Object.keys(obj);
for (var j = 0, jj = keys.length; j < jj; j++) {
var key = keys[j];
- dst[key] = obj[key];
+ var src = obj[key];
+
+ if (isDeep && isObject(dst[key])) {
+ src = extend(dst[key], src, true);
+ }
+
+ dst[key] = src;
@caitp
caitp Dec 17, 2014 Contributor

we just copied everything from src into dst[key] --- why are we clobbering dst[key] now? Should only do this if src is a non-object

@caitp caitp commented on the diff Dec 17, 2014
test/AngularSpec.js
@@ -223,6 +223,21 @@ describe('angular', function() {
// make sure we retain the old key
expect(hashKey(dst)).toEqual(h);
});
+
+ it('should perform deep extend when last argument is true', function() {
+ var src = { foo: { bar: 'foobar', age: 10, nothing: null, undef: void 0 }},
+ dst = { foo: { bazz: 'foobazz' }};
+ extend(dst, src, true);
+ expect(dst).toEqual({
+ foo: {
+ bar: 'foobar',
+ bazz: 'foobazz',
+ age: 10,
@caitp
caitp Dec 17, 2014 Contributor

This is not what I said before. Don't worry about the tests, I'll add some when I check this in :\

@prettycode

Sorry, I'm going to have to abandon this. I feel like I'm just embarrassing myself now. :) Maybe you can pull something useful out of the ashes. The environment on this machine is not setup for me to be properly writing/testing code and I've been trying to make the fixes during a holiday celebration at work.

@prettycode

I firmly think that if there's not an obvious deep extend (vs. the copy workaround) y'all will keep getting merge requests like mine until the end of time. :)

@caitp
Contributor
caitp commented Dec 17, 2014

@prettycode don't worry about it too much, its just some really simple comments that need to be addressed. Take your time and keep going, you're doing good =)

@caitp caitp added a commit to caitp/angular.js that referenced this pull request Dec 18, 2014
@caitp caitp refactor(extend): apply caitp's review comments for good measure :)
Derpity doot

Closes #10507
cdf07ef
@caitp caitp added a commit to caitp/angular.js that referenced this pull request Dec 18, 2014
@caitp caitp refactor(extend): apply caitp's review comments for good measure :)
Derpity doot

Closes #10507
f683061
@petebacondarwin petebacondarwin modified the milestone: 1.3.8 Dec 18, 2014
@btford btford modified the milestone: 1.3.8, 1.3.9 Dec 19, 2014
@petebacondarwin
Member

Closing in favour of #10519

@caitp caitp added a commit that referenced this pull request Mar 3, 2015
@caitp @petebacondarwin caitp + petebacondarwin feat(angular.merge): provide an alternative to `angular.extend` that …
…merges 'deeply'

Closes #10507
Closes #10519
c0498d4
@hansmaad hansmaad pushed a commit to hansmaad/angular.js that referenced this pull request Mar 10, 2015
@caitp caitp + hansmaad feat(angular.merge): provide an alternative to `angular.extend` that …
…merges 'deeply'

Closes #10507
Closes #10519
b794975
@netman92 netman92 added a commit to netman92/angular.js that referenced this pull request Aug 8, 2015
@caitp @netman92 caitp + netman92 feat(angular.merge): provide an alternative to `angular.extend` that …
…merges 'deeply'

Closes #10507
Closes #10519
558b2b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment