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

Deprecate {{each}} without a key specified. #11337

Closed
rwjblue opened this issue Jun 3, 2015 · 22 comments
Closed

Deprecate {{each}} without a key specified. #11337

rwjblue opened this issue Jun 3, 2015 · 22 comments
Milestone

Comments

@rwjblue
Copy link
Member

rwjblue commented Jun 3, 2015

The default key of index is confusing. We should deprecate usage of keyless {{each}}.

@rwjblue rwjblue added this to the 1.13.0 milestone Jun 3, 2015
@jayphelps
Copy link
Contributor

A good number of each's in the various apps I've worked on do not have any stable unique prop to key to. It's quite a pain to have to decorate each item with a key. Can this be helped in some other way? What about auto-generating Ember.guidFor per item, perhaps stored in Ember.meta? I realize it's more brittle than a user-provided string, because it relies on the objects being the same references, but maybe most of the time that is the case? (just thoughts)

In testing canary with one of our apps, we noticed some incorrect dom-reuse when the array changed. (more info on that as we discover the cause, we'll create a ticket)

@rwjblue
Copy link
Member Author

rwjblue commented Jun 3, 2015

There will be "@Index" and "@Guid" that you can use (@Index is what we are using now by default).

@rwjblue
Copy link
Member Author

rwjblue commented Jun 4, 2015

In testing canary with one of our apps, we noticed some incorrect dom-reuse when the array changed.

This is generally because of the default usage of the item's array index as key.

@knownasilya
Copy link
Contributor

What about defaulting to "@Guid"?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 4, 2015

It has been done.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 4, 2015

Whoops, sorry I thought you meant adding @guid as an option. It isn't the default because Ember.guidFor actually mutates the object you call it on (to add __embermeta134124131 with the guid). In the long term, we could use an ES6 Map but there isn't a good polyfill that I know of (that doesn't mutate the object).

@knownasilya
Copy link
Contributor

Maybe instead of deprecating, we just spit out a warning whenever there is an each without a key.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 4, 2015

@knownasilya - How is that much different? People should be supplying a key, deprecation vs warning doesn't seem to make much difference there...

@knownasilya
Copy link
Contributor

Because deprecation means eventual removal, but as in @jayphelps' case there is that case where there is no stable id.

But I guess that's a small use case. Really if you have that, then just use guid, otherwise use a key? I guess I'm convinced 😸

@rwjblue
Copy link
Member Author

rwjblue commented Jun 4, 2015

as in @jayphelps' case there is that case where there is no stable id

In either case key is always used. In his case, you would just use @guid as the key itself like: {{#each thingsArray key="@guid" as |item|}}{{/each}}.

Because deprecation means eventual removal

Completely agreed. I'll review with the rest of the team before deprecation. If we intend to allow keyless usage long term (like when we can have native ES6 Map's and don't need to mutate the object) we should not deprecate.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 7, 2015

I have implemented as warning for now, closing...

@rwjblue rwjblue closed this as completed Jun 7, 2015
@christophermlne
Copy link

i feel like the syntax is looking a bit unfriendly... complicating what should be a simple looping construct.

my opinion only.

i also don't know what "@Index" is or where it's coming from or what the difference is between it and "@Guid"...

@knownasilya
Copy link
Contributor

simple=inefficient. Not sure how key='option' is unfriendly..

@Guid is a generated guid, probably based on the item
@Index is the index of the item, hence it's not efficient, since if you sort your array you loose your key..

@christophermlne
Copy link

if "@Guid" is the new default, why are we required to specify it?

@knownasilya
Copy link
Contributor

@christophermlne it's not the default. Previously it was @Index, but that isn't performant so now you "must" specify a key or you see a warning.

@funtusov
Copy link
Contributor

We must specify one key or another, so if the recommended "fast" option is @Guid. why not make it a default?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 15, 2015

Great point, I submitted a PR for exactly that :)

@ianwdunlop
Copy link

The each helper documentation implies that this key is purely to help the renderer. I shouldn't have to care about that. Convention over configuration etc.....

@funtusov
Copy link
Contributor

@rwjblue fixing code even before someone realize there's a problem with it since: 'xx :)

@gerry3
Copy link

gerry3 commented Jun 17, 2015

FYI, this warning was removed in favor of a default key of @identity by #11461, but that isn't in 1.13.0 & also not in 1.13.1 according to #11461 (comment)

@rwjblue
Copy link
Member Author

rwjblue commented Jun 17, 2015

@gerry3 - Correct, I missed the commit when I released 1.13.1, I will be shipping a 1.13.2 within a day or two.

@gerry3
Copy link

gerry3 commented Jun 17, 2015

Thanks @rwjblue! Just wanted to make sure anyone who came across this thread knew the plan.

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

No branches or pull requests

7 participants