key= not working as expected

[amk221]

When specifying key="@index", rows do not seem to be stabilising as expected (perhaps I am expecting the wrong thing?)

Please see this screencast and demo repo for an example:

https://github.com/amk221/-ember-vertical-collection-issue#ember-vertical-collection-issue

Each person has some tags. (Their ids match for the simplicity of the test case).
However, upon scrolling notice how the tag id no longer matches the person for that row.

[pzuraq]

The issue you're having is caused by the fact that this computed property does not have any dependent keys: https://github.com/amk221/-ember-vertical-collection-issue/blob/master/app/components/person-tags.js#L7

Vertical collection recycles components, which means that you have 8 table rows that are never being fully destroyed or recreated. The computed property won't update unless you change something it's dependent on, person in this case. You can disable recycling for a performance hit by setting shouldRecycle=false.

[amk221]

Yes, I thought that would be mentioned. Thanks for the shouldRecycle tip.

I mention this because, in the past, I've had this same issue with a standard {{#each}}. Specifying key= lets me control the re-use. (At least, I thought it did?). And I expected VC to behave the same.

[runspired]

Roughly speaking, key does the same for {{each as it does for {{vertical-collection.

In both cases it allows the diff algorithm to determine when an item in the previous state of the list matches an item in the new state of the list.

In the case of {{each, this happens to mean that any item that has a stable key from one state to the next state will have a component instance that is stable. Any item that does not will be a newly instantiated component.

In the case of {{vertical-collection, this means that any item that has a stable key from one state to the next state will have a state-object instance that is stable. Any item that does not will be a newly instantiated state-object. It would defeat the purpose of recycling and infinite scroll to tie unique key to a unique component instance.

Fundamentally, recycling components in Ember or Glimmer today has fundamental difficulties due to the non FP nature of some older Ember features and patterns. Generally speaking, setting state in init is bad. Similarly, avoid the use of {{unbound and of computed properties without dependent keys. If you make your components functional in nature, recycling becomes clean and efficient.

I don’t think that @amk221 wants to use shouldRecycle: false here, fundamentally his example app is an example of a non functional-programming pattern that should be linted against.

Changing to the following would resolve the issue

tags: computed('person.tags.length', function() {
    return this.get('person.tags').map(t => t.name);
})

If for some reason this to be very expensive and something that required more stability, moving this calculation higher up the chain would be better

e.g.

Person {
  tags = [];
  tagNames = computed('tags.length', function() { return this.tags.map(t => t.name); };
}

[amk221]

Thanks for the write up.

My use of computed with no dependant keys was intentional (I'm aware it would have been better in init).

Using key= with #each results in the behaviour I expected (a new component instance, and therefore, the computed property computing again).

So I think my point that key= doesn't behave like I'd expect in VC is still a valid one. It may catch others out too.