Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[glimmer2] fix #each with key option test #13721

Merged
merged 1 commit into from
Jun 20, 2016

Conversation

fivetanley
Copy link
Member

In Ember, you can remove duplicates in an array by providing the key
option to the {{#each}} helper.

For example, if given the following model property:

[
  {
    "id": "1",
    "name": "Robert Jackson"
  },
  {
    "id": "1",
    "name": "Robert Jackson with diff name but same ID"
  },
  {
    "id": "2",
    "name": "Katie Gengler"
  }
]

And the following template:

{{#each model key="id" as |person|
  {{person.name}}
{{/each}}

It would render the following:

Robert Jackson
Katie Gengler

Even though we have two objects with "Robert Jackson" as the name, they
share the same "id", so Ember will only render the first object it
encounters with that "id" while iterating. That's why "Robert Jackson"
shows up but not "Robert Jackson with diff name but same ID".

This test was adding in a new object (in our case {text: ' '}), but an
object with {text: ' '} already existed in the setup line. Because it
was cached by the same 'text' value, it did not show up in the
subsequent rerender.

We change it to a different text value so we can assert it was added
correctly in the list.

refs #13644

In Ember, you can remove duplicates in an array by providing the `key`
option to the `{{#each}}` helper.

For example, if given the following `model` property:

```javascript
[
  {
    "id": "1",
    "name": "Robert Jackson"
  },
  {
    "id": "1",
    "name": "Robert Jackson with diff name but same ID"
  },
  {
    "id": "2",
    "name": "Katie Gengler"
  }
]
```

And the following template:

```handlebars
{{#each model key="id" as |person|
  {{person.name}}
{{/each}}
```

It would render the following:

```html
Robert Jackson
Katie Gengler
```

Even though we have two objects with "Robert Jackson" as the name, they
share the same "id", so Ember will only render the first object it
encounters with that "id" while iterating. That's why "Robert Jackson"
shows up but not "Robert Jackson with diff name but same ID".

This test was adding in a new object (in our case `{text: ' '}`), but an
object with `{text: ' '}` already existed in the setup line. Because it
was cached by the same `'text'` value, it did not show up in the
subsequent rerender.

We change it to a different text value so we can assert it was added
correctly in the list.

refs emberjs#13644
@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2016

This seems like a good fix for this test, but we definitely need to fix the duplicate key case in general. There are a few "duplicate keys with each" tests, can you confirm?

@chancancode
Copy link
Member

👍 seems fine to fix this particular test, but also 👍 to fixing the collision case in general (and make sure we have tests for those).

The plan is to track collision inside the iterator on the ember side, and fallback to using a guid (or whatever we were doing before). Do we have tests for not specifying a key? I believe that should use the guid as well.

@fivetanley
Copy link
Member Author

to fixing the collision case in general (and make sure we have tests for those).

tests: #13722

Do we have tests for not specifying a key?

Yeah.

Objects
Primitives

@rwjblue
Copy link
Member

rwjblue commented Jun 19, 2016

@chancancode - Using the item being iterateds guid isn't enough, unless you are saying that we can never iterate over an array that contains the same object. The same assumption was made during initial Ember 1.13 work, and was fixed by @krisselden in tildeio/htmlbars#393. For example:

let obj = { foo: 'bar' };
let array = [ obj, obj, obj ];

It should definitely be possible to {{#each over that array...

@chancancode
Copy link
Member

Yeah, Kris told me about it but I forgot the details. s/guid/the Kris algorithm/

@rwjblue rwjblue merged commit d2b56a2 into emberjs:master Jun 20, 2016
@rwjblue rwjblue deleted the fix-each-key-test branch June 20, 2016 01:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants