Ng model options extend #15389

petebacondarwin · 2016-11-14T19:15:18Z

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

feat

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

ngModelOptions cannot be inherited from ancestor ngModelOptions

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

ngModelOptions can inherit if specified via $inherit values

Does this PR introduce a breaking change?

A small internal-ish change to the programmatic API for reading options.

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:

…stor ngModelOptions This reverts commit 87a2ff7

gkalpak · 2016-11-14T23:27:14Z

src/ng/directive/ngModelOptions.js

@@ -0,0 +1,378 @@
+'use strict';
+
+/* global $DefaultModelOptionsProvider: true */


Shouldn't it be exported, rather than global?

gkalpak · 2016-11-14T23:33:15Z

src/ng/directive/ngModelOptions.js

+ * @name $defaultModelOptionsProvider
+ * @description
+ *
+ * Here, you can change the default settings from which {@link ngModelOptions}


This service is not only used for ngModelOptions, but for ngModel as well.
It would be good to clarify that, so people know what they are modifying 😃

gkalpak · 2016-11-14T23:34:36Z

src/ng/directive/ngModelOptions.js

+ * @description
+ * This directive allows you to modify the behaviour of ngModel and input directives within your
+ * application. You can specify an ngModelOptions directive on any element and the settings affect
+ * the ngModel and input directives on all descendent elements.


on all descendent elements

Not exactly 😃 It is worth clarifying that it doesn't (necessarily) affect elements past its closest ngModelOptions descendant.

gkalpak · 2016-11-14T23:35:16Z

src/ng/directive/ngModelOptions.js

+ * @name ngModelOptions
+ *
+ * @description
+ * This directive allows you to modify the behaviour of ngModel and input directives within your


Why input directives? It only affects ngModel afaict.

gkalpak · 2016-11-14T23:41:46Z

src/ng/directive/ngModelOptions.js

+ * ```
+ *
+ * Notice that the `debounce` setting now inherits The value from the outer `<div>` element.
+ *


Here is a good place to add a warning alert for library authors, mentioning that "*" should be used with caution, because of the possibility of new options being introduced in future versions.

gkalpak · 2016-11-15T07:57:25Z

src/ng/directive/ngModelOptions.js

+        if (key === '*') {
+          inheritAll = true;
+        } else {
+          options[key] = isDefined(parentOptions[key]) ? parentOptions[key] : defaultOptions[key];


If we always extended options with defaultOptions (see comment above), then here too we could simply use: options[key] = parentOptions[key]

gkalpak · 2016-11-15T08:08:27Z

src/ng/directive/ngModelOptions.js

+     * specified.
+     */
+    $get: function() {
+      return new ModelOptions(defaultOptions, defaultOptions);


I think it would simplify the ModelOptions implementation if defaultOptions was a top-level var, so we didn't have to pass it around from parent to child.

Are you suggesting we create a global variable!?!
I agree.

gkalpak · 2016-11-15T08:28:12Z

test/ng/directive/ngModelOptionsSpec.js

+        expect(inputOptions.getOption('updateOnDefault')).toEqual($defaultModelOptions.getOption('updateOnDefault'));
+        expect(inputOptions.getOption('debounce')).toEqual($defaultModelOptions.getOption('debounce'));
+        expect(inputOptions.getOption('getterSetter')).toEqual($defaultModelOptions.getOption('getterSetter'));
+        expect(inputOptions.getOption('allowInvalid')).toEqual($defaultModelOptions.getOption('allowInvalid'));


What about timezone?

gkalpak · 2016-11-15T08:30:44Z

test/ng/directive/ngModelOptionsSpec.js

+/* globals generateInputCompilerHelper: false */
+describe('ngModelOptions', function() {
+
+  describe('$defaultModelOptions', function() {


I would add a test showing the default options are overwritable.

Actually they are not :-P
I just need to fix the docs - see my other comment

gkalpak · 2016-11-15T10:11:59Z

test/ng/directive/ngModelOptionsSpec.js

+            '</form>')($rootScope);
+        $rootScope.$digest();
+
+        var inputElm = doc.find('input').eq(0);


Nit: Isn't .eq(0) redundant?

I think that find does not return the object that we want (i.e. some collection thing) so we must convert it to get an instance.

I see that it is not used elsewhere... I'll try removing it

eq returns a new collection with a subset of the items (typically just one):

eq: function(index) { return (index >= 0) ? jqLite(this[index]) : jqLite(this[this.length + index]); }

Here there is only one input element anyway, so it is redundant.

…ModelOptions` Previously, you had to apply the complete set of `ngModelOptions` at every point where you might want to modify just one or two settings. This change allows more general settings to be applied nearer to the top of the DOM and then for more specific settings to override those general settings further down in the DOM. To prevent unwanted inheritance you must opt-in on a case by case basis: * To inherit as single property you simply provide the special value `"$inherit"`. * To inherit all properties not specified locally then include a property `"*": "$inherit"`. Closes angular#10922 Closes angular#15389 BREAKING CHANGE: The programmatic API for `ngModelOptions` has changed. You must now read options via the `ngModelController.getOption(name)` method, rather than accessing the option directly as a property of the `ngModelContoller.$options` object. This does not affect the usage in templates and only affects custom directives that might have been reading options for their own purposes.

gkalpak · 2016-11-15T13:01:45Z

src/ng/directive/ngModelOptions.js

+          this.$$options[key] = this.$$parentOptions[key];
+        }
+      }
+    }, this);


Who needs arrow functions 😄

gkalpak · 2016-11-15T13:10:23Z

src/ng/directive/ngModelOptions.js

+ * { allowInvalid: true, updateOn: 'default', debounce: 200 }
+ * ```
+ *
+ * Notice that the `debounce` setting now inherits The value from the outer `<div>` element.


The value --> the value

gkalpak · 2016-11-15T13:26:35Z

src/ng/directive/ngModelOptions.js

+  $$processOptions: function() {
+    var options = this.$$options;
+    // updateOn and updateOnDefault
+    if (isDefined(options.updateOn) && options.updateOn.trim()) {


We need to handle this differently now. For example:

<div ng-model-options="{updateOn: 'default blur'}"> <div ng-model-options="{'*': '$inherit'}"></div> </div>

The outer ModelOptions will have updateOn: 'blur', updateOnDefault: true.

During the $$extendParent() phase, the values will be copied onto the inner ModelOptions.

Then, during the $$processOptions phase, updateOn: 'blur' will be treated as the initial value and updateOnDefault will be reset to false.

Right! Now this is a good review point. I knew there must be something that needed fixing. I'll take a look right now.

…ModelOptions` Previously, you had to apply the complete set of `ngModelOptions` at every point where you might want to modify just one or two settings. This change allows more general settings to be applied nearer to the top of the DOM and then for more specific settings to override those general settings further down in the DOM. To prevent unwanted inheritance you must opt-in on a case by case basis: * To inherit as single property you simply provide the special value `"$inherit"`. * To inherit all properties not specified locally then include a property `"*": "$inherit"`. Closes angular#10922 Closes angular#15389 BREAKING CHANGE: The programmatic API for `ngModelOptions` has changed. You must now read options via the `ngModelController.getOption(name)` method, rather than accessing the option directly as a property of the `ngModelContoller.$options` object. This does not affect the usage in templates and only affects custom directives that might have been reading options for their own purposes.

gkalpak · 2016-11-16T10:54:32Z

src/ng/directive/ngModel.js

