iterate maps with ng-for #2246

Closed
rolandjitsu opened this Issue May 31, 2015 · 35 comments

Projects

None yet
@rolandjitsu

I am trying to iterate over Map values with the built in ng-for, but I can see that for some reason it is not working.

In the specs, you can iterate over a Map iterable with:

let map = new Map();
let values = map.values();
for (let item of values) {
    console.log(item);
}

And none of the ES5 standard for (...)/for (... in ...) will work with that.

Does the built in ng-for actually support iterating over maps?

@mhevery
Member
mhevery commented May 31, 2015

Maps have no orders in keys and hence they iteration is unpredictable. This was supported in ng1, but we think it was a mistake and will not be supported in NG2

The plan is to have a mapToIterable pipe

<div *ng-for"var item of map | mapToIterable">

@mhevery mhevery closed this May 31, 2015
@rolandjitsu

@mhevery sounds fair enough. Can I already make use of a pipe inside ng-for that would transform that map into an array for instance? Or that has not been implemented yet?

@gdi2290
Member
gdi2290 commented May 31, 2015

@rolandjitsu this is kinda supported if you write it like this example

  <ul>
    <li *ng-for="var item of myMap.values(); var i = index">
    {{ item | json }} {{ i | json }}
    </il>
  </ul>
  <ul>
    <li *ng-for="var item of myMap.entries(); var i = index">
    {{ item | json }} {{ i | json }}
    <br>
    {{ item[0] | json }} {{ item[1] | json }}
    </il>
  </ul>
@Component({ selector: 'app' })
@View({ directives: [NgFor], template: template })
export class App {
  myMap: any;
  constructor( ) {
    this.myMap = new Map();
    this.myMap.set('Some Key', 'Some Value');
  }
}

@mhevery inside of *ng-for this doesn't work var i = index; var mykey=item[0]; var value=item[1]

<li *ng-for="var item of myMap.entries(); var i = index; var mykey=item[0]; var myvalue=item[1]">
  {{ mykey | json }}: {{ myvalue | json }}
</li>

compiler blows up when it sees [. This example doesn't work with JitChangeDetection with changeDetection: ON_PUSH but the default one is fine.

Oddly enough, the original example works in changeDetection: ON_PUSH and JitChangeDetection until ChangeDetection does another tick then it removes them

<template type="text/ng-template">
  <li *ng-for="var item of values">
</template>

<script type="text/ng-typescript">
@Component()
@View()
class App {
  constructor() {
    let map = new Map();
    map.set('Some Key', 'Some Value');
    this.values = map.values();
  }
}
</script>
@rolandjitsu

@gdi2290 I experience the same thing, it renders them but then they're gone.

P.S.:I do not fully understand the internals of the change detection (like when the next tick occurs).

@gdi2290
Member
gdi2290 commented Nov 23, 2015

Whenever you mutate the component, change the value of one of it's properties, then angular's change detection will be able to detect the changes in the component tree. If you want to learn more about ChangeDetection that's a post by Victor Savkin. I'm not sure why the side-effect above happens with JIT

@e-oz
e-oz commented Nov 25, 2015

Not supporting this is a mistake in ng2. It's a very often case when you don't care about order, you just need indexes of object for direct access (to display value in "View" mode, such as {{someList[someModel.id]}}), and to show options for select, <option *ng-for="#item of someList" [value]="item.id">{{item.title}}</option>.

@obscur666

try to use this pipe

@Pipe({ name: 'values',  pure: false })
export class ValuesPipe implements PipeTransform {
  transform(value: any, args: any[] = null): any {
    return Object.keys(value).map(key => value[key]);
  }
}

<div *ng-for="#value of object | values"> </div>
@e-oz
e-oz commented Dec 3, 2015

@obscur666 thank you. I did it already, but it means we multiply *2 size of list in memory and had to use more lines of code.

@marcj
marcj commented Dec 21, 2015

Not supporting this is a mistake in ng2

Jupp, especially when you take into consideration that all current modern browser sustain the key order of objects (except if you use numbers as key, but that is known). Libraries and developer are used to work with objects that way (especially for configurations). Angular's decision is based on theory, but the practice tells us something different. They should rather adjust the Javascript specification so Browsers officially aren't allowed to change the order theoretically anymore. ;)

@mhevery
Member
mhevery commented Dec 26, 2015

this can be better supported by a pipe which converts an object into array
and sorts the keys.

On Sun, Dec 20, 2015 at 6:06 PM Marc J. Schmidt notifications@github.com
wrote:

Not supporting this is a mistake in ng2

