Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

ng-options track by and select as are not compatible #6564

Closed
damiengenet opened this issue Mar 6, 2014 · 39 comments
Closed

ng-options track by and select as are not compatible #6564

damiengenet opened this issue Mar 6, 2014 · 39 comments

Comments

@damiengenet
Copy link

When both a track by expression and a select as expression are used in ng-options, binding doesn't work anymore.

For instance:

      it('should bind to scope value through experession while tracking/identifying objects', function() {
        createSelect({
          'ng-model': 'selected',
          'ng-options': 'item.id as item.name for item in values track by item.id'
        });

        scope.$apply(function() {
          scope.values = [{id: 10, name: 'A'}, {id: 20, name: 'B'}];
          scope.selected = scope.values[0].id;
        });

        expect(element.val()).toEqual('10');

        scope.$apply(function() {
          scope.selected = scope.values[1].id;
        });

        expect(element.val()).toEqual('20');
      });

It seems that in the ngOptions directive, the track function always directly use the value expression even if a select as expression is set.

@janv
Copy link

janv commented May 13, 2014

Oops, accidentally deleted my previous comment.
I created/modified a fiddle to illustrate the problem:
http://jsfiddle.net/qWzTb/418/

@SQiShER
Copy link

SQiShER commented May 13, 2014

+1 I'm having the same problem.

@janv
Copy link

janv commented May 13, 2014

It's annoying but you can really just leave away the track by and it'll work.

@ahoereth
Copy link

Can confirm this problem still exists as of v1.3.0-beta.15.

@wojciechfornal
Copy link
Contributor

Having the same problem with 1.2.20 :/

@mvalim
Copy link

mvalim commented Aug 6, 2014

Same here

janv added a commit to janv/angular.js that referenced this issue Aug 8, 2014
@janv
Copy link

janv commented Aug 8, 2014

I've been encountering this again.
The problem was created in c32a859 by @quazzie:

For checking if an option is selected, the trackFn is used, where the valueFn should be used instead. I'll try to come up with a fix.

@janv
Copy link

janv commented Aug 8, 2014

Ok, I have investigated this. It's not actually a bug, just misleading documentation.
The thing is, you just can't combine value as label for collection with track by.
You have to pick the one or the other. There are two different Use Cases:

1. You want a shiny presentation for an ugly value

Use value as label:

items = [
  {value: 1, label: 'One'},
  {value: 2, label: 'Two'},
]
template = 'ng-options="item.value as item.label for items"'

When you change the dropdown, the model is assigned whatever you define in front of the as.

2. You want to iterate over complex objects

Use track by:

items = [
  {id: 1, ...},
  {id: 2, ...},
  {id: 3, ...},
]
template = 'ng-options="item as item.label for items track by items.id"'

When you change the dropdown, the model is assigned the item. The track by expression is only used to associate options with items through the value attribute on the <option>.

Illustrated in http://jsfiddle.net/0vpsv1wa/1/

janv added a commit to janv/angular.js that referenced this issue Aug 8, 2014
janv added a commit to janv/angular.js that referenced this issue Aug 9, 2014
ngOptions introduced `track by` in c32a859.
Using `track by` puts constraints on the value you can use in the
interpolation expression in ngOptions.
This patch both documents this and adds an exception if you use
ngOptions in an unsupported way.

Closes angular#6564
janv added a commit to janv/angular.js that referenced this issue Aug 9, 2014
ngOptions introduced `track by` in c32a859.
Using `track by` puts constraints on the value you can use in the interpolation
expression in ngOptions. This patch both documents this and adds an exception
if you use ngOptions in an unsupported way.

Closes angular#6564
@jsanta
Copy link

jsanta commented Aug 20, 2014

Faced the same problem, and figured a workaround, but I don't know if this is going to solve future issues (at least in the project I'm doing this).

Somewhere on your code add this ugly script.

$(document).on('change', 'select', function(e){        
        var scope = angular.element($(this)).scope();
        var val   = scope[$(this).attr('name')];   
        $(this).val(val);
});

@btford btford removed the gh: issue label Aug 20, 2014
janv added a commit to janv/angular.js that referenced this issue Sep 26, 2014
ngOptions introduced `track by` in c32a859.
Using `track by` puts constraints on the value you can use in the interpolation
expression in ngOptions. This patch documents this.

Closes angular#6564
@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.5, Backlog Sep 29, 2014
@jeffbcross jeffbcross assigned btford and jeffbcross and unassigned btford Sep 29, 2014
@jeffbcross
Copy link
Contributor

Interestingly, if I remove track-by from the original test, the expectations still fail:

'ng-options': 'item.id as item.name for item in values'

Expected '0' to equal '10
Expected '1' to equal '20'

@gkalpak
Copy link
Member

gkalpak commented Oct 1, 2014

@jeffbcross: This is "expected", since with ngOptions the select will use the option's index as value (for the <option> element) and not for example item.id.
This is understandable, since the modelValue could be an object.

The test should rather be:

it('should bind to scope value through experession while tracking/identifying objects', function() {
  createSelect({
    'ng-model': 'selected',
    'ng-options': 'item.id as item.name for item in values track by item.id'
  });

  var selectedIdx;

  scope.$apply(function() {
    scope.values = [{id: 10, name: 'A'}, {id: 20, name: 'B'}];
    selectedIdx = 0;
    scope.selected = scope.values[selectedIdx].id;
  });

  expect(element.val()).toEqual('' + selectedIdx);

  scope.$apply(function() {
    selectedIdx = 1;
    scope.selected = scope.values[selectedIdx].id;
  });

  expect(element.val()).toEqual('' + selectedIdx);
});

(And removing track by ... makes it pass as expected.)

@greglockwood
Copy link

Hi @tbosch,

I have constructed a Plunkr example that illustrates the main place I was relying on both select as and track by to make things easier.

Essentially, I have an array of options that contains properties that are used for the group by label, and a value property which is the object actually bound as the model value.

I would then have a track by expression that would work for both the subobject and the option in the array.

So the example you cited could be fixed to use this technique to have:
ng-options="item.subItem as item.label for item in values track by (item.subitem.name || item.name)".
This should then work correctly.

Is this not a valid use case?

bullgare pushed a commit to bullgare/angular.js that referenced this issue Oct 9, 2014
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
@jeffbcross
Copy link
Contributor

@greglockwood I forked the plnkr and changed the expression to work with rc.5. I just changed ng-model to to be data and removed the selectAs. I can imagine use cases of shared models where this could cause some inconvenience though.

<!--Before-->
<select ng-model="data.value" ng-options="opt.value as opt.display group by opt.groupName for opt in options track by (opt.id || opt.value.id)"></select>
<!--After-->
<select ng-model="data" ng-options="opt.display group by opt.groupName for opt in options track by (opt.id || opt.value.id)"></select>

Your track by expression is one that was select as-aware and would actually work. Unfortunately, there's no way we can know at runtime if the track expression is compatible with model and option value. It's worth considering something less restrictive than a thrown error, like a "I know what I'm doing" opt-out, or just logging a warning. @tbosch what do you think?

@jeffbcross
Copy link
Contributor

Reopening so we can give this some more consideration. After some discussion with @tbosch and @IgorMinar, here are some proposed approaches to allowing track by and select as to work together:

  • BREAKING CHANGE: Set evaluation context for track-by to value that is set to ng-model. In the example two comments above, track by would be "name", and would evaluate against the subItems, since this is what was set to the model via select as.
  • Add a magic $selectedValue object, which would be the encouraged variable to use in track by expressions. We could deprecate accessing item and log a warning when it is accessed, and in a future version we could change this warning to throw an error.
  • Leave as is

@jeffbcross jeffbcross reopened this Oct 9, 2014
jeffbcross added a commit to jeffbcross/angular.js that referenced this issue Oct 9, 2014
Instead of throwing an error when using "track by" and "select as" expressions,
ngOptions will assume that the track by expression is valid, and will use it to
compare values.

Closes angular#6564
@mgol mgol modified the milestones: 1.3.0-rc.6, 1.3.0-rc.5 Oct 9, 2014
@mgol
Copy link
Member

mgol commented Oct 9, 2014

Changing the milestone since 1.3.0-rc.5 is already released.

@jeffbcross
Copy link
Contributor

