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

feat(ngOptions): extract parsing into a controller #14522

Closed
wants to merge 1 commit into from

Conversation

klieber
Copy link
Contributor

@klieber klieber commented Apr 27, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feature

What is the current behavior? (You can also link to an open issue here)
The logic for parsing ng-options is unavailable via a public api.

What is the new behavior (if this is a feature change)?
Extracts the code for parsing the ng-options expression into a controller on the ngOptions directive so that custom directives can read the options.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

Other information:

Moves the code for parsing ng-options expressions into a controller
on the ngOptions directive. This allows us to create custom
directives that require ngOptions and can access the options list. An
example would be a directive that automatically selects one of the options
if none are selected.

closes #7888

Moves the code for parsing `ng-options` expressions into a controller
on the `ngOptions` directive. This allows us to create custom
directives that require `ngOptions` and can access the options list. An
example would be a directive that automatically selects one of the options
if none are selected.

closes angular#7888
@gkalpak
Copy link
Member

gkalpak commented Apr 28, 2016

On a high level, my only concern with this is that we are exposing too much of our (currently internal) implementation, which binds us to the current API (potentially for little gain). Still, if we decide to commit ourselves to it, we should at least document what that API is

@Narretz
Copy link
Contributor

Narretz commented Apr 28, 2016

I agree that this exposes maybe too much of the internal implementation. If you only need the options list, why do you need to expose trackBy, getWatchables etc?

@klieber
Copy link
Contributor Author

klieber commented Apr 28, 2016

Quite honestly I don't and would guess most people won't. This was more of a first run at this idea. I don't claim to even completely understand how this code works so I was going for the minimal amount of changes I could make to do this.

I'm open to other approaches and I agree that a simpler more well thought out public API would make sense.

Another thought I had was moving the options parsing into a service rather than a controller. It would be something similar to $parser but specialized to take an options expression and it would then return an API for reading the options. This might be better so that other directives similar to ng-options could use it. An example would be Angular UI's typeahead directive. The controller might still be useful but less necessary since you could just use the service and pass the expression to it yourself.

I'm open to any feedback or changes. Just let me know what you think.

@klieber
Copy link
Contributor Author

klieber commented Apr 29, 2016

Just to clarify what the API "might" look like for a $optionsParser service:

var parsedOptions = $optionsParser("option.id as option.description for option in items");
var value = parsedOptions.getValue($scope, locals);
var selectAs = parsedOptions.getSelectAs($scope, locals);
var viewValue = parsedOptions.getViewValue($scope, locals);
var trackBy = parsedOptions.getTrackBy($scope, locals);
var trackByValue = parsedOptions.getTrackByValue($scope, locals);
var display = parsedOptions.getDisplay($scope, locals);
var groupBy = parsedOptions.getGroupBy($scope, locals);
var disableWhen = parsedOptions.getDisableWhen($scope, locals);
var values = parsedOptions.getValues($scope, locals);

My main goal of this whole thing is to avoid need to open the source code to ngOptions, copy the huge regular expression, and paste it into my application code just so I can read the options. This is essentially what Angular Strap has done: https://github.com/mgcrea/angular-strap/blob/master/src/helpers/parse-options.js

@Narretz
Copy link
Contributor

Narretz commented Apr 29, 2016

But do you really need all that info / all these methods? I wonder if the return value of this fn would be enough if it was exposed (surface area is smaller): https://github.com/angular/angular.js/blob/master/src/ng/directive/ngOptions.js#L364-L388

@Narretz
Copy link
Contributor

Narretz commented May 19, 2016

@klieber can you clarify which information you actually need?

@Narretz
Copy link
Contributor

Narretz commented Jun 3, 2016

I'm going to close this issue because we haven't got any feedback and can't move forward / decide without it. Feel free to reopen this issue if you can provide new feedback.

@Narretz Narretz closed this Jun 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comprehension Expression in select directive should be a service
4 participants