@@ -244,7 +245,7 @@ function NgModelController($scope, $exceptionHandler, $attr, $element, $parse, $
  this.$pending = undefined; // keep pending keys here
  this.$name = $interpolate($attr.name || '', false)($scope);
  this.$$parentForm = nullFormCtrl;
-  this.$options = $modelOptions;
+  this.$options = $defaultModelOptions;


Nit: Typically, internal/private variables are not prefixed. The $ prefix is supposed to indicate public stuff.

gkalpak · 2016-11-16T11:08:50Z

src/ng/directive/ngModelOptions.js

+        } else {
+          options[key] = this.$$options[key];
+        }
+      } else if (key === 'updateOn') {


There are still some issues with this implementation:

If {updateOn: '$inherit'} it will inherit the updateOn, but not the updateOnDefault (which might be inherited by $defaultModelOptions).

Great. I am really working your reviewing skills today!

…ModelOptions` Previously, you had to apply the complete set of `ngModelOptions` at every point where you might want to modify just one or two settings. This change allows more general settings to be applied nearer to the top of the DOM and then for more specific settings to override those general settings further down in the DOM. To prevent unwanted inheritance you must opt-in on a case by case basis: * To inherit as single property you simply provide the special value `"$inherit"`. * To inherit all properties not specified locally then include a property `"*": "$inherit"`. Closes angular#10922 Closes angular#15389 BREAKING CHANGE: The programmatic API for `ngModelOptions` has changed. You must now read options via the `ngModelController.getOption(name)` method, rather than accessing the option directly as a property of the `ngModelContoller.$options` object. This does not affect the usage in templates and only affects custom directives that might have been reading options for their own purposes.

petebacondarwin · 2016-11-16T11:37:54Z

OK, trying again. See if you can find another failing scenario @gkalpak

gkalpak

Fourth time's a charm 😃
LGTM

gkalpak · 2016-11-16T12:13:11Z

test/ng/directive/ngModelOptionsSpec.js

+      it('should `updateOnDefault` as well if we have `updateOn: "$inherit"`', function() {
+        var container = $compile(
+                  '<div ng-model-options="{updateOn: \'keyup\'}">' +
+                    '<input ng-model-options="{\'updateOn\': \'$inherit\'}">' +


Unnecessary quotes around updateOn (here and below).

…ModelOptions` Previously, you had to apply the complete set of `ngModelOptions` at every point where you might want to modify just one or two settings. This change allows more general settings to be applied nearer to the top of the DOM and then for more specific settings to override those general settings further down in the DOM. To prevent unwanted inheritance you must opt-in on a case by case basis: * To inherit as single property you simply provide the special value `"$inherit"`. * To inherit all properties not specified locally then include a property `"*": "$inherit"`. Closes angular#10922 Closes angular#15389 BREAKING CHANGE: The programmatic API for `ngModelOptions` has changed. You must now read options via the `ngModelController.getOption(name)` method, rather than accessing the option directly as a property of the `ngModelContoller.$options` object. This does not affect the usage in templates and only affects custom directives that might have been reading options for their own purposes.

…ModelOptions` Previously, you had to apply a complete set of `ngModelOptions` at many places in the DOM where you might want to modify just one or two settings. This change allows more general settings to be applied nearer to the root of the DOM and then for more specific settings to inherit those general settings further down in the DOM. To prevent unwanted inheritance you must opt-in on a case by case basis: * To inherit as single property you simply provide the special value `"$inherit"`. * To inherit all properties not specified locally then include a property `"*": "$inherit"`. Closes angular#10922 Closes angular#15389 BREAKING CHANGE: The programmatic API for `ngModelOptions` has changed. You must now read options via the `ngModelController.$options.getOption(name)` method, rather than accessing the option directly as a property of the `ngModelContoller.$options` object. This does not affect the usage in templates and only affects custom directives that might have been reading options for their own purposes. One benefit of these changes, though, is that the `ngModelControler.$options` property is now guaranteed to be defined so there is no need to check before accessing. So, previously: ``` var myOption = ngModelController.$options && ngModelController.$options['my-option']; ``` and now: ``` var myOption = ngModelController.$options.getOption('my-option'); ```

revert: feat(ngModelOptions): allow options to be inherited from ance…

31f061c

…stor ngModelOptions This reverts commit 87a2ff7

googlebot added the cla: yes label Nov 14, 2016

petebacondarwin added branch: 1.6.x needs: breaking change needs: public api type: feature labels Nov 14, 2016

petebacondarwin added this to the 1.6.0-rc.1 milestone Nov 14, 2016

gkalpak reviewed Nov 15, 2016

View reviewed changes

petebacondarwin force-pushed the ng-model-options-extend branch from 898ba1a to 24be474 Compare November 15, 2016 11:32

petebacondarwin force-pushed the ng-model-options-extend branch from 24be474 to 82f32ec Compare November 15, 2016 11:53

gkalpak suggested changes Nov 15, 2016

View reviewed changes

gkalpak suggested changes Nov 16, 2016

View reviewed changes

petebacondarwin force-pushed the ng-model-options-extend branch from d44e5b9 to e0b5bd2 Compare November 16, 2016 11:37

gkalpak approved these changes Nov 16, 2016

View reviewed changes

petebacondarwin force-pushed the ng-model-options-extend branch from e0b5bd2 to 1ba33e7 Compare November 16, 2016 13:40

petebacondarwin closed this in 296cfce Nov 16, 2016

petebacondarwin mentioned this pull request Nov 16, 2016

feat(ngModelOptions): introduce separate directive for inheriting from parent ngModelOptions #15388

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ng model options extend #15389

Ng model options extend #15389

petebacondarwin commented Nov 14, 2016

gkalpak Nov 14, 2016

gkalpak Nov 14, 2016

gkalpak Nov 14, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 14, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 14, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 15, 2016

petebacondarwin Nov 15, 2016

gkalpak Nov 15, 2016

gkalpak Nov 15, 2016

gkalpak Nov 15, 2016

gkalpak Nov 15, 2016

petebacondarwin Nov 16, 2016

gkalpak Nov 16, 2016

gkalpak Nov 16, 2016

petebacondarwin Nov 16, 2016

petebacondarwin commented Nov 16, 2016

gkalpak left a comment

gkalpak Nov 16, 2016

		@@ -0,0 +1,378 @@
		'use strict';

		/* global $DefaultModelOptionsProvider: true */

Ng model options extend #15389

Ng model options extend #15389

Conversation

petebacondarwin commented Nov 14, 2016

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

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

Choose a reason for hiding this comment

petebacondarwin commented Nov 16, 2016

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment