refactor(extend): apply caitp's review comments for good measure :)

Derpity doot

Closes #10507

Author: caitp

src/Angular.js

@@ -333,37 +333,43 @@ 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: Pass `true` in as the last argument to perform a recursive merge (deep copy).
  *
  * @param {Object} dst Destination object.
  * @param {...Object} src Source object(s).
+ * @param {boolean=} deep if the last parameter is set to `true`, objects are recursively merged
+ *    (deep copy). Defaults to `false`.
  * @returns {Object} Reference to `dst`.
  */
 function extend(dst) {
   var h = dst.$$hashKey;
   var argsLength = arguments.length;
-  var isDeep = (argsLength >= 3) && (arguments[argsLength - 1] === true);
+  var isDeep = false;
+  if (argsLength >= 3) {
+    var maybeIsDeep = arguments[argsLength - 1];
+    // Secret code to use deep extend without adding hash keys to destination object properties!
+    if (maybeIsDeep === true || maybeIsDeep === 0xFACECAFE) isDeep = maybeIsDeep;
+  }
 
   if (isDeep) --argsLength;
 
   for (var i = 1; i < argsLength; i++) {
     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];
-        var src = obj[key];
-        
-        if (isDeep && isObject(dst[key])) { 
-          src = extend(dst[key], src, true);
-        }
+    if (!isObject(obj) && !isFunction(obj)) continue;
+    var keys = Object.keys(obj);
+    for (var j = 0, jj = keys.length; j < jj; j++) {
+      var key = keys[j];
+      var src = obj[key];
 
+      if (isDeep && isObject(src)) { 
+        if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {};
+        extend(dst[key], src, 0xFACECAFE);
+      } else {
         dst[key] = src;
       }
     }
   }
 
-  setHashKey(dst, h);
+  if (isDeep !== 0xFACECAFE) setHashKey(dst, h);
   return dst;
 }
 

test/AngularSpec.js

@@ -225,18 +225,66 @@ describe('angular', function() {
     });
 
     it('should perform deep extend when last argument is true', function() {
-      var src = { foo: { bar: 'foobar', age: 10, nothing: null, undef: void 0 }},
+      var src = { foo: { bar: 'foobar' }},
           dst = { foo: { bazz: 'foobazz' }};
       extend(dst, src, true);
       expect(dst).toEqual({
         foo: {
           bar: 'foobar',
           bazz: 'foobazz',
-          age: 10,
-          nothing: null,
-          undef: void 0
         }
       });
+      expect(dst.foo.$$hashKey).toBeUndefined();
+    });
+
+
+    it('should replace primitives with objects when deep extending', function() {
+      var dst = { foo: "bloop" };
+      var src = { foo: { bar: { baz: "bloop" }}};
+      extend(dst, src, true);
+      expect(dst).toEqual({
+        foo: {
+          bar: {
+            baz: "bloop"
+          }
+        }
+      });
+    });
+
+
+    it('should replace null values with objects when deep extending', function() {
+      var dst = { foo: null };
+      var src = { foo: { bar: { baz: "bloop" }}};
+      extend(dst, src, true);
+      expect(dst).toEqual({
+        foo: {
+          bar: {
+            baz: "bloop"
+          }
+        }
+      });
+    });
+
+
+    it('should not deep extend function-valued sources when deep extending', function() {
+      function fn() {}
+      var dst = { foo: 1 };
+      var src = { foo: fn };
+      extend(dst, src, true);
+      expect(dst).toEqual({
+        foo: fn
+      });
+    });
+
+
+    it('should create a new array if destination property is a non-object and source property is an array when deep-extending', function() {
+      var dst = { foo: NaN };
+      var src = { foo: [1,2,3] };
+      extend(dst, src, true);
+      expect(dst).toEqual({
+        foo: [1,2,3]
+      });
+      expect(dst.foo).not.toBe(src.foo);
     });
   });