fix(ngModelController): allow $overrideModelOptions to set updateOn #16352

Narretz · 2017-11-29T14:40:38Z

Also adds more docs about "default" events and how to override
ModelOptions.

Closes #16351

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

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?

Please check if the PR fulfills these requirements

The commit message follows our guidelines
Fix/Feature: Docs have been added/updated
Fix/Feature: Tests have been added; existing tests pass

Other information:

petebacondarwin · 2017-11-29T16:59:04Z

I think, due to inheritance, that implementing this is more complicated than it seems. If a child ngModel is inheriting the current ngModelOptions, then changing the updateOn field on the parent would need to tell all descendant ngModels to reapply their event handlers.

Narretz · 2017-11-29T17:11:12Z

I've added a note that overrideModelOptions does not propagate the changes. This is a general problem - even the simple options like debounce, allowInvalid are not updated.
I personally think that this is an acceptable limitation, as overriding options should be a rather rare and specific operation.

gkalpak · 2017-11-30T15:23:31Z

src/ng/directive/ngModel.js

+   * <div class="alert alert-warning">
+   * **Note:** overriding the model options with this function only affects the
+   * current `ngModelController` and does not propagate the new options to
+   * `ngModelOptions` directives that have initially inherited from it.


Technically, since createChild() returns a new instance, nobody has inherited from it. It might make sense to highlight that.

I wonder if the whole notice is irrelevant, as you can only $overrideModelOptions on ngModel, and nested ngModels are definitely "here be dragons" territory. Or should we mention that this doesn't change the options on ngModelOptions that this instance has inherited from?

Actually, I think you are right. The inheritance is between ngModelOptions (which can be nested). ngModel just takes the options of its nearest ngModuleOptions and it can overwrite them. Where the corresponding ngModelOptions got its values (inherited or not) is not affected (nor is there any reason to assume it would).

gkalpak · 2017-11-30T15:38:16Z

src/ng/directive/ngModel.js

@@ -1321,9 +1348,7 @@ var ngModelDirective = ['$rootScope', function($rootScope) {
        post: function ngModelPostLink(scope, element, attr, ctrls) {
          var modelCtrl = ctrls[0];
          if (modelCtrl.$options.getOption('updateOn')) {


This check could be skipped here.

gkalpak · 2017-11-30T15:38:57Z

src/ng/directive/ngModel.js

+    }
+
+    this.$$updateEvents = this.$options.getOption('updateOn');
+    this.$$element.on(this.$$updateEvents, this.$$updateEventHandler);


I think it would be better to avoid this call if $$updateEvents are empty. (Several operations take place unnecessarily.)

gkalpak · 2017-11-30T15:41:01Z

src/ng/directive/ngModelOptions.js

+ *     matches the default events belonging to the control. These are the events that are bound to
+ *     the control, and when fired, update the `$viewValue` via `$setViewValue`.
+ *
+ *     `ngModelOptions` considers every event that is not listed in `updateOn` a "default" event,


gkalpak · 2017-11-30T15:41:29Z

src/ng/directive/ngModelOptions.js

+ * ### Default events, extra triggers, and catch-all debounce values
+ *
+ * This example shows the relationship between "default" update events and
+ * additional `updateOn`triggers.


Missing whitespace before triggers.

gkalpak · 2017-11-30T15:45:10Z

src/ng/directive/ngModelOptions.js

+        });
+ *   </file>
+ *   <file name="template.html">
+      <form name="$ctrl.form">


Too little indentation.

gkalpak · 2017-11-30T15:45:30Z

src/ng/directive/ngModelOptions.js

+      <form name="$ctrl.form">
+        Input: <input type="text" name="input" ng-model="$ctrl.name" ng-model-options="$ctrl.options" />
+      </form>
+        Model: <tt>{{$ctrl.name}}</tt>


Too much indentation.

gkalpak · 2017-11-30T16:20:44Z

src/ng/directive/ngModelOptions.js

+        <input type="submit" value="Update options">
+      </form>
+ *   </file>
+ * </example>


This is one advanced example indeed. I am not even sure what it does 😛

Have you read the description? 😛

petebacondarwin

LGTM - despite @gkalpak's sad face.
The extra explanatory docs are great.

petebacondarwin · 2017-12-05T14:55:42Z

src/ng/directive/ngModelOptions.js

+            };
+
+            this.updateEvents = function() {
+              var eventL = this.options.updateOn.split(' ');


Also adds more docs about "default" events and how to override ModelOptions. Closes angular#16351

Also adds more docs about "default" events and how to override ngModelController options. Closes angular#16351 Closes angular#16352

Narretz added component: forms type: bug type: docs labels Nov 29, 2017

Narretz added this to the 1.7.0 milestone Nov 29, 2017

Narretz requested review from petebacondarwin and gkalpak November 29, 2017 14:40

googlebot added the cla: yes label Nov 29, 2017

Narretz force-pushed the fix-ngmodeloptions-updateon branch from 559f92d to 3984ce4 Compare November 29, 2017 14:43

gkalpak reviewed Nov 30, 2017

View reviewed changes

Narretz force-pushed the fix-ngmodeloptions-updateon branch from 3984ce4 to baac542 Compare December 4, 2017 11:55

petebacondarwin approved these changes Dec 5, 2017

View reviewed changes

fix(ngModelController): allow $overrideModelOptions to set updateOn

7f59fa1

Also adds more docs about "default" events and how to override ModelOptions. Closes angular#16351

Narretz force-pushed the fix-ngmodeloptions-updateon branch from baac542 to 7f59fa1 Compare December 5, 2017 15:21

Narretz merged commit da72477 into angular:master Dec 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ngModelController): allow $overrideModelOptions to set updateOn #16352

fix(ngModelController): allow $overrideModelOptions to set updateOn #16352

Narretz commented Nov 29, 2017

petebacondarwin commented Nov 29, 2017

Narretz commented Nov 29, 2017 •

edited

Loading

gkalpak Nov 30, 2017

Narretz Dec 1, 2017

gkalpak Dec 1, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

gkalpak Nov 30, 2017

Narretz Dec 1, 2017

petebacondarwin left a comment

petebacondarwin Dec 5, 2017

fix(ngModelController): allow $overrideModelOptions to set updateOn #16352

fix(ngModelController): allow $overrideModelOptions to set updateOn #16352

Conversation

Narretz commented Nov 29, 2017

petebacondarwin commented Nov 29, 2017

Narretz commented Nov 29, 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

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz commented Nov 29, 2017 •

edited

Loading