Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(angular): do not copy $$hashKey in copy/extend functions. #2423

Closed
wants to merge 1 commit into from
Closed

fix(angular): do not copy $$hashKey in copy/extend functions. #2423

wants to merge 1 commit into from

Conversation

spamdaemon
Copy link

Copying the $$hashKey as part of copy/extend operations makes little
sense since hashkey is used primarily as an object id, especially in
the context of the ngRepeat directive. This change maintains the
existing $$hashKey of an object that is being copied into (likewise for
extend).
It is not uncommon to take an item in a collection, copy it,
and then append it to the collection. By copying the $$hashKey, this
leads to duplicate object errors with the current ngRepeat.

Closes #1875

@petebacondarwin
Copy link
Member

There is already a PR related to this: #2382. Can you collaborate with them to get the best overall solution?
Thanks

@spamdaemon
Copy link
Author

I'm not sure whether its a good idea to ignore the $$ properties in either extend or copy. With regards to extend I found at least 1 use in location.js that relied on $$ properties to be copied over.

I'd almost want separate public and private version extend/copy because the use-cases are slightly different.

Btw, I've created a new function startsWith(x,prefix) which is efficient and doesn't create garbage on the heap. It would be nice to make that available via angular.startsWith().

@spamdaemon
Copy link
Author

I was wondering if copy($location) should work. If it should work then we cannot drop those $$ properties since they are referenced directly in the public interface of that service.

I think my first patch is semantically more correct and will result in fewer problems down the road.

@spamdaemon
Copy link
Author

Any feedback?

Copying the $$hashKey as part of copy/extend operations makes little
sense since hashkey is used primarily as an object id, especially in
the context of the ngRepeat directive. This change maintains the
existing $$hashKey of an object that is being copied into (likewise for
extend).
It is not uncommon to take an item in a collection, copy it,
and then append it to the collection. By copying the $$hashKey, this
leads to duplicate object errors with the current ngRepeat.

Closes #1875
@spamdaemon
Copy link
Author

I've decided to abandon the second patch and revert back to my initial patch.

@petebacondarwin
Copy link
Member

@spamdaemon - so where does this leave us? :-)

@spamdaemon
Copy link
Author

You can just merge this patch as far as I'm concerned. I've not heard from anyone whether the patch is fine or not.
I just did not think it was ok to drop the $$ properties in general.
So this patch does 2 things:

  1. it does not copy the $$hashKey over in both extend and copy functions
  2. it keeps the $$hashKey in the destination object, if it was already set.

@petebacondarwin
Copy link
Member

OK, looks good. Merged at 6d0b325 and 016e1e6. Thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

angular.copy() doesn't strip $$hashKey
2 participants