Jupp, especially when you take into consideration that all current modern
browser sustain the key order of objects (except if you use numbers as key,
but that is known). Libraries and developer are used to work with objects
that way (especially for configurations). Angular's decision is based on
theory, but the practice tells us something different. They should rather
adjust the Javascript specification so Browsers officially aren't allowed
to change the order theoretically anymore. ;)


Reply to this email directly or view it on GitHub
#2246 (comment).

@jhiemer
jhiemer commented Jan 19, 2016

@obscur666 when using your Pipe I ran into:

Expression 'entity.props | values in Component@140:26' has changed after it was checked.

This whole tweaking really feels a bit buggy in Angular 2. Are there more solid approaches?

@mhevery
Member
mhevery commented Jan 19, 2016

@jhiemer if it is a bug, then it should be filed, and fixed, rather than worked around it.

@jhiemer
jhiemer commented Jan 19, 2016

@mhevery the thing is, reading through other tickets this does not seem to be a bug. When I get an object and transform it into a array I need to return a new object, which results into the error.

http://stackoverflow.com/questions/34313743/expressionchangedafterithasbeencheckedexception-since-angular-2-beta

This behaviour is described in different tickets/stackoverflow entries.

@mhevery
Member
mhevery commented Jan 19, 2016

@vsavkin is working on a less strong dev mode, which will treat this as OK. But yes, in the strictest sense the pipe should make sure that it returns the same array.

@jhiemer
jhiemer commented Jan 19, 2016

@mhevery that would help a lot. The problem is see is getting an object with properties and returning an array. Normally I would tend to return a new array instead of changing the keys in the existing object. But that's definitely a matter of taste.

@laco0416
Contributor

@mhevery Hi.
I have trouble about a migration from ng-repeat to ngFor.
If so it is the design, I think that a solution for the migration also should be in the design. I wrote a pipe, objectToIterable, for this. But it's hacky.
Shouldn't something for smooth migration be provided by the library?

@gdi2290
Member
gdi2290 commented Jan 27, 2016 edited

I think the current design is fine. What's going on is that ngFor is enforcing the developer to shape the collection correctly.

The problem with ng-repeat was that it allowed for both objects and map which allowed more developers to shoot themselves in the foot (more often when they are concerned with the order). In Angular 1 a lot of "perf" problems were due to developers overloading ng-repeat without considering what is really going on (leaky abstraction). With ng-repeat what you're doing is inverting control of the management of your ViewModel from your controller to the helper directives to compose behavior. You're meant to refactor the behavior into a new directive for complex views such as a grid. With Angular 1, I would say no one took the time to take the next step of refactoring their behavior into components.

So what ngFor is doing, this time around, is enforcing best practices currently discovered in Angular 1 today (of managing the ViewModel in the component (container/controller/smart component) before passing it to the ngFor directive). While this may hurt migration a little bit, it forces developers down the correct path of managing their state correctly. Some developers even go so far as to say if Angular 1 did not include ng-repeat then we wouldn't have any performance problems since developers would have to consider how to manage their collections in a view. This change may also hurt prototyping which was one of Angular 1's strongest strength but someone could also create a ng-repeat/orderBy/filter directive/pipes which has the same API that relied on side effects

@colinjlacy

If anyone's interested, I wrote up a blog post following the suggested Pipe solution made by @mhevery: https://webcake.co/object-properties-in-angular-2s-ngfor/

@user414
user414 commented Mar 1, 2016

Ran into this issue, and used the solution posted somewhere since the pipe by @mhevery seem to have the same downside as here
#6392

which is it makes it read only. Instead just used something that was suggested somewhere else, sorry can't find where.

Add something like this in your class

@Input() jsonObject: Object;
...
keys() : Array<string> {
    return Object.keys(this.jsonObject);
}

Do something like this in the template

<template ngFor #currentKey [ngForOf]="keys()" #i="index">
      <div><strong>{{currentKey}}:</strong> {{jsonObject[currentKey]}}</div>
</template>
@IsaacBorrero

Just checking if the mapToIterable pipe has been added to ng2? Is there an issue item that I can track to get status updates?

@mhevery
Member
mhevery commented Apr 1, 2016 edited

@IsaacBorrero It has not, would you like to make a PR?

@IsaacBorrero

@mhevery I apologize, but what does that mean? Please let me know and I will gladly do it...

@amcdnl
Contributor
amcdnl commented Apr 13, 2016
@pstephenwille

This seems like a huge glitch. Is it that ordering is inconsistent, in which case, does that matter? Or that looping at all is problematic? At the minimum, I'd expect a default pipe to be available.

@svdb0
svdb0 commented Aug 3, 2016

@mhevery
For what it's worth, as opposed to the order of properties in an object, the order of the elements of a Map has been strictly defined from the first version of ECMAScript which introduced it.

