BUG: angular.merge breaks some objects

[Doomic]

I believe this is a bug. The angular merge function will break some objects. I found this issue because of another plugin called Angular-Chart. This plugin build charts (with the use of chartjs) in the angular way.
I posted the issue there, but in depth i believe this is an angular bug.
because the object generated with ctx.createLinearGradient function will be merged like it is an object, but should be treaded as value.
If you want to know the whole issue, you can find it here:
jtblin/angular-chart.js#510

Shortly, the merge function should check if the item is a normal object or a special object to deside if going deeper or just putting the object like a value it the target is the correct way.

bellow how jquery does this check

var normalObject = {};
var gradient = ctx.createLinearGradient(0, 0, 250, 0);

typeof normalObject -> "object"
typeof gradient -> "object"

toString.call(normalObject) -> "[object Object]"
toString.call(gradient) -> "[object CanvasGradient]"

if more info is needed, please let me know

[Narretz]

Angular.merge is known to not support every type of value / object. We actually advise to use more general merge functions, such as thise provided by lodash etc. for general use cases. Angular-chart could make that change.
If a fix isn't too involved we could also consider merging a PR, if provided.

[Doomic]

So you are saying it is up to angular-chart to fix this?
sorry, but your last sentence confused me and i am not realy sure what you mean by that (english isn't my native language). Are you saying if the fix is a simple piece of code it can be implimenteted by angular as well?
In that case, here is you 3 extra lines of code

else if (toString.call(src) != "[object Object]") { 
  dst[key] = src;
}

where the final code will look like this:

function baseExtend(dst, objs, deep) {
  var h = dst.$$hashKey;

  for (var i = 0, ii = objs.length; i < ii; ++i) {
    var obj = objs[i];
    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 (deep && isObject(src)) {
        if (isDate(src)) {
          dst[key] = new Date(src.valueOf());
        } else if (isRegExp(src)) {
          dst[key] = new RegExp(src);
        } else if (src.nodeName) {
          dst[key] = src.cloneNode(true);
        } else if (isElement(src)) {
          dst[key] = src.clone();
        } else if (toString.call(src) != "[object Object]") { 
          dst[key] = src;
        } else {
          if (!isObject(dst[key])) dst[key] = isArray(src) ? [] : {};
          baseExtend(dst[key], [src], true);
        }
      } else {
        dst[key] = src;
      }
    }
  }

  setHashKey(dst, h);
  return dst;
}

[Narretz]

It's definitely easier to fix it in angular-chart, because they can simply switch to another merge function. In core, we discuss each of the changes to angular.merge individually, because we don't want to blow up the code for this function.

[jtblin]

@Narretz, author of angular-chart here, I'd rather not bring another dependency to the library or have to roll my own merge function. Given my library provides chart directives for angular, using angular native merge function is best from an assets size perspective.

The fix provided by @Doomic seems to add only 3 lines of code and is taken from jQuery merge function which doesn't have this problem.

Moreover if it's breaking in my library, it is probably causing other issues as well and should be fixed here.

Edit: happy to send a PR if needed.

[Narretz]

The native angular.* helpers are almost notorious for not providing every bit of functionality needed for every library / purpose.
But if the change really is simple and small, please open a PR with a test for the new behavior, so we can see if it breaks anything.

[gkalpak]

@jtblin, would angular.copy() work for your usecase? It seems to handle this a little better (because it retains the prototype).

[jtblin]

Sorry for the lag, does angular.copy() performs a deep copy?

[gkalpak]

@jtblin, yes.

[jtblin]

Reading the docs, it doesn't seem to merge but it replaces instead so I don't think that would work:

If a destination is provided, all of its elements (for arrays) or properties (for objects) are deleted and then all elements/properties from the source are copied to it.

I'll send a PR as suggested by @Narretz

[gkalpak]

@jtblin, yes, if you need to actually merge objects, copy won't work. It is only for (deeply) copying.