refact(ngOptions): major refactoring... #10639

petebacondarwin · 2015-01-04T14:30:47Z

Major reworking of select and ngOptions:

The SelectController is now used as an abstraction for the select and ngOptions directives
to override to get their desired behaviour
The select directive is completely oblivious to the ngOptions directive now - the ngOptions
directive could be deleted without having to make any changes to the select directive.
Select related directives (single/multiple/ngOptions) can provide specific versions of
SelectController.writeValue and SelectController.readValue, which are responsible for getting
the $viewValue in or out of the actual <select> element and its <option> children.

BREAKING CHANGE:

When using ngOptions: the directive applies a surrogate key as the value of the <option> element.
This commit changes the actual string used as the surrogate key. We now store a string that is computed
by calling hashKey on the item in the options collection; previously it was the index or key of the
item in the collection.

Before you might have seen:

<select ng-model="x" ng-option="i in items">
  <option value="1">a</option>
  <option value="2">b</option>
  <option value="3">c</option>
  <option value="4">d</option>
</select>

Now it will be something like:

<select ng-model="x" ng-option="i in items">
  <option value="string:a">a</option>
  <option value="string:b">b</option>
  <option value="string:c">c</option>
  <option value="string:d">d</option>
</select>

If your application code relied on this value, which it shouldn't, then you will need to modify your
application to accommodate this. You may find that you can use the track by feaure of ngOptions
as this provides the ability to specify the key that is stored.

BREAKING CHANGE:

When iterating over an object's properties using the (key, value) in obj syntax the order of the elements used to be sorted alphabetically. This was an artificial attempt to create a deterministic ordering since browsers don't guarantee the order. But in practice this is not what people want and so this change iterates over properties in the order they are returned by Object.keys(obj), which is almost always the order in which the properties were defined.

Closes #8019
Closes #9714
Closes #10531

lgalfaso · 2015-01-04T15:11:15Z

would it be possible to split this into two commits, one that move things around and another that make the modifications? It would make it a lot easier to review the patch. One PR is ok and so is the order of the commits

petebacondarwin · 2015-01-04T17:02:49Z

Not really. The original file (select.js) is so interwoven it was not really feasible to extract the ngOptions bits without doing a lot of the refactoring. I'll see what I can do...

petebacondarwin · 2015-01-04T17:05:29Z

I guess I could combine these two files into one and then split them in a subsequent commit?

lgalfaso · 2015-01-04T18:06:01Z

the issue with the current patch is that is very hard to know what actually changed and what was only moved. Whatever makes this easy, works for me

petebacondarwin · 2015-01-04T18:54:09Z

Well pretty much everything that was related to ngOptions changed :-)

petebacondarwin · 2015-01-04T20:56:42Z

@lgalfaso - How's that?

lgalfaso · 2015-01-05T13:30:16Z

what will happen if the same option value shows up several times? This can happen when the two options are in different option groups

petebacondarwin · 2015-01-05T13:44:45Z

what will happen if the same option value shows up several times? This can happen when the two options are in different option groups

Good question, I have to check the previous implementation but I think it didn't support multiple options with the same value either,

JohnYoungers · 2015-01-05T13:53:58Z

Even if the previous implementation did support it, is that really a valid requirement? How would you even select the correct item in the list the next time they view that page? you'd have to 'track by' group and value.

petebacondarwin · 2015-01-05T13:55:48Z

@jayoungers - thanks for commenting. I agree that this requirement would be tricky. You could certainly create a track by expression that was unique for a given group/value combination.

lgalfaso · 2015-01-05T14:03:23Z

I was thinking when there is no value, and the text is used

petebacondarwin · 2015-01-05T14:06:05Z

@lgalfaso - so you are referring to the standard select and option directive combination rather than ngOptions? I don't know if this was ever supported either since the addOption method always took the value of the option as its parameter, which means that you would only get one option in the SelectController's map

lgalfaso · 2015-01-05T14:08:30Z

@petebacondarwin ok, makes sense

Narretz · 2015-01-05T20:00:11Z

This looks like a great refactoring, because now we could even put ngOptions inside another module.

Three general observations:

will using a hashmap impact performance with many options? Generally, extraordinary numbers of options smell of bad ui, but it's possible, see e.g. Bad performance when selecting an item the first time in select with many items #8019 - 2500 options is definitely too much, but we should test if there's a visible regression for first render time with say, 200 options.
In the breaking changes you say that the application shouldn't rely on the value of the option. However if you use ngOptions to render, but submit the form to the server via action="", then your backend will probably rely on the correct select value. I guess the number of people who do this is very low, though.
now that ngOptions and select are two different directives, could this cause problems of interoperability with other directives? For example, if someone attached a directive to a select that is supposed to run after the select is completely initialized, could this break?

petebacondarwin · 2015-01-09T23:10:01Z

Awesome bit of review @gkalpak - thank you. I will address those issues over the weekend.

…ns are loaded async **Major reworking of select and ngOptions**: * The `SelectController` is now used as an abstraction for the `select` and `ngOptions` directives to override to get their desired behaviour * The `select` directive is completely oblivious to the ngOptions directive now - the `ngOptions` directive could be deleted without having to make any changes to the `select` directive. * Select related directives (single/multiple/ngOptions) can provide specific versions of `SelectController.writeValue` and `SelectController.readValue`, which are responsible for getting the `$viewValue` in or out of the actual `<select>` element and its `<option>` children. BREAKING CHANGE: When using `ngOptions`: the directive applies a surrogate key as the value of the `<option>` element. This commit changes the actual string used as the surrogate key. We now store a string that is computed by calling `hashKey` on the item in the options collection; previously it was the index or key of the item in the collection. (This is in keeping with the way that the unknown option value is represented in the select directive.) Before you might have seen: ``` <select ng-model="x" ng-option="i in items"> <option value="1">a</option> <option value="2">b</option> <option value="3">c</option> <option value="4">d</option> </select> ``` Now it will be something like: ``` <select ng-model="x" ng-option="i in items"> <option value="string:a">a</option> <option value="string:b">b</option> <option value="string:c">c</option> <option value="string:d">d</option> </select> ``` If your application code relied on this value, which it shouldn't, then you will need to modify your application to accommodate this. You may find that you can use the `track by` feaure of `ngOptions` as this provides the ability to specify the key that is stored. Closes angular#9714 Closes angular#10639

…ns are loaded async **Major reworking of select and ngOptions**: * The `SelectController` is now used as an abstraction for the `select` and `ngOptions` directives to override to get their desired behaviour * The `select` directive is completely oblivious to the ngOptions directive now - the `ngOptions` directive could be deleted without having to make any changes to the `select` directive. * Select related directives (single/multiple/ngOptions) can provide specific versions of `SelectController.writeValue` and `SelectController.readValue`, which are responsible for getting the `$viewValue` in or out of the actual `<select>` element and its `<option>` children. BREAKING CHANGE: When using `ngOptions`: the directive applies a surrogate key as the value of the `<option>` element. This commit changes the actual string used as the surrogate key. We now store a string that is computed by calling `hashKey` on the item in the options collection; previously it was the index or key of the item in the collection. (This is in keeping with the way that the unknown option value is represented in the select directive.) Before you might have seen: ``` <select ng-model="x" ng-option="i in items"> <option value="1">a</option> <option value="2">b</option> <option value="3">c</option> <option value="4">d</option> </select> ``` Now it will be something like: ``` <select ng-model="x" ng-option="i in items"> <option value="string:a">a</option> <option value="string:b">b</option> <option value="string:c">c</option> <option value="string:d">d</option> </select> ``` If your application code relied on this value, which it shouldn't, then you will need to modify your application to accommodate this. You may find that you can use the `track by` feaure of `ngOptions` as this provides the ability to specify the key that is stored. Closes angular#8019 Closes angular#9714 Closes angular#10639

lgalfaso · 2015-01-12T19:26:32Z

LGTM, @petebacondarwin can you land this?

…ns are loaded async **Major reworking of select and ngOptions**: * The `SelectController` is now used as an abstraction for the `select` and `ngOptions` directives to override to get their desired behaviour * The `select` directive is completely oblivious to the ngOptions directive now - the `ngOptions` directive could be deleted without having to make any changes to the `select` directive. * Select related directives (single/multiple/ngOptions) can provide specific versions of `SelectController.writeValue` and `SelectController.readValue`, which are responsible for getting the `$viewValue` in or out of the actual `<select>` element and its `<option>` children. BREAKING CHANGE: When using `ngOptions`: the directive applies a surrogate key as the value of the `<option>` element. This commit changes the actual string used as the surrogate key. We now store a string that is computed by calling `hashKey` on the item in the options collection; previously it was the index or key of the item in the collection. (This is in keeping with the way that the unknown option value is represented in the select directive.) Before you might have seen: ``` <select ng-model="x" ng-option="i in items"> <option value="1">a</option> <option value="2">b</option> <option value="3">c</option> <option value="4">d</option> </select> ``` Now it will be something like: ``` <select ng-model="x" ng-option="i in items"> <option value="string:a">a</option> <option value="string:b">b</option> <option value="string:c">c</option> <option value="string:d">d</option> </select> ``` If your application code relied on this value, which it shouldn't, then you will need to modify your application to accommodate this. You may find that you can use the `track by` feaure of `ngOptions` as this provides the ability to specify the key that is stored. BREAKING CHANGE: When iterating over an object's properties using the `(key, value) in obj` syntax the order of the elements used to be sorted alphabetically. This was an artificial attempt to create a deterministic ordering since browsers don't guarantee the order. But in practice this is not what people want and so this change iterates over properties in the order they are returned by Object.keys(obj), which is almost always the order in which the properties were defined. Closes angular#8019 Closes angular#9714 Closes angular#10639

Since select is not aware of ngOptions, it makes sense to move it into its own file for more easy maintenance.

…y appear Closes angular#10531

…ns maps to null Closes angular#7605

Closes angular#7736

…ple attribute

Closes angular#9464

Utilize the $watchDelegate on the watcher used to detect changes to the labels. Closes angular#10687 Closes angular#10694

petebacondarwin · 2015-01-12T19:48:04Z

Landing now.

googlebot added the cla: yes label Jan 4, 2015

petebacondarwin added branch: 1.4.x component: forms frequency: moderate needs: review severity: broken expected use type: bug type: refactor labels Jan 4, 2015

petebacondarwin added this to the 1.4.x milestone Jan 4, 2015

petebacondarwin assigned Narretz Jan 4, 2015

petebacondarwin mentioned this pull request Jan 4, 2015

Regression using ngOptions asynchronously in angular 1.3 #9714

Closed

petebacondarwin force-pushed the select-refact branch 2 times, most recently from d4b1762 to 6f38132 Compare January 4, 2015 20:21

petebacondarwin force-pushed the select-refact branch from 6f38132 to 1658c39 Compare January 5, 2015 12:35

petebacondarwin force-pushed the select-refact branch from 26c769d to f87382e Compare January 5, 2015 19:01

lgalfaso force-pushed the master branch from 35aec52 to e24d968 Compare January 5, 2015 20:11

petebacondarwin force-pushed the select-refact branch from 8d9dfd6 to 78b9344 Compare January 10, 2015 21:36

petebacondarwin changed the title ~~fix(ngOptions): ensure that the correct option is selected when options are loaded async~~ refact(ngOptions): major refactoring... Jan 10, 2015

This was referenced Jan 10, 2015

ngOptions: non-grouped options shouldn't float to the top when using group by #10531

Closed

Angular Select select as label for value in array syntax broken for nullable #7605

Closed

petebacondarwin force-pushed the select-refact branch from ede182e to c77b006 Compare January 11, 2015 13:37

petebacondarwin mentioned this pull request Jan 11, 2015

Bad performance when selecting an item the first time in select with many items #8019

Closed

petebacondarwin force-pushed the select-refact branch from 6c0b011 to aa0064c Compare January 11, 2015 15:57

petebacondarwin force-pushed the select-refact branch from aa0064c to 4a9cd13 Compare January 11, 2015 16:04

petebacondarwin added 9 commits January 12, 2015 19:47

refact(ngOptions): move into its own file

578e8c4

Since select is not aware of ngOptions, it makes sense to move it into its own file for more easy maintenance.

test(ngOptions): should place non-grouped items in the list where the…

b4b6822

…y appear Closes angular#10531

test(ngOptions): should not insert a blank option if one of the optio…

94ae280

…ns maps to null Closes angular#7605

style(ngOptionsSpec): add extra newline for better formatting

599e5b8

fix(ngOptions): update model if selected option is removed

2d0a0a5

Closes angular#7736

refact(ngOptions): specialize readValue and writeValue based on multi…

de7c178

…ple attribute

fix(ngOptions): prevent infinite digest if track by expression is stable

4725ddb

Closes angular#9464

fix(ngOptions): support one-time binding on the option values

70e5015

Utilize the $watchDelegate on the watcher used to detect changes to the labels. Closes angular#10687 Closes angular#10694

petebacondarwin force-pushed the select-refact branch from dbb0ab3 to 70e5015 Compare January 12, 2015 19:48

petebacondarwin closed this in 7fda214 Jan 12, 2015

realityking mentioned this pull request Jan 14, 2015

Remove sortedKeys() #10757

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refact(ngOptions): major refactoring... #10639

refact(ngOptions): major refactoring... #10639

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

JohnYoungers commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

lgalfaso commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

lgalfaso commented Jan 5, 2015

Narretz commented Jan 5, 2015

petebacondarwin commented Jan 9, 2015

lgalfaso commented Jan 12, 2015

petebacondarwin commented Jan 12, 2015

refact(ngOptions): major refactoring... #10639

refact(ngOptions): major refactoring... #10639

Conversation

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

petebacondarwin commented Jan 4, 2015

lgalfaso commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

JohnYoungers commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

lgalfaso commented Jan 5, 2015

petebacondarwin commented Jan 5, 2015

lgalfaso commented Jan 5, 2015

Narretz commented Jan 5, 2015

petebacondarwin commented Jan 9, 2015

lgalfaso commented Jan 12, 2015

petebacondarwin commented Jan 12, 2015