We ended up deciding to just make the old behavior possible. Working on a PR

@greglockwood
Copy link

@jeffbcross This is a tricky one. Whilst I am grateful for undoing the change to make the old behaviour possible, I agree it was confusing. In particular, that you have to construct an expression for the track by that works for both types of context is not obvious, and was a pain point for me when developing that part of the application I am working on.

In response to your fork of my plnkr, I offer you a counter-fork which demonstrates something closer to my actual use case, in that it is indeed using a "shared model". In such a case, it is not valid to overwrite the parent object with the complete option selected, as the dbObject and each opt share a common structure only in terms of the value property.

As for your proposed solutions, I don't think either of them is the perfect solution, as they both have their downsides.

The first is probably the better of the two, though. Whilst it is a breaking change, it is not a huge deal to cope with, as most people would just find themselves deleting <identifier>. from their track by expressions to adapt.

My main misgiving about this change is that the context for the track by expression would be different to the context for the majority of the other subexpressions in the greater ng-options expression.

Here is a graphic to illustrate what I mean:

image

As you can see, there would be 4 different contexts within a single ng-options expression with this change:

  1. the current scope for the <select>, shown in blue
  2. the context of what is effectively a child scope of the select's scope, with a single property on it for each option, shown in red
  3. the ng-model expression value, shown in green
  4. each option in the array, shown in green

In particular, that the track by expression is evaluated against two different contexts is reasonably without precedent AFAIK, and potentially confusing.

The cons of the second solution, introducing a $selectedValue magic value for use in track by, include:

  • Having to remember exactly what it is called. "Is it $selectValue, or was it $selectedValue, or was it just $selected or $value? Darnit, let me check the docs again!"
  • Inconsistency with the way track by works with ng-repeat (this con is shared by the first solution too).
  • I cannot see a valid use case where you would not need to use $selectedValue in your track by expression, which effectively makes it redundant.
  • Lack of backwards compatibility (unless I am not fully understanding something correctly)

The biggest thing in its favour is that it is an explicit context, which is potentially less confusing for people and it is easier to document and explain it as a special "double purpose" context. It would also be fairly easy to migrate to.

Anyways, those are just my thoughts (with input from a talented co-worker).

jeffbcross added a commit to jeffbcross/angular.js that referenced this issue Oct 10, 2014
Instead of throwing an error when using "track by" and "select as" expressions,
ngOptions will assume that the track by expression is valid, and will use it to
compare values.

Closes angular#6564
@jeffbcross
Copy link
Contributor

Thanks for bringing the issue up, and thanks for the feedback on the approaches. This is a tough problem without a clean solution that satisfies all constraints, is consistent with ng-repeat, and is backwards-compatible. In the meantime, I removed the thrown error, added tests for how track by and select as should work together, and fixed the implementation.

Unfortunately, the originally reported issue would not work with the expression/model as is, but could work with some re-working.

@jonathansolorzn
Copy link

This is still happening in v1.4.7, no fix yet?

@mariusstaicu
Copy link

same problem here!!

@agvalente
Copy link

Same problem!!

@DanieleSassoli
Copy link

same thing here with in v1.4.7

@dlazzarino
Copy link

Same thing with v1.4.8

@aj07mm
Copy link

aj07mm commented May 5, 2016

Same thing with v1.5.3

@hebergentilin
Copy link

hebergentilin commented May 10, 2016

The ng-options works fine, but when update the model manual, the select value hadn't update.

<select ng-model="shiny"
                ng-options="opt.value as opt.label for opt in options">
            </select>

$scope.complex = 2;

See the http://jsfiddle.net/0vpsv1wa/110/

@gkalpak
Copy link
Member

gkalpak commented May 13, 2016

There are certain (documented) limitations in using track by together with select ... as.
Not all usecases are supported.

@mgsstms
Copy link

mgsstms commented Jan 29, 2017

ng-options="item.id as item.name for item in values track by item.id" works, but doesn't se the default value
http://plnkr.co/edit/6YdeJviPnCwY3Em9ltyo?p=preview

@gkalpak
Copy link
Member

gkalpak commented Jan 29, 2017

No, it doesn't work (i.e. track by has no effect).

@kash-pram
Copy link

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.