angular.copy fails for objects implementing a forEach that depends on some internal state.

[jelbourn]

angular.forEach attempts to use an object's own forEach function if it is available.

angular.copy uses Object.create to make a new empty destination object with the same prototype as the source. angular.copy then uses angular.forEach to iterate over the keys of the destination object and delete any properties.

The problem: if a class implements it's own forEach, but that forEach depends on some class property normally initialized in its constructor, then this iteration over the destination's keys will result in an error.

Real example: The Google Closure goog.struct.Map. This implements its own forEach, but it that forEach depends on the keys_ property to have been initialized in the constructor. Other collections in goog.struct probably have the same problem.

Running contrived example: http://jsbin.com/qumajeyuse/1/edit?js,console
Running example with goog.struct.Map: http://jsbin.com/fuvaliboli/2/edit?js,console

[pkozlowski-opensource]

@jelbourn yeh, this is unfortunate.... Are you bumping into this issue while invoking angular.copy from your code directly? Or maybe the error manifests itself since AngularJS is invoking copy from some directives / helper functions?

The current way of coping objects is not ideal and it fails for other "special" objects (ex.: #10210). What would be good, maybe, is to have some way of specifying a cloning objects (instead of creating a new regular object).

I guess that recognizing .forEach was introduced to take advantage of the native forEach function, when available. But yeh, I can see why it fails in your case.

But yeh, if you are invoking angular.copy directly maybe it would be better to use another dedicated clone / copy function... I'm not sure how we could solve properly in AngularJS but if anyone has a better idea of checking for forEach it would be great to hear it now.

[gkalpak]

How about checking is obj is an array (i.e. instead of if (obj.forEach && obj.forEach !== forEach) use if (isArray(obj))). The obj.forEach check was there to accomodate for IE8's not supporting Array.prototype.forEach, but with IE8 out of the way, all array objects are expected to have a forEach method (and indeed we have code that relies on this assumption elsewhere).

We might miss some very rare cases where non-array objects do have a proper forEach method, but this is obviously negligible.

[pkozlowski-opensource]

@gkalpak we seem to have code branch for arrays / array-like objects already:

angular.js/src/Angular.js

Line 254 in 32806ca

} else if (isArray(obj) || isArrayLike(obj)) {

that kicks-in before.

[realityking]

@gkalpak Given that the new ES6 Set and Map objects fall into that category, this doesn't seem like a very feature compatible approach.

[gkalpak]

@realityking: When ES6 is supported by 1.x we can add checks for the specific types.

@pkozlowski-opensource: Too lazy to look that up 😇
In the case of goog.struct.Map, I think we wouldn't be able to copy it successfully anyway (even if we eliminatd the forEach branch). So, I don't think it is really a question of how to check for forEach anyway.

[pkozlowski-opensource]

Yes, agree, this is more a question of how to clone objects where creating a new instance with the same prototype is not enough. Or more generally a questions of limits of the built-in angular.copy. Or to be more concrete: should people be able to use angular.copy with objects which got "non-standard" (what is a standard here???) cloning semantic. In this particular case why not to simply use clone() of http://closure-library.googlecode.com/git-history/docs/class_goog_structs_Map.html?

[jelbourn]

@pkozlowski-opensource We ran into this directly using angular.copy after upgrading to 1.3. Our project has a workaround, but I imagine other Googlers will probably encounter this due to the prevalent use of Closure.

[pkozlowski-opensource]

@jelbourn Ok, thnx for clarifying. Can you think of a reasonable fix here (without explicitly testing for closure classes)? I must say that I'm short of ideas that would work in the general case....

[jelbourn]

@pkozlowski-opensource

I should have mentioned this earlier: the example we ran into was that we had an object that, deeply nested in its properties, had a goog.struct.Map that came from another library we depend on. The call to angular.copy is used in our wrapper service for $resource, which clones anything being saved and runs a pre-save transformation on the clone before sending the request. So it's a case of angular.copy running on any arbitrary data model.

As for a fix,

1. It's impossible to safely use an object's own forEach without that object having been created through its normal way, whatever that may be (constructor/factory/builder).
2. It's impossible for angular.copy to know the normal construction for an arbitrary object.

Because of these two things, it can't be safe to use angular.forEach inside of angular.copy. What would be wrong with doing a manual iteration over the object keys? Doesn't work for ECMA6 collections?