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

[BUGFIX release] Default {{each}} key to @guid or @item based on type. #11461

Merged
merged 1 commit into from Jun 16, 2015

Conversation

@rwjblue
Copy link
Member

commented Jun 15, 2015

This PR uses @item as the key when the item being iterated is a string or number, and uses @guid when it is anything else.

This is an attempt at making a better default, to prevent users from having to specify a key= to each {{each}} invocation.

It is absolutely possible, that we will want to revisit this and make specifying key required in the future, but it seems that we should attempt at good defaults before making that decision.

@rwjblue rwjblue force-pushed the rwjblue:use-default-key branch Jun 15, 2015

@rwjblue rwjblue changed the title [BUGFIX release] Default {{each}} key to @guid or @index based on type. [BUGFIX release] Default {{each}} key to @guid or @item based on type. Jun 15, 2015

@mmun

View changes

packages/ember-htmlbars/lib/utils/decode-each-key.js Outdated
@@ -19,8 +19,13 @@ export default function decodeEachKey(item, keyPath, index) {
if (keyPath) {
key = get(item, keyPath);
} else {
Ember.warn('Using `{{each}}` without specifying a key can lead to unusual behavior. Please specify a `key` that identifies a unique value on each item being iterated. E.g. `{{each model key="@guid" as |item|}}`.');
key = index;
let type = typeOf(item);

This comment has been minimized.

Copy link
@mmun

mmun Jun 15, 2015

Member

Why not just typeof?

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jun 15, 2015

Member

yes, lets just use JS typeof here

@mmun

This comment has been minimized.

Copy link
Member

commented Jun 15, 2015

Looks good 👍

@rwjblue rwjblue force-pushed the rwjblue:use-default-key branch Jun 15, 2015

@KTKate

This comment has been minimized.

Copy link

commented Jun 15, 2015

yay 👍

@jpadilla jpadilla referenced this pull request Jun 15, 2015

@rwjblue rwjblue force-pushed the rwjblue:use-default-key branch Jun 15, 2015

@rwjblue

This comment has been minimized.

Copy link
Member Author

commented Jun 15, 2015

Update:

  • Use normal typeof (instead of Ember.typeOf).
  • Added @detect as a new type (makes it much easier to document the default)
  • Added API docs saying what the default is.
@mixonic

This comment has been minimized.

Copy link
Member

commented Jun 15, 2015

I am +1. I've also heard many WAT about the current behavior

@rwjblue rwjblue force-pushed the rwjblue:use-default-key branch 2 times, most recently Jun 15, 2015

@rwjblue rwjblue referenced this pull request Jun 16, 2015
0 of 2 tasks complete

@rwjblue rwjblue force-pushed the rwjblue:use-default-key branch to 01a7103 Jun 16, 2015

@rwjblue

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2015

Updated (hopefully for the last time) to deprecate @guid and @item in favor of adding the default of @identity.

rwjblue added a commit that referenced this pull request Jun 16, 2015

Merge pull request #11461 from rwjblue/use-default-key
[BUGFIX release] Default {{each}} key to @Guid or @item based on type.

@rwjblue rwjblue merged commit 6d6d9a8 into emberjs:master Jun 16, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rwjblue rwjblue deleted the rwjblue:use-default-key branch Jun 16, 2015

@funtusov

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2015

FYI: It seems that this commit didn't get into the 1.13.1 tag

@sandstrom

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2015

Thanks!! ⛵️

@jmurphyau jmurphyau referenced this pull request Jun 25, 2015

jlblcc referenced this pull request in adiwg/mdEditor Jun 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.