fix(select): make ngOptions support selectAs and trackBy together by jeffbcross · Pull Request #9419 · angular/angular.js

jeffbcross · 2014-10-03T22:58:43Z

This commit implements a function, "getSelected()", that takes the
selectAs expression into account when determining if a particular
option is selected or not. Previously, the selectAs part of the
ngOptions comprehension expression was being ignored.

Fixes #6564

jeffbcross · 2014-10-03T23:01:48Z

@IgorMinar mind taking a look?

IgorMinar · 2014-10-03T23:59:30Z

this is an implementation detail and not a public api contract. if we want to have this in the docs then we should just reword it to state the public api contract first and then mention the implementation detail as an explanation of why option.value and select.value contain weird stuff.

IgorMinar · 2014-10-04T00:19:14Z

otherwise lgtm

jeffbcross · 2014-10-06T19:43:43Z

There are some issues when changing from the view that need some work. See this plnkr

Select "one" and nothing happens. Select "two" and "one" becomes selected (and the model is 1).

If I would decrement both ids, this would work as expected. It seems the logic that assigns the viewValue before rendering needs to also be updated with this change.

caitp · 2014-10-07T16:11:12Z

#9467 may be related to this in some way, I haven't looked into it much --- in any case both sort of revolve around tracking expressions (the bug it tries to fix is not the same issue, but should be related)

jeffbcross · 2014-10-07T17:36:03Z

Fixed some more issues, and improved viewValue computation. I'm going to do some manual testing and add some more unit tests.

jeffbcross · 2014-10-07T21:36:05Z

I added more tests and fixed some more cases. @tbosch mind taking a look?

btford · 2014-10-07T21:37:29Z

shouldn't val be 2 since values[1] is {id: 2, name: 'second'} and we track by id?

The element value is always either the index (for arrays) or key (for objects), even though the model can be different.

This allows models to be more complex objects. I was confused by this behavior as well, which is why I added a note in the docs about the element value.

jeffbcross · 2014-10-07T22:03:58Z

new describe: selectAs with trackBy
Maybe add 2 more tests for object data sources

jeffbcross · 2014-10-07T23:34:52Z

More todos aside from commit comments:

Make sure new tests aren't redundant
Add a protractor test or two to simulate E2E experience

This commit implements two functions, "isSelected()" and "getViewValue()" to properly compute an option's selected state and the model controller's viewValue respectively. These functions give proper precedence to "track by" and "select as" parts of the ngOptions comprehension expression, which were previously inconsistent and incompatible. Fixes angular#6564

trackBy and selectAs have never worked together, and are fundamentally incompatible since model changes cannot deterministically be reflected back to the view. This change throws an error to help developers better understand this scenario.

tbosch · 2014-10-08T19:01:45Z

Replace , e.g. with :

tbosch · 2014-10-08T19:10:36Z

Otherwise LGTM.

tbosch · 2014-10-08T19:11:37Z

Travis failed because of a jshint error...

   test/ng/directive/selectSpec.js
    779 |        }).toThrowMinErr('ngOptions', 'trkslct', "Comprehension expression cannot contain both selectAs ('item.id') and trackBy ('item.id') expressions.")

jeffbcross · 2014-10-08T20:16:13Z

Closed at 30996f8

jeffbcross added the In Progress label Oct 3, 2014

jeffbcross added this to the 1.3.0-rc.5 milestone Oct 3, 2014

jeffbcross added origin: google type: bug needs: review and removed origin: google labels Oct 3, 2014

jeffbcross assigned IgorMinar Oct 3, 2014

mary-poppins added cla: yes and removed cla: no labels Oct 3, 2014

IgorMinar reviewed Oct 3, 2014
View reviewed changes

btford force-pushed the master branch from 9011a65 to 46db47b Compare October 6, 2014 22:54

caitp mentioned this pull request Oct 7, 2014

Passing in an a function to the ngOptions array clause that generates elements on the fly results in infinite $digest loop #9464

Closed

jeffbcross force-pushed the issue-6564 branch from be66e74 to af50253 Compare October 7, 2014 17:31

jeffbcross force-pushed the issue-6564 branch from af50253 to 35da770 Compare October 7, 2014 21:10

btford reviewed Oct 7, 2014
View reviewed changes

jeffbcross reviewed Oct 7, 2014
View reviewed changes

jeffbcross force-pushed the issue-6564 branch from 35da770 to 9b4e0c9 Compare October 8, 2014 06:03

tbosch and others added 2 commits October 8, 2014 11:38

refactor(select): reduce duplication and reorder functions

a68223b

fix(select): throw for selectAs and trackBy

6e8bf21

trackBy and selectAs have never worked together, and are fundamentally incompatible since model changes cannot deterministically be reflected back to the view. This change throws an error to help developers better understand this scenario.

jeffbcross force-pushed the issue-6564 branch from 9bdb214 to 6e8bf21 Compare October 8, 2014 18:41

tbosch reviewed Oct 8, 2014
View reviewed changes

Comment thread docs/content/error/ngOptions/trkslct.ngdoc

tbosch Oct 8, 2014

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace , e.g. with :

jeffbcross force-pushed the master branch 2 times, most recently from abdaab7 to 30996f8 Compare October 8, 2014 19:47

jeffbcross closed this Oct 8, 2014

btford removed the In Progress label Oct 8, 2014

Conversation

jeffbcross commented Oct 3, 2014

Uh oh!

jeffbcross commented Oct 3, 2014

Uh oh!

IgorMinar Oct 3, 2014

Choose a reason for hiding this comment

Uh oh!

IgorMinar commented Oct 4, 2014

Uh oh!

jeffbcross commented Oct 6, 2014

Uh oh!

caitp commented Oct 7, 2014

Uh oh!

jeffbcross commented Oct 7, 2014

Uh oh!

jeffbcross commented Oct 7, 2014

Uh oh!

btford Oct 7, 2014

Choose a reason for hiding this comment

Uh oh!

jeffbcross Oct 7, 2014

Choose a reason for hiding this comment

Uh oh!

jeffbcross Oct 7, 2014

Choose a reason for hiding this comment

Uh oh!

jeffbcross Oct 7, 2014

Choose a reason for hiding this comment

Uh oh!

jeffbcross Oct 7, 2014

Choose a reason for hiding this comment

Uh oh!

jeffbcross commented Oct 7, 2014

Uh oh!

tbosch Oct 8, 2014

Choose a reason for hiding this comment

Uh oh!

tbosch commented Oct 8, 2014

Uh oh!

tbosch commented Oct 8, 2014

Uh oh!

jeffbcross commented Oct 8, 2014

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants