[REGRESSION] In 1.13.2 `{{#each}}` fails with duplicate values #11549

seanpdoyle · 2015-06-24T21:46:58Z

http://emberjs.jsbin.com/hedunosumi/edit?html,js,console,output

Might be a duplicate of #11504

has #11525 landed in 1.13.2 yet?

The text was updated successfully, but these errors were encountered:

seanpdoyle · 2015-06-24T21:48:49Z

ping: @rwjblue @cibernox @toranb

stefanpenner · 2015-06-24T21:53:34Z

Seems like a duplicate

rwjblue · 2015-06-24T23:00:30Z

#11525 added a 'useful' error (the error previously was super obtuse if you take a look at the same JSBin for 1.13.1). It does not fix the issue with duplicates in general.

In theory we could continue to add prefixes (like the index and/or a guid) to the key until we get a unique value (therefore avoiding a collision), but that feels like a kludge to me. I could easily submit a PR for this if others think it would be useful in 1.13.x.

The proper path forward is understood by @krisselden and @wycats but it is not completely clear to me.

seanpdoyle · 2015-06-25T00:20:06Z

Duplicate values in a list is common enough to require a 1.13.x fix, isn't it?

rwjblue · 2015-06-25T00:46:57Z

@seanpdoyle - I am unsure, this is the first issue for it 😺.

Also, my "fix" is mostly a hack (just continue fiddling with the key until it is unique), but the real fix upstream is almost certainly more involved and will require too many changes for us to be comfortable landing in 1.13.

seanpdoyle · 2015-06-25T02:17:58Z

@rwjblue I tried to get a failing test before submitting this issue, but I couldn't.

Which test would good to copy and alter? I've looked in packages/ember-htmlbars/tests/helpers/each_test.js but this module has [DEPRECATED] in the title.

rwjblue · 2015-06-25T02:25:52Z

Failing tests are here:

ember.js/packages/ember-htmlbars/tests/helpers/each_test.js

Lines 1305 to 1337 in 6130fa6

    
           QUnit.test('duplicate keys trigger a useful error (temporary until we can deal with this properly in HTMLBars)', function() { 
        
             runDestroy(view); 
        
             view = EmberView.create({ 
        
               items: ['a', 'a', 'a'], 
        
               template: compile('{{#each view.items as |item|}}{{item}}{{/each}}') 
        
             }); 
        
             throws( 
        
               function() { 
        
                 runAppend(view); 
        
               }, 
        
               `Duplicate key found ('a') for '{{each}}' helper, please use a unique key or switch to '{{#each model key="@index"}}{{/each}}'.` 
        
             ); 
        
           }); 
        
           QUnit.test('pushing a new duplicate key will trigger a useful error (temporary until we can deal with this properly in HTMLBars)', function() { 
        
             runDestroy(view); 
        
             view = EmberView.create({ 
        
               items: A(['a', 'b', 'c']), 
        
               template: compile('{{#each view.items as |item|}}{{item}}{{/each}}') 
        
             }); 
        
             runAppend(view); 
        
             throws( 
        
               function() { 
        
                 run(function() { 
        
                   view.get('items').pushObject('a'); 
        
                 }); 
        
               }, 
        
               `Duplicate key found ('a') for '{{each}}' helper, please use a unique key or switch to '{{#each model key="@index"}}{{/each}}'.` 
        
             ); 
        
           });

Those tests should both be changed from throws to asserting the proper output

sly7-7 · 2015-06-29T15:09:11Z

I'm encountering this as well during 1.11 to 1.13 migration. Just to be sure, is using key=@index or key=@guid good workarounds for now ?
Also, it seems like 1.13.2 from ember-source gem is not up to date with the useful warning

pixelhandler · 2015-07-21T21:16:40Z

@stefanpenner this is preventing me from updating from 1.12.1 to 1.13.5.

I'm getting the same error Error: Duplicate key found ('ember850') for '{{each}}' helper, please use a unique key or switch to '{{#each model key="@index"}}{{/each}}' when upgrading from 1.12.1 to 1.13.5

stefanpenner · 2015-07-21T21:17:08Z

@pixelhandler does the advice in the error help you?

stefanpenner · 2015-07-21T21:18:06Z

I believe the plan to make this transparent may or may not be backported, as it has not yet been implemented. @krisselden or @mmun can likely speak more clearly.

pixelhandler · 2015-07-21T21:18:25Z

I tried it but still had the same error. I tried in my smallest app, so maybe need to do that everwhere in my core library and all 3rd party code too. Seems like the deprecation caused a breaking error

stefanpenner · 2015-07-21T21:19:06Z

Seems like the deprecation cased a breaking error

That isn't a deprecation

I tried in my smallest app

can you share this repo? or demonstrate in a jsbin. Im sure whoever looks at this would be interested in seeing.

pixelhandler · 2015-07-21T21:19:47Z

@stefanpenner so API change to #each ? oh perhaps just an error. I swear I saw that as deprecated around 1.13.x early on. Oh well, everything broke for me. Guess I would've caught it earlier if was able to run beta. (if I could've kept up with the churn in the large codebase we have.)

krisselden · 2015-07-21T21:23:52Z

@pixelhandler the plan is to make it transparent with binning like in a hash map collision

stefanpenner · 2015-07-21T21:24:31Z

This should be made transparent for the 1.13.x, as it is clearly a breaking change.

rwjblue · 2015-07-21T21:27:46Z

The error suggests using the following in the case of a duplicate:

{{#each things key="@index"}}
  {{! your stuff here }}
{{/each}}

pixelhandler · 2015-07-21T21:32:29Z

@rwjblue so confirm, a breaking change in 1.13.x, must NOT use duplicates when iterating over an array using #each. And the fix is to do as the error suggests use key="@index" (or guid).

Can that be added to the blog post as a breaking change (we broke your apps, lol) ? http://emberjs.com/blog/2015/06/12/ember-1-13-0-released.html

rwjblue · 2015-07-21T21:36:12Z

@pixelhandler - Feel free to PR an update to the blog post, I would love to get this fixed for real in a point release of 1.13, but I do not have the expertise required to fix it.

stefanpenner · 2015-07-21T21:37:46Z

@rwjblue so confirm, a breaking change in 1.13.x, must NOT use duplicates when iterating over an array using #each. And the fix is to do as the error suggests use key="@index" (or guid).

@pixelhandler IMHO this is a bug. Its just time consuming to fix, I hope @wycats or someone has time fix.

It might be worth calling out, as a known bug, the will hopefully be fixed.

pixelhandler · 2015-07-21T21:38:50Z

@krisselden @rwjblue @stefanpenner - To handle the error why not just create a unique id on the fly?

@stefanpenner no the company I work for values privacy, can't share the app. That's why I tinker with my blog project so I do have code to share. In that project I was able to add the key as suggested, it's a tiny app. Some background is that we have a core library with a few other 3rd party libs as well. The core lib is imported as a gem and all the apps require the same template compiler now (since htmlbars) so deps are tightly coupled. The templates are compiled in the core lib and the apps using asset pipeline so my guess is that some things aren't getting unique ids? but can't repro in a jsbin.

stefanpenner · 2015-07-21T21:39:34Z

@stefanpenner no the company I work for values privacy, can't share the app.

a reproduction does not need to be a full app, a JSBIN or isolated repo would be best. I honestly don't want to sift through someones app unless it has to be.

pixelhandler · 2015-07-21T21:41:08Z

To me it seems better to generate a unique id on the fly instead of throwing an error, maybe it's the inline compiler I don't know. Prolly the best use of time is to deal with the error and try to use the key everywhere.

stefanpenner · 2015-07-21T21:42:56Z

To me it seems better to generate a unique id on the fly instead of throwing an error, maybe it's the inline compiler I don't know.

this isn't related to the compiler. It is a runtime concern, the list-dffing takes into account some key to identify how the list changes over time.

pixelhandler · 2015-07-21T21:44:08Z

@stefanpenner @rwjblue I knew our apps would find a regression. how much test coverage does ember have?

stefanpenner · 2015-07-21T21:45:30Z

@stefanpenner @rwjblue I knew our apps would find a regression. how much test coverage does ember have?

11,000 tests

I believe this was a known regression, I'm not sure why it was deemed tolerable, but that choice wasn't made by me.

krisselden · 2015-07-21T21:45:56Z

@pixelhandler the generated id may collide with a future key, if based on the dup, may collide with a future key.

rwjblue · 2015-07-21T21:59:18Z

From #11549 (comment) (above):

#11525 added a 'useful' error (the error previously was super obtuse if you take a look at the same JSBin for 1.13.1). It does not fix the issue with duplicates in general.

stefanpenner · 2015-07-21T22:02:28Z

@pixelhandler there already is a failing example, the provided JSBIN offers it.

Everyone is aware of this issue, solving it is just a function of available time.

pixelhandler · 2015-07-21T22:04:36Z

@stefanpenner I offer my Friday all day @rwjblue and you can tell me what do fix do, etc. I have time. I think the question is of priority not time, we all have the same 24 hrs.

stefanpenner · 2015-07-21T22:06:31Z

@pixelhandler #11549 (comment)

It's worth noting, this issue is already well known, and well fleshed out. Beating on it isn't going to make it happen any faster.

Please note, It is very important that unreported issues are reported. For existing issues, it is valuable to add missing information, to ensure the issue is fully formed. Additionally commentary provides diminishing returns.

Not to offend, but if the problem solution was "just tell me what to do" easy, I suspect @rwjblue would have landed it 22 days ago. He is a machine after-all.

You may consider exploring some of the suggestions kris gave, and seeing if you can apply them yourself.

stefanpenner · 2015-07-21T22:15:49Z

@pixelhandler you seem very motivated capably and eager to help, lets plan for a hangout friday morning. I would love to harness some of your availability and time.

raytiley · 2015-07-21T22:15:49Z

Could #11856 work for this?

pixelhandler · 2015-07-21T22:16:23Z

@stefanpenner just spoke with @mmun in slack he mentioned maybe looking into tonight.

stefanpenner · 2015-07-21T22:17:01Z

@raytiley that looks similar to a bucketing approach @krisselden was talking about. We should get him to weigh in, to be sure this exact approach is what he was thinking about. Or if it was more elaborate. (or different entirely)

It would be lovely if the adjustment was that.

pixelhandler · 2015-07-21T22:19:05Z

@stefanpenner sounds good, I'll send you an invite for Fri morning. I like taking larger blocks of time for offering help. During the week I have football with the kids meetings with teams at work, Meetup w/ Ember at night, a wife, 5 kids and 94 yr old grandpa at the house so I understand juggling priority. Ember.js is my 1st priority on Friday, all day (unless we have a server burnt with fire at work).

stefanpenner · 2015-07-21T22:20:12Z

Ember.js is my 1st priority on Friday, all day (unless we have server burnt with fire at work).

Understood, i'll be mindful of that. I suspect once everyone motivations/costs/availiabity is hashed out we can work in harmony. Smooth out these few upgrade pains and make the world a better place. And on saturdays we can go surfing

krisselden · 2015-07-21T22:32:24Z

@stefanpenner my suggestion though is to do it inside yieldItem, in the current map use binning, this is duplicating already a seen that is also inside yieldItem.

krisselden · 2015-07-21T22:34:38Z

seen: https://github.com/tildeio/htmlbars/blob/05226993a4e75b9827dc9abec1ebc8f21031311f/packages/htmlbars-runtime/lib/hooks.js#L227

map that needs binning https://github.com/tildeio/htmlbars/blob/05226993a4e75b9827dc9abec1ebc8f21031311f/packages/htmlbars-runtime/lib/hooks.js#L221

pixelhandler · 2015-07-22T00:02:11Z

@stefanpenner for us we tried the work around with @index as the key and that didn't work for us, oh well.

pixelhandler · 2015-07-22T00:26:02Z

@stefanpenner yeah it wasn't a deprecation is was a warning but that was removed here https://github.com/emberjs/ember.js/releases/tag/v1.13.2 https://github.com/emberjs/ember.js/pull/11461/files

rwjblue · 2015-07-22T00:55:32Z

Those are unrelated concerns. The warning in versions prior to 1.13.2 was for when no key was specified. During 1.13.0/1 you would have received a very hairy error message from deep inside htmlbars.

The error that instructs you how to recover was added in 1.13.3 (from #11525) and is intended as a stop gap measure to provide at least a small amount of insight into the cause of the error that was happening in 1.13.0/1/2.

stefanpenner · 2015-07-22T14:57:30Z

Wooohoo. The magic of @mmun

pixelhandler · 2015-07-22T18:18:56Z

@rwjblue, @stefanpenner @mmun - Thanks for fixing the regression! So will this be in a patch release (v1.13.6) soon?

rwjblue · 2015-07-22T18:24:49Z

Yes, it will be in the next patch release. "soon" is relative though 👯

pixelhandler · 2015-07-23T19:35:41Z

@rwjblue just curious for those of us with broken apps and blocked from upgrading to 1.13.x why wait for a patch? What's the workaround use master branch? or just wait?

stefanpenner · 2015-07-23T20:04:01Z

@pixelhandler building ember is documented here: https://github.com/emberjs/ember.js/#building-emberjs.

I am unsure if @rwjblue has had a chance to backport the fix to the stable branch (which is 1.13.x) but if he has you can likely just build stable. In theory, builds.emberjs.com has every commit built and published, but currently it can be tricky to find the version you want. (would be nice if someone could help here)

krisselden · 2015-07-23T21:14:09Z

@stefanpenner if it was backported to stable, it would be published to the release channel http://emberjs.com/builds/#/release

stefanpenner · 2015-07-23T21:22:52Z

@stefanpenner if it was backported to stable, it would be published to the release channel http://emberjs.com/builds/#/release

ah, even unreleased but merely backported commits, Correct?

pixelhandler · 2015-07-27T16:45:44Z

@rwjblue I’m curious… the release version of ember on the website has @version 1.13.5+4eb55108 in the source (ember.debug.js) and the release page says updated 2 days ago, the tag in the repo for 1.3.5 (95d42b8) was made 8 days ago.

What does that mean? Is there non-patch release code in the release?

So, I guess since this issue blocks me from upgrading to 1.13.5 I need to build from source? and manage my own fork (if there is something in the future that is back ported)?

Can someone tell me when I can expect an official release on the 1.13.x series that has this regression patched?

I'm really happy it's fixed but extremely bummed that it's not released. Am I missing something? I guess I was under the impression that a patch release was warranted from this regression (breaking 1.x) in 1.13

stefanpenner · 2015-07-27T16:53:57Z

the big buttons (download 1.13.5) are for the tagged release
the list of builds bellow are (to quote the website)

The builds listed below are incremental improvements made since 1.13.5 and may become 1.13.6.

TL;DR you fix should be in the list of files bellow, as they are on the way to becoming 1.13.6

rwjblue · 2015-07-27T16:54:29Z

@pixelhandler:

the release version of ember on the website has @Version 1.13.5+4eb55108 in the source (ember.debug.js) and the release page says updated 2 days ago, the tag in the repo for 1.3.5 (95d42b8) was made 8 days ago.

What does that mean? Is there non-patch release code in the release?

Every commit to stable, beta, and master branches are published to both S3 and bower. You can use those builds at any time, no need to wait for a tagged release.

The builds page (as you discovered) shows the asset paths for these, feel free to use them (they are quite literally what will become the next tagged version). The release channel builds are generally always more up to date than the last tag.

So, I guess since this issue blocks me from upgrading to 1.13.5 I need to build from source? and manage my own fork (if there is something in the future that is back ported)?

Nope. The builds that you have already discovered include the patch you want. Feel free to use them 😉.

Can someone tell me when I can expect an official release on the 1.13.x series that has this regression patched?

Ember 1.13.6 will be the version that includes this regression fix. It will be released when it is done.

I'm really happy it's fixed but extremely bummed that it's not released.

It is available to use via the channel that you have already checked!

stefanpenner · 2015-07-27T16:55:36Z

We may have a UX issue here on the builds place (i was also confused), if someone is interested in pursuing it likely a discussion on the website repo is appropriate.

pixelhandler · 2015-07-27T17:14:51Z

@rwjblue ah that makes sense now our team did need clarity on…

The builds listed below are incremental improvements made since 1.13.5 and may become 1.13.6

Thanks we'll get started with the latest 'release' channel update. :)

stefanpenner · 2015-07-27T17:28:15Z

Thanks we'll get started with the latest 'release' channel update. :)

awesome, keep us posted!

mmun added the Regression label Jun 26, 2015

jakesjews mentioned this issue Jul 9, 2015

Ember array not properly updating in template #11701

Closed

stefanpenner assigned wycats Jul 21, 2015

rwjblue mentioned this issue Jul 22, 2015

[BUGFIX release] Update HTMLBars to allow duplicate {{each}} keys. #11861

Merged

rwjblue closed this as completed in #11861 Jul 22, 2015

[REGRESSION] In 1.13.2 {{#each}} fails with duplicate values #11549

[REGRESSION] In 1.13.2 {{#each}} fails with duplicate values #11549

Comments

seanpdoyle commented Jun 24, 2015

seanpdoyle commented Jun 24, 2015

stefanpenner commented Jun 24, 2015

rwjblue commented Jun 24, 2015

seanpdoyle commented Jun 25, 2015

rwjblue commented Jun 25, 2015

seanpdoyle commented Jun 25, 2015

rwjblue commented Jun 25, 2015

sly7-7 commented Jun 29, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

krisselden commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

rwjblue commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

rwjblue commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

krisselden commented Jul 21, 2015

rwjblue commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

raytiley commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

pixelhandler commented Jul 21, 2015

stefanpenner commented Jul 21, 2015

krisselden commented Jul 21, 2015

krisselden commented Jul 21, 2015

pixelhandler commented Jul 22, 2015

pixelhandler commented Jul 22, 2015

rwjblue commented Jul 22, 2015

stefanpenner commented Jul 22, 2015

pixelhandler commented Jul 22, 2015

rwjblue commented Jul 22, 2015

pixelhandler commented Jul 23, 2015

stefanpenner commented Jul 23, 2015

krisselden commented Jul 23, 2015

stefanpenner commented Jul 23, 2015

pixelhandler commented Jul 27, 2015

stefanpenner commented Jul 27, 2015

rwjblue commented Jul 27, 2015

stefanpenner commented Jul 27, 2015

pixelhandler commented Jul 27, 2015

stefanpenner commented Jul 27, 2015

[REGRESSION] In 1.13.2 `{{#each}}` fails with duplicate values #11549

[REGRESSION] In 1.13.2 `{{#each}}` fails with duplicate values #11549