wip: feat(ngModel): run formatters / setModelValue #16237

Narretz · 2017-09-21T13:29:54Z

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)
There's currently no way to manually run the model -> view pipeline / formatters

What is the new behavior (if this is a feature change)?
An API to run the whole pipeline / the formatters is introduced.

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

The PR includes both a $format and a $setModelValue function. I think only one is needed, we just need to decide which one.

$format:

PRO: does the most needed thing, very straightforward: run the formatters and updates the viewValue
CON: developer must run $render manually
CON: empty classes are set even though the DOM is not updated. Could be moved to $render()
CON: no equivalent on the view -> model side (inconsistent API)

$setModelValue

PRO: equivalent to $setViewValue, runs the whole pipeline
CON: unintuitive that the function argument should / must be set to the current $modelValue, so the control does not get out of sync with the scope (same behavior as $setViewValue though)
CON: only runs $render if the viewValue has actually changed.
CON: always runs the validation, even though it might not be necessary

Both methods can handle the basic case, where an app developer wants to run the formatters the view -> model pipeline has been run, see #3407 or #5221

I personally tend to introduce $format as it has the smaller surface area and introduces fewer side effects. The full model -> view pipeline is not really needed for most cases.

jbedard · 2017-09-21T16:12:04Z

src/ng/directive/ngModel.js