The standard uses a list type to describe it; adding a new element appends it to the end of the list of the key-value pairs, and changing the value of an element leaves the order intact.
See http://www.ecma-international.org/ecma-262/6.0/#sec-map.prototype.set for the Map.prototype.set() function. Other operations use the same list representation.

If there are no other reasons than the perceived inconsistent ordering, considering that iterating through Maps would be very convenient, maybe another look at this could be justified?

@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 4, 2016
@harunurhan harunurhan feat(common) new pipe to support object for *ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
ae089bc
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 4, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
b5e2d37
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 5, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
9a74a1b
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 5, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
aef8d6a
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 5, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
f717d3a
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 18, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
2691bfe
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 20, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
b25dd2e
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 20, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
40b5932
@harunurhan harunurhan added a commit to harunurhan/angular that referenced this issue Sep 25, 2016
@harunurhan harunurhan feat(common): new pipe to support object for ngFor
Add keys_pipe which uses Object.keys in order to transform
an object to array of its keys.

Fix #2246
7a4ee77
@megalucio

So, for when are we going to have a mapToIterable pipe as a default in the angular2 commons?

@IsaacBorrero
IsaacBorrero commented Oct 10, 2016 edited

I agree with @megalucio ... Really looking forward to this, as it would allow me to take advantage of ES6 and build more robust collections.

But, Angular2 is awesome and I appreciate the work the team has put into it...

@e-oz
e-oz commented Oct 10, 2016

What's going on is that ngFor is enforcing the developer to shape the collection correctly.

So what ngFor is doing, this time around, is enforcing best practices currently discovered

Not in this case. Only thing we are doing in this case - write more code without any benefits.

@gdi2290
Member
gdi2290 commented Oct 10, 2016 edited

(opinions are my own)
A good example of where we are reverting control over the ViewModel from a developer concern to the template is below. (assuming we have something like keys pipe)

<ul>
  <li *ngFor="let key of asyncItems$ | async | keys">
    ?
  </li>
</ul>

|async is managing the subscription of the asyncItems$ Observable and value that comes out of it over time. After the value is emitted we convert that Model into a ViewModel which are the keys of the Model. The problem occurs when you're trying to grab the Model.

<ul>
  <li *ngFor="let key of asyncItems$ | async | keys">
    {{ key }} : {{ (asyncItems$ | async)[key] }}
  </li>
</ul>

To get the Model that the async pipe created in the template you now have to do the same operation for the template. Another problem occurs that the developer is unaware of and that the asyncItems$ is being subscribed more than once. For each |async request you're creating a new subscription. You can "fix" this with asyncItems$ being telling the Observable it's a multicast Observable via .share(). Each time you subscribe to the same Observable you will receive the same reference of the first value.

If the developer controls the Model/ViewModel then we would create the subscription in our component and create a reference to our ViewModel from our Observable

@Component({
  template: `
<ul>
  <li *ngFor="let key of viewModel">
    {{ key }} : {{ model[key] }}
  </li>
</ul>
`
})
class Home {
  asyncItems$ = Observable.of({ 'prop1': 'value1', 'prop2': 'value2'});
  model = {};
  viewModel = [];
  sub;
  constructor() {
    this.sub = asyncItems$.subscribe(model => {
      this.model = model;
      this.viewModel = Object.keys(model);
    });
  }
}

Notice how we're now able to reason about what's going on in our template since it's simpler to understand. Since the logic is simple to understand the problems we potentially had in the template-driven approach doesn't exist here. We are handling the complexity of the async value in our component where we have full control over what we expose to our template. If we want to determine the order of our Model we would just create the order in the component either before the subscribe or after.

@coli
coli commented Nov 4, 2016

let key of Object.keys(hash)

@MTyson
MTyson commented Nov 22, 2016

"Maps have no orders in keys and hence they iteration is unpredictable".

So what? Set iteration is unpredicable in Java, does that mean it shouldn't be iterated over?

No, this is just a mistake made in design that has become obvious in practice.

So much good in angular 2, just fix this thing please.

@pdavin
pdavin commented Jan 5, 2017

Should this issue be closed? On May 31 2015 @mhevery commented "The plan is to have a mapToIterable pipe". Is that still the plan? If that work isn't being tracked in this (closed) issue, is there another place I can follow along? Having it in ng2 core would be better than the many third party implementations getting propagated I think.
Thanks.

@DzmitryShylovich
Contributor

@pdavin #11319

@slpixe slpixe added a commit to slpixe/Tally that referenced this issue Jan 8, 2017
@slpixe slpixe got counters saving to db again, had to switch it to arrys for ngFor
angular/angular#11319
angular/angular#2246

might be pushed to stable soon ?
f29f171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment