Key functions for `each`/`each-in` by mmun · Pull Request #321 · emberjs/rfcs

mmun · 2018-03-24T21:23:03Z

Closes #306

Turbo87 · 2018-03-25T13:48:56Z

+
+If `key` is a function, then Glimmer's diffing algorithm will call this function to determine the item's key.
+
+For `each`, the function would be called with arguments `keyFn(item, index)`.


this seems strange because in an array the index is sort of like that key, which means in one case we're passing key, value and value, key in the other case 🤔

The arguments are intended to match the order of the block params for those helpers.

Turbo87 · 2018-03-25T13:49:16Z

+For `each`, the function would be called with arguments `keyFn(item, index)`.
+For `each-in`, the function would be called with arguments `keyFn(key, value)`.
+
+The key function should not be invoked with a `this`.


what does that mean?

That glimmer should call the key function as keyFn(...args) as not to bind this to any particular value.

but with #308 would we then need to use key=this.someUnboundFunction? would seem strange if that was actually not bound to this in that case 🤔

I don't think it's strange. For example,

{{some-component onClick=this.someUnboundFunction}}

is also not bound. This is generally true (albeit a bit annoying!) in Handlebars today. You have to write

{{some-component onClick=(action this.someUnboundFunction)}}

yeah, true, I guess passing around this.someUnboundFunction in JS also doesn't have to mean that it's bound...

The key function should not be invoked with a this.

I think due to #308 we should rephrase this as something like "The key function will not be bound to this automatically", since in the future we will have to call it with this if #308 gets merged.

👍 That phrasing is better.

rwjblue

Left a couple of small tweak suggestions inline, but I'm definitely 👍 on this...

rwjblue · 2018-03-27T12:54:42Z

+
+## Motivation
+
+The `key` argument to `each` and `each-in` determines the key used in the Glimmer diffing algorithm. Here is a description of the currently accepted values?


The ending question mark seems pretty out of place here. Is this a description of the current support, or is the RFC asking for clarification?

https://www.youtube.com/watch?v=tqNhEzrWQpY

Yeah, I don't know what I was thinking?

rwjblue · 2018-03-27T12:57:48Z

+
+## How we teach this
+
+This feature just needs to be documented in the API docs and possibly the guides. We may want to reframe the existing (string-based) key documentation to be explained as shorthands for key functions.


I'd prefer to remove "just" here, otherwise 👍

cibernox · 2018-03-31T08:36:04Z

+
+`key` argument | `each` key function                | `each-in` key function
+---------------|------------------------------------|-----------------------------------
+`@identity`    | `(item, index) => guidFor(item)`   | `(key, value) => guidFor(value)`


One possible suggestion I have is to reverse the order of the arguments in one of the functions. I believe that the value should be in the same position in both.

key argument each key function each-in key function

@identity (item, index) => guidFor(item) (value, key) => guidFor(value)

or

key argument each key function each-in key function

@identity (index, item) => guidFor(item) (key, value) => guidFor(value)

I have the feeling that it would make the table simpler:

key argument key function for both

@identity (_, value) => guidFor(value)

@index/@key (ref, _) => ref

other strings (_, value) => get(value, path)

However I also see the convenience on {{each}} taking the arguments in the same order that .forEach and {{each-in}} having them as (key, value)

That order seems plausible since it's the order you get when calling the Array.prototype.entries and Map.prototype.entries iterators, however I think it still makes more sense to mirror the block param order.

Additionally, I don't like promoting array index to the first slot because you almost never want to use it as a key (unless you know the whole #each block is stateless and you know that no one will ever add a stateful component/helper into it in the future, see https://medium.com/@robinpokorny/index-as-a-key-is-an-anti-pattern-e0349aece318).

Yeah, I'm not really opposed to the proposed order, I just thought that this one presented a "chance for unification", in case that was desirable or simplified the implementation in any form. I'm happy either way.

bgentry · 2018-05-23T23:26:06Z

I'm definitely a 👍 on this.

With ember-apollo-client, we're always just passing either POJOs wrapped in an EmberObject or NativeArrays. Out-of-the-box, the arrays do not work cleanly with Ember unless you pass key="id" or similar to avoid re-rendering the components for the entire list. That's because the POJOs themselves do not have the same guid, so Ember thinks they're different.

However, users typically already have to tell Apollo how to get a unique identifier from their objects by configuring a dataIdForObject() function. That gets used for cache normalization purposes and typically just defaults to obj => obj.id for apps with globally unique IDs. I would love to use this same function to make it easier for users to get correct behavior on re-renders. The suggestion from @mmun was to build an {{#apollo-each}} helper that gets AST transformed to reference the key function. This RFC would make that possible.

The proposal itself seems straightforward and I would love to see it built.

wagenet · 2022-07-23T17:58:52Z

Seem like there has been some solid interest for this. I'm going to see if we can get some better review and a proper yay or nay on it.

wagenet · 2023-01-27T19:24:44Z

I'm moving this FCP to close since it's had virtually no activity in the last few years. If someone wants to pick this up and make sure the RFC is up to date, we can re-open.

mmun changed the title ~~Key Functions for each/each-in~~ Key functions for each/each-in Mar 24, 2018

Key functions for each/each-in

d011278

mmun force-pushed the key-functions branch from b2c5de6 to d011278 Compare March 24, 2018 21:23

Turbo87 reviewed Mar 25, 2018

View reviewed changes

rwjblue assigned mmun Mar 27, 2018

rwjblue reviewed Mar 27, 2018

View reviewed changes

cibernox reviewed Mar 31, 2018

View reviewed changes

bgentry mentioned this pull request May 25, 2018

a change to an array item causes entire list to re-render ember-graphql/ember-apollo-client#135

Closed

rwjblue mentioned this pull request Jun 8, 2018

Documentation for special values of key in each template helper emberjs/ember.js#16719

Closed

rwjblue mentioned this pull request Jun 20, 2018

Allow passing a function as the key for {{each}} #306

Closed

locks added Needs Champion T-framework RFCs that impact the ember.js library labels Jan 25, 2019

chancancode mentioned this pull request Jan 26, 2019

#each re-render bug emberjs/ember.js#16975

Open

wagenet added S-Proposed In the Proposed Stage S-Exploring In the Exploring RFC Stage and removed S-Proposed In the Proposed Stage labels Dec 2, 2022

wagenet added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Jan 27, 2023

ef4 closed this Feb 3, 2023


		If `key` is a function, then Glimmer's diffing algorithm will call this function to determine the item's key.

		For `each`, the function would be called with arguments `keyFn(item, index)`.


		## Motivation

		The `key` argument to `each` and `each-in` determines the key used in the Glimmer diffing algorithm. Here is a description of the currently accepted values?


		## How we teach this

		This feature just needs to be documented in the API docs and possibly the guides. We may want to reframe the existing (string-based) key documentation to be explained as shorthands for key functions.

`key` argument	key function for both
`@identity`	`(_, value) => guidFor(value)`
`@index`/`@key`	`(ref, _) => ref`
other strings	`(_, value) => get(value, path)`

Conversation

mmun commented Mar 24, 2018 • edited by rwjblue Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmun Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwjblue left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmun Mar 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgentry commented May 23, 2018

Uh oh!

wagenet commented Jul 23, 2022

Uh oh!

wagenet commented Jan 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

mmun commented Mar 24, 2018 •

edited by rwjblue

Loading

mmun Mar 25, 2018 •

edited

Loading

mmun Mar 31, 2018 •

edited

Loading