-
Notifications
You must be signed in to change notification settings - Fork 27.5k
fix(copy): preserve property descriptors #8034
fix(copy): preserve property descriptors #8034
Conversation
Instead of values being simply copied, this change allows to preserve definitions of own properties. Thus, enumerable (or not), writable (or not), getter/setter, etc. property definitions are preserved across copies. It improves change from b59b04f preserving prototype chain. As a side effect, it also includes previously ignored non-enumerable properties by using `Object.getOwnPropertyNames` to iterate **every** own properties instead of `for ... in ...` + `hasOwnProperty`.
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
So far, angular.copy was copying all properties including those from prototype chain and was losing the whole prototype chain (except for Date, Regexp, and Array). Deep copy should exclude properties from the prototype chain because it is useless to do so. When modified, properties from prototype chain are overwritten on the object itself and will be deeply copied then. Moreover, preserving prototype chain allows instanceof operator to be consistent between the source object and the copy. Before this change, var Foo = function() {}; var foo = new Foo(); var fooCopy = angular.copy(foo); foo instanceof Foo; // => true fooCopy instanceof Foo; // => false Now, foo instanceof Foo; // => true fooCopy instanceof Foo; // => true The new behaviour is useful when using $http transformResponse. When receiving JSON data, we could transform it and instantiate real object "types" from it. The transformed response is always copied by Angular. The old behaviour was losing the whole prototype chain and broke all "types" from third-party libraries depending on instanceof. Closes #5063 Closes #3767 Closes #4996 BREAKING CHANGE: This changes `angular.copy` so that it applies the prototype of the original object to the copied object. Previously, `angular.copy` would copy properties of the original object's prototype chain directly onto the copied object. This means that if you iterate over only the copied object's `hasOwnProperty` properties, it will no longer contain the properties from the prototype. This is actually much more reasonable behaviour and it is unlikely that applications are actually relying on this. If this behaviour is relied upon, in an app, then one should simply iterate over all the properties on the object (and its inherited properties) and not filter them with `hasOwnProperty`. **Be aware that this change also uses a feature that is not compatible with IE8.** If you need this to work on IE8 then you would need to provide a polyfill for `Object.create` and `Object.getPrototypeOf`.
sorry but angular.copy is primarily intended for the internal needs of the framework (and it was our mistake to expose it). having said that we don't have intentions to change the implementation to support use-cases not needed by the framework. |
@IgorMinar I agree This is not a real blocker for me. I was just trying to improve it. It didn't seem to introduce breaking changes at first and tests were all passing. That said, thank you very much for your great work for Angular as a whole. |
@IgorMinar - I think we should relook at this PR. If we are copying objects in Angular we need to make sure that we copy everything. |
Having discussed this with Igor, we feel that ensuring high fidelity when copying objects that contain dynamic properties adds too much overhead to the We are going to consider copying non-enumerable value properties though, since not being enumerable does not necessarily imply that they should not be copied. But we need to ensure that this is done in a way that keeps their non-enumerability status while not impacting on the performance of |
@petebacondarwin @IgorMinar I have open PR for this: #15693 |
Instead of values being simply copied, this change allows to preserve
definitions of own properties. Thus, enumerable (or not), writable (or
not), getter/setter, etc. property definitions are preserved across copies. It
improves change from b59b04f preserving prototype chain.
As a side effect, it also includes previously ignored non-enumerable properties
by using
Object.getOwnPropertyNames
to iterate every own properties insteadof
for ... in ...
+hasOwnProperty
.