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

Angular.copy support for ES6 Map (and other ES6 types?) #16067

Closed
1 of 3 tasks
phasmal opened this issue Jun 22, 2017 · 6 comments
Closed
1 of 3 tasks

Angular.copy support for ES6 Map (and other ES6 types?) #16067

phasmal opened this issue Jun 22, 2017 · 6 comments

Comments

@phasmal
Copy link

phasmal commented Jun 22, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:
angular.copy when called on an object containing an ES6 Map object fails to copy the Map contents. This is surprising for the developer-user since they would expect the new object to contain all the attributes of the old one, including contents (angular.copy advertises as a deep copy)

Expected / new behavior:

Addition of ES6 Map copy would mean angular.copy behaves as expected regardless of the presence of a Map in an object.

I haven't looked at other ES6 types, but I would expect angular.copy should sensibly copy them as well, if it doesn't already.

Minimal reproduction of the problem with instructions:

> m = new Map()
Map(0) {}

> m.set('a', 'b')
Map(1) {"a" => "b"}

> o = {myProperty : m}
Object {myProperty: Map(1)}

> c = angular.copy(o)
Object {myProperty: Map}

> o.myProperty
Map(1) {"a" => "b"}

> c.m
Map {}

Angular version: 1.x.y

1.6.4-local+sha.617b36117

looking at latest code, it is iterating keys, which won't work for maps:
https://github.com/angular/angular.js/blob/master/src/Angular.js#L902

> m = new Map()
Map(0) {}

> m.set('a', 'b')
Map(1) {"a" => "b"}

> for (k in m) { console.log(k)}
undefined

Browser: [all | Chrome XX | Firefox XX | Edge XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

Chrome 59.0.3071.86 (Official Build) (64-bit)

Tried replicating on Firefox, but was an older version of angular and caused error:

> m = new Map()
Map {  }

> m.set('a', 'b')
Map { a: "b" }

> c = angular.copy(m)
TypeError: Map.prototype.forEach called on incompatible object

> angular.version
Object { full: "1.3.20", major: 1, minor: 3, dot: 20, codeName: "shallow-translucence" }

Looking at code, though it wouldn't work for Map on any browser that doesn't allow key iteration over map values (every browser I suspect).

For example, on FF:

> for (k in m) { console.log(k)}
undefined

Anything else:

This overlaps with the following issue:
#10304

However, that issue is talking about general 'iterable' object support (of which ES6 collections are a sub-set) which is a more intractable problem. This is focused specifically on ES6 collections.

Suggest treating Map similar to Array as a special case and using the built-in iteration features to populate the copy.

Alternately, define a global javascript 'clone' standard that objects can declare support for and make angular.copy redundant ;)

@frederikprijck
Copy link
Contributor

There's an open (but stale) PR regarding this:
#15697

@Narretz Narretz added this to the Purgatory milestone Jun 23, 2017
@jbedard jbedard self-assigned this May 3, 2018
Narretz added a commit that referenced this issue Jan 21, 2019
@phasmal
Copy link
Author

phasmal commented Jan 22, 2019

Adding a list of unsupported types doesn't change this feature request. I'm still requesting that copy supports Map/other ES6 types rather than not supporting them.

@phasmal
Copy link
Author

phasmal commented Jan 22, 2019

... unless this is saying 'won't fix this' separate from the fact that it's been commented that they're not supported?

Otherwise, can we have this reopened? or should I copy/paste the description into a new request?

@frederikprijck
Copy link
Contributor

I'd guess it's due to the fact that AngularJS is currently in LTS that features like this aren't getting added.

@petebacondarwin
Copy link
Contributor

@phasmal - indeed @frederikprijck is correct. We are now well into the LTS period and we are not adding new features to AngularJS.

@phasmal
Copy link
Author

phasmal commented Jan 22, 2019

Ahh, great, thanks for the clarification - that makes much more sense!

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

No branches or pull requests

5 participants