ember.js icon indicating copy to clipboard operation
ember.js copied to clipboard

changed each-behaviour for indexed arrays in 2.10

Open Suven opened this issue 9 years ago • 8 comments

Hey!

I just noticed that Ember 2.10 introduces a changed behaviour when looping over indexed arrays with unset indexes.

Let's assume this array:

let a = [];
a[5] = 'foo';
a[6] = 'bar';

and this hbs:

{{#each testArray as |v k|}}{{k}},{{/each}}

In 2.9 this produces 5,6 and in 2.10 it produces 0,1,2,3,4,5,6.

I also made a twiddle for that

Suven avatar Dec 16 '16 11:12 Suven

@Suven that seems like a bug. Perhaps the implementation has changed from using forEach to a for loop. (just a guess).

⚡ cat changed-each.js

a = [];
a[5] = 'foo';
a[6] = 'bar';

for (i = 0; i < a.length; i++) {
  console.log(i, a[i]);
}

a.forEach(function(v, i) {
  console.log(i, v);
});

⚡ node changed-each.js

0 undefined
1 undefined
2 undefined
3 undefined
4 undefined
5 'foo'
6 'bar'
5 'foo'
6 'bar'

pixelhandler avatar Dec 16 '16 16:12 pixelhandler

Yup, that might be. I guess for-loops were tempting with perfomance having in mind first.

Just a little remark (although I guess that's obvious): This also happens when only the value is accessed (i.e each myArray as |val|)

Suven avatar Dec 16 '16 17:12 Suven

I can confirm that it's bugged. Also using each-in instead of each temporary solves this problem.

kamilogorek avatar Dec 27 '16 20:12 kamilogorek

The issue here is that (for performance reasons) we are using a standard for loop for {{#each support in 2.10+. Prior versions used .forEach and that natively support sparse arrays (and that is one of the reasons for some known performance issues).

We did it intend to support sparse arrays in prior versions and making changes to 2.10+ to support them would negatively impact performance of all usages of {{#each which is not an acceptable trade off for us.

We should document the fact that we do not support sparse arrays with {{#each in the API docs and add a test (likely a tweak of #14760) that shows the current behavior is intentional.

For folks that definitely need this support, I'd recommend using {{#each-in as someone else mentioned above.

rwjblue avatar Dec 27 '16 22:12 rwjblue

@Suven I labelled this as "documentation" per @rwjblue comments above

For folks that definitely need this support, I'd recommend using {{#each-in as someone else mentioned above.

pixelhandler avatar Sep 14 '18 17:09 pixelhandler

@rwjblue is it alive?

lifeart avatar Sep 10 '20 12:09 lifeart

We need to write the docs I mentioned in my last comment.

rwjblue avatar Sep 22 '20 21:09 rwjblue

@rwjblue https://github.com/emberjs/ember.js/pull/19147

lifeart avatar Oct 17 '20 20:10 lifeart