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

perf(ngOptions): only perform deep equality check on ngModel if using track by #11448

Closed
wants to merge 1 commit into from
Closed

perf(ngOptions): only perform deep equality check on ngModel if using track by #11448

wants to merge 1 commit into from

Conversation

@booleanbetrayal
Copy link
Contributor

@booleanbetrayal booleanbetrayal commented Mar 27, 2015

Closes #11447

@@ -1657,7 +1701,7 @@ describe('ngOptions', function() {
scope.values.pop();
});

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

This comment has been minimized.

@booleanbetrayal

booleanbetrayal Mar 27, 2015
Author Contributor

Wish I had a better understanding of unknownValue usage in select + options. Had to revert the spec back to its form in ef894c8.

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 30, 2015
Member

OK, so the reason for this is that, if the model is null then element.val() is empty but if the model contains a value that doesn't match any option then element.val() is '?'.

When we remove the option the value becomes invalid so we get '?' but then the model is cleared out and becomes null. If we have a watch on the model then this will cause the ngModel to run $render, which update the element value to ''.

I think that what we need is to trigger a new call to $render when the selected option is removed...

This comment has been minimized.

@petebacondarwin

petebacondarwin Mar 30, 2015
Member

Actually what we need to do is fix the bit of code inside updateOptions (https://github.com/angular/angular.js/pull/11448/files#diff-0a9e86a58b2663074f51454f7a18a0efR646) that checks if the model has changed

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Mar 27, 2015

I'll take a better look over the weekend but I think this is good.

@booleanbetrayal
Copy link
Contributor Author

@booleanbetrayal booleanbetrayal commented Mar 27, 2015

👍

* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/).
* **Note:** By default, `ngModel` compares by reference, not value. This is important when binding to an
* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/). When using `track by`
* in an `ngOptions` expression, however, equality checks will be performed.

This comment has been minimized.

@gkalpak

gkalpak Mar 28, 2015
Member

I might add deep (equality) or something, to make it more clear.
(Technically, even in the default case, it is checking for equality (by reference).)

This comment has been minimized.

@booleanbetrayal

booleanbetrayal Mar 28, 2015
Author Contributor

good point! updated in rebased commit below.

* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/).
* **Note:** By default, `ngModel` compares by reference, not value. This is important when binding to an
* array of objects. See an example [in this jsfiddle](http://jsfiddle.net/qWzTb/). When using `track by`
* in an `ngOptions` expression, however, equality checks will be performed.

This comment has been minimized.

@gkalpak

gkalpak Mar 28, 2015
Member

Same as above.

This comment has been minimized.

@booleanbetrayal

booleanbetrayal Mar 28, 2015
Author Contributor

Same as above. =]

@Narretz
Copy link
Contributor

@Narretz Narretz commented Mar 28, 2015

There's a also regression with circular references in models which was introduced by 6a03ca2 here: #11372

@barillax
Copy link

@barillax barillax commented Mar 30, 2015

👍

1 similar comment
@nbrustein
Copy link

@nbrustein nbrustein commented Mar 30, 2015

👍

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented Mar 30, 2015

Thanks @booleanbetrayal for this

@booleanbetrayal
Copy link
Contributor Author

@booleanbetrayal booleanbetrayal commented Mar 30, 2015

np ... thanks @petebacondarwin !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants