fix(merge): treat dates as atomic values instead of objects.

[gabrielmaldi]

Makes angular.merge copy dates correctly.

Before and after this change:

[gkalpak]

In the docs, it says that merge() "recursively descends into object properties of source objects, performing a deep copy". That is so not true :(
It performs a "deep extend".

In that sense, this PR seems to be doing the right thing.

[gabrielmaldi]

I think we should also include this:

dst[key] = isDate(src) ? new Date(src.valueOf()) : src;

so that dates get cloned instead of assigning a reference to the same object.

[gkalpak]

I don't think this is the right think to do. It will also apply to non-deep copying, where I believe it would be better to just assign the same Date object.

It's OK for deep extending I guess, so maybe something like:

if (deep && isObject(src)) {
  if (isDate(src)) {
    dst[key] = new Date(src.valueOf());
  } else {
    if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {};
    baseExtend(dst[key], [src], true);
  }
} else {
  dst[key] = src;
}

[gabrielmaldi]

I agree! Done and thanks for the snippet.

[gkalpak]

The implementation lgtm. But we should have some tests in place, in order to ensure there will be no accidental regressions in the future.

@gabrielmaldi, could add a test (i.e. ones that would fail without this fix and pass with it) ?

[gabrielmaldi]

@gabrielmaldi, could add a test (i.e. ones that would fail without this fix and pass with it) ?

Sure, I'll work on it later today.

[gabrielmaldi]

I wrote a couple of tests but didn't push them because one is failing and I believe it's an issue with the version of Jasmine which Angular is using.

Here's a gist with the tests; it fails on the last line with this error:

I wrote this pen which uses Jasmine 2.0.0 and Angular 1.4.0-rc.1 and the test passes just fine.

I also tried using:

var dst2 = {};
brokenMerge(dst2, src);
expect(dst2).not.toEqual({
  foo: date
});

but it fails with the same error:

Thanks for your help.

EDIT: I also tried separating the tests and I get the same error.

[gkalpak]

Angular is using jamsine 1.3. In this pen, I have a couple of tests that are currently failing.
I would expect them to pass with your fix. (If that is indeed the case, you can just use those two (maybe update ther descriptions if necessary).)

_{In case it wasn't clear, you don't have a brokenMerge in your testcase.
It suffices to have tests that would have failed, were they run prior to the fix and pass with the fix.}

[gabrielmaldi]

Yes, your tests pass with the fix. I guess Jasmine 1.3 behaves differently and using valueOf to compare dates is a valid workaround. Thanks for your guidance.