+  function ngModelWatchAction(modelValue) {
    // if scope model value and ngModel value are out of sync
    // TODO(perf): why not move this to the action fn?


Can delete this now

jbedard · 2017-09-21T16:12:57Z

src/ng/directive/ngModel.js


+  function ngModelWatchAction(modelValue) {
    // if scope model value and ngModel value are out of sync
    // TODO(perf): why not move this to the action fn?
    if (modelValue !== ctrl.$modelValue &&


Can this be deleted now?

jbedard · 2017-09-21T16:13:16Z

src/ng/directive/ngModel.js

@@ -921,7 +924,8 @@ function setupModelWatcher(ctrl) {
    }

    return modelValue;


Can this be deleted now?

I guess we still need to return this because we need the digest to cycle again if the model value has actually changed...?

Yeah this comment is outdated now. This was from when this was moved to the listener fn...

Narretz · 2017-09-25T09:44:29Z

@jbedard I had to revert the code that was moved to the watchActionFn, because it caused test failures. Specifcally, it caused failures when the model was updated inside ngChange or the model setter - so I think the way it's done is necessary.

petebacondarwin · 2017-09-25T09:49:22Z

What about something like $processModelValue, which is effectively the same as your $setModelValue but doesn't take an argument, instead just using whatever is in th current $modelValue?

Narretz · 2017-09-25T11:46:19Z

Hm, that sounds like a good in-between option. It' doesn't have a public equivalent for view -> model but $$parseAndValidate could be exposed if necessary.

gkalpak · 2017-09-25T19:07:23Z

Generally I prefer to keep the API consistent (it makes it much easier to wrap your head around), but in this case $processModelValue() sounds like a good idea.

BTW, I agree it makes sense to tie updateEmptyClasses() with the rendering.

Narretz · 2017-09-26T09:15:23Z

Î'll update the PR then. Regarding $$updateEmptyClasses(), I thought about it again, and think it's okay that this is set before $render() is called. It's also more of an issue if we had a $format() function, because that would not have called render.
But $processModelValue calls everything in the pipeline, including $render(), so it's not a big deal that $$updateEmptyClasses() is called right before it.
On the view -> model pipeline it's also only called when the viewValue is committed, so it's also not directly tied to the actual DOM / render value.

Narretz · 2017-09-26T13:25:29Z

Updated.
I left $$format and $$setModelValue as internal functions, and added $processModelValue with a PoC autocomplete example to illustrate the use case.

gkalpak

Left some docs-related comments.
Otherwise LGTM 👍

gkalpak · 2017-09-27T07:21:24Z

src/ng/directive/ngModel.js

-       (ctrl.$modelValue === ctrl.$modelValue || modelValue === modelValue)
+      // checks for NaN is needed to allow setting the model to NaN when there's an asyncValidator
+      // eslint-disable-next-line no-self-compare
+     (ctrl.$modelValue === ctrl.$modelValue || modelValue === modelValue)


Nice try! But the indentation i still off 😛

gkalpak · 2017-09-27T07:29:36Z

src/ng/directive/ngModel.js

+   * @description
+   *
+   * Runs the model -> view pipeline on the current
+   * {@link ngModel.NgModelController#$modelValue $modelValue}


Missing . at the end.

gkalpak · 2017-09-27T07:37:26Z

src/ng/directive/ngModel.js

+   * Application developers usually do not have to call this function themselves.
+   *
+   * This function can be used when the $viewValue or the rendered DOM value of the control should
+   * be updated after a user input.


It is not necessarily about user input. The way I understand it, it is for when app state has changed in a way that may affect formatting. (OK, I admit most of the time the state changes as a result of user input, but still... 😛)

That's right. This is is just the most common case. Another would be that the formatters have changed but the model hasn't. If the model changes, the normal model -> view code should be enough. Unless someone changes the $modelValue directly, which is not recommended. I'll make this a it more generic.

gkalpak · 2017-09-27T07:37:49Z

src/ng/directive/ngModel.js

+   * be updated after a user input.
+   *
+   * For example, consider a text input with an autocomplete list (for fruit, where the items are
+   * objects with a name and an id.


Missing closing ).

gkalpak · 2017-09-27T07:48:21Z

src/ng/directive/ngModel.js

+   * For example, consider a text input with an autocomplete list (for fruit, where the items are
+   * objects with a name and an id.
+   * A user enters `ap` and then selects `Apricot` from the list.
+   * Based on this, the autocomplete widget will call $setViewValue({name: 'Apricot', id: 443}),


It is better to wrap code in `. (E.g. $setViewValue(...) here or ctrl.$processModelValue() and $viewValue below.)

Do you wrap in inline code blocks? It looks like your formatting is a bit off

Apparently if you want to escape `, you need to place the \ before, not after the ` 😇

gkalpak · 2017-09-27T07:56:27Z

src/ng/directive/ngModel.js

+   * but the rendered value will still be `ap`.
+   * The widget can then call ctrl.$processModelValue() to run the model -> view
+   * pipeline again, which includes a formatter that converts the object into the string `Apricot`
+   * which is set to the $viewValue and rendered in the DOM.


Consider avoiding two which clauses. E.g. something line:

The widget can then call ctrl.$processModelValue() to run the model -> view pipeline, which will use a formatter to convert the object to the string Apricot, update the $viewValue and finally rendered it in the DOM.

gkalpak · 2017-09-27T07:57:53Z

src/ng/directive/ngModel.js

+
+              ngModel.$parsers.push(function(value) {
+                if (angular.isString(value)) {
+                  return scope.items.find(function(item) {


This is not ES5-compatible 😁

Do we care? This only affects IE for core support, but I don't think we make guarantees for the docs

I don't feel too strongly, but it would be good for the docs to show code that works in all supportd browsers. It would also be a first (afaik).

gkalpak · 2017-09-27T07:58:49Z

src/ng/directive/ngModel.js

+     </file>
+     <file name="index.html">
+       <div style="display: flex;">
+         <div margin-right: 30px;">


Forgot about style 😃

gkalpak · 2017-09-27T08:02:56Z

src/ng/directive/ngModel.js

+           <input ng-model="val" process-model />
+
+           <ul>
+             <li ng-repeat="item in items | filter:val"><button ng-click="select(item)">{{item.name}}</li>


Ehem...relying on a method set on the scope by a directive on a different element doesn't sound like a good practice 😉
Maybe move all relevant code into the directive's template.

It's just a POC that doesn't show correct encapsulation.
I thought about putting it in the directive's template but then it gets more complex too. I'd have to create another directive for the ngModel formatters / parsers, or search for the input directive in the autocomplete directive ... no child selectors in AngularJS unfortunately. Something like this: https://plnkr.co/edit/OOSjPSQag2huO6gtLFgg?p=preview
Idk if it's worth the complexity

I was having something like this in mind: https://plnkr.co/edit/AeOA4eMGGMONSTrLinH2?p=preview
(TBH, I find this whole usecase weird. It sounds like an unnecessary complicated way to create an autocomplete 😃)

Well, that's less complex and shows better encapsulation. I'll take it. But for the record, I don't think a real world autocomplete would be successful with a hard-coded input element. At least not for me ;)

You know the customer is always right, and apparently many devs implement autocomplete like this.

Updated with a small tweak

Narretz

I've fixed the typos and commented on the more tricky stuff

Narretz · 2017-09-27T09:21:51Z

src/ng/directive/ngModel.js

+   * Application developers usually do not have to call this function themselves.
+   *
+   * This function can be used when the $viewValue or the rendered DOM value of the control should
+   * be updated after a user input.


That's right. This is is just the most common case. Another would be that the formatters have changed but the model hasn't. If the model changes, the normal model -> view code should be enough. Unless someone changes the $modelValue directly, which is not recommended. I'll make this a it more generic.

Narretz · 2017-09-27T09:25:16Z

src/ng/directive/ngModel.js

+   * For example, consider a text input with an autocomplete list (for fruit, where the items are
+   * objects with a name and an id.
+   * A user enters `ap` and then selects `Apricot` from the list.
+   * Based on this, the autocomplete widget will call $setViewValue({name: 'Apricot', id: 443}),


Do you wrap in inline code blocks? It looks like your formatting is a bit off

Narretz · 2017-09-27T09:27:50Z

src/ng/directive/ngModel.js

+
+              ngModel.$parsers.push(function(value) {
+                if (angular.isString(value)) {
+                  return scope.items.find(function(item) {


Do we care? This only affects IE for core support, but I don't think we make guarantees for the docs

Narretz · 2017-09-27T09:36:36Z

src/ng/directive/ngModel.js

+           <input ng-model="val" process-model />
+
+           <ul>
+             <li ng-repeat="item in items | filter:val"><button ng-click="select(item)">{{item.name}}</li>


It's just a POC that doesn't show correct encapsulation.
I thought about putting it in the directive's template but then it gets more complex too. I'd have to create another directive for the ngModel formatters / parsers, or search for the input directive in the autocomplete directive ... no child selectors in AngularJS unfortunately. Something like this: https://plnkr.co/edit/OOSjPSQag2huO6gtLFgg?p=preview
Idk if it's worth the complexity

petebacondarwin · 2017-09-28T09:31:45Z

src/ng/directive/ngModel.js

@@ -921,7 +924,8 @@ function setupModelWatcher(ctrl) {
    }

    return modelValue;


I guess we still need to return this because we need the digest to cycle again if the model value has actually changed...?

Closes angular#3407 Closes angular#10764

…el -> view pipeline Closes #3407 Closes #10764 Closes #16237

Narretz added component: forms frequency: moderate severity: inconvenient type: feature labels Sep 21, 2017

Narretz added this to the 1.6.x milestone Sep 21, 2017

googlebot added the cla: yes label Sep 21, 2017

Narretz changed the title ~~Feat model set~~ wip: feat(ngModel): run formatters / setModelValue Sep 21, 2017

jbedard reviewed Sep 21, 2017

View reviewed changes

Narretz force-pushed the feat-model-set branch 2 times, most recently from 4eede61 to 09f501d Compare September 25, 2017 09:40

Narretz force-pushed the feat-model-set branch from 09f501d to d00ed40 Compare September 25, 2017 13:36

Narretz force-pushed the feat-model-set branch from d00ed40 to 29256ce Compare September 26, 2017 13:23

Narretz force-pushed the feat-model-set branch from 29256ce to dc8a725 Compare September 26, 2017 14:16

gkalpak reviewed Sep 27, 2017

View reviewed changes

Narretz commented Sep 27, 2017

View reviewed changes

Narretz force-pushed the feat-model-set branch 3 times, most recently from 1ee7a7f to 6f08ae8 Compare September 27, 2017 19:06

petebacondarwin approved these changes Sep 28, 2017

View reviewed changes

feat(ngModel): expose $processModelValue to run model -> view pipeline

e895e27

Closes angular#3407 Closes angular#10764

Narretz force-pushed the feat-model-set branch from 6f08ae8 to e895e27 Compare September 29, 2017 10:43

Narretz merged commit 05fdf91 into angular:master Sep 29, 2017

Narretz added a commit that referenced this pull request Sep 29, 2017

feat(ngModel.NgModelController): expose $processModelValue to run mod…

1451948

…el -> view pipeline Closes #3407 Closes #10764 Closes #16237

This was referenced Sep 30, 2017

feat(ngModel.NgModelController): expose $processModelValue to run mod… angular-indonesia/angular.js#73

Merged

feat(ngModel.NgModelController): expose $processModelValue to run mod… javascript-indonesias/angular.js#15

Merged

dependencies bot mentioned this pull request Dec 5, 2017

Update 26 dependencies from npm JetBrains/ring-ui#30

Merged

		@@ -921,7 +924,8 @@ function setupModelWatcher(ctrl) {
		}

		return modelValue;

wip: feat(ngModel): run formatters / setModelValue #16237

wip: feat(ngModel): run formatters / setModelValue #16237

Conversation

Narretz commented Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Sep 25, 2017

petebacondarwin commented Sep 25, 2017 • edited Loading

Narretz commented Sep 25, 2017

gkalpak commented Sep 25, 2017

Narretz commented Sep 26, 2017

Narretz commented Sep 26, 2017 • edited Loading

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Sep 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Sep 21, 2017 •

edited

Loading

petebacondarwin commented Sep 25, 2017 •

edited

Loading

Narretz commented Sep 26, 2017 •

edited

Loading

gkalpak Sep 27, 2017 •

edited

Loading