feat(ngModelOptions): allow options to be inherited from ancestors

[petebacondarwin]

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 at nearer the top of the DOM
and then for more specific settings to override those general settings further down
in the DOM.

It should also be noted that this change means that ctrl.$options will always reference a valid object
and so a large part of this PR is removing the unnecessary checks for existence of that object.

BREAKING CHANGE:

Previously, if a setting was not applied on ngModelOptions, then it would default
to undefined. Now the setting will be inherited from the nearest ngModelOptions
ancestor.

It is possible that an ngModelOptions directive that does not set a property,
has an ancestor ngModelOptions that does set this property to a value other than
undefined. This would cause the ngModel and input controls below this ngModelOptions
directive to display different behaviour. This is fixed by explictly setting the
property in the ngModelOptions to prevent it from inheriting from the ancestor.

[gkalpak]

effectively ?
following ?

[petebacondarwin]

Yes, "following" thanks @gkalpak

[gkalpak]

Not a big deal, but shouldn't effective be effectively ?
(I am not insisting or anything, just bringing it up again in case you overlooked it; you obviously know better (being a native speaker 😃).)

[petebacondarwin]

:-) yes. Sorry about that

[Narretz]

For readability, could you assign ctrl[2] to a variable?

[petebacondarwin]

Will do

[Narretz]

Looks good so far, but could you mention the modelOptionsProvider in the commit message?

[Narretz]

Btw, in the last paragraph, I am not completely sure what "This would cause the ngModel and input controls below this ngModelOptions directive to display different behaviour." means. Different as in unexpected? Because the option is applied (from ancestor) even though you did not specify it on the current element?

[petebacondarwin]

@gkalpak and @Narretz - thanks for the reviews. I will update now.

[petebacondarwin]

Oops, we all missed that I had the parameters in the wrong order here!!

[Narretz]

Oh my ... well you didn't miss it. ;) Should the merging be tested?

[petebacondarwin]

On it

[fenduru]

What is the motivation for this change? From the docs:

Although we try to avoid breaking changes, there are some cases where it is unavoidable:

• AngularJS has undergone thorough security reviews to make applications safer by default, which drives many of these changes.
• Several new features, especially animations, would not be possible without a few changes.
• Finally, some outstanding bugs were best fixed by changing an existing API.

This does not seem to fit into any of these categories of unavoidable breaking changes.

As the author of an Angular component library, this is massively breaking due to the fact that we export controls that support ng-model as part of their API, but also consume smaller components in their templates. We have already had to guard against the fact that ng-model does require: '^^ngModelOptions'.

In all the places that we guard against this, we will now also have to add every possible ngModelOptions key explicitly. This has the probably unintentional side effect of making any addition to ngModelOptions (for instance, introducing a new option in Angular 1.7) a breaking change.

This seems like something that has small enough value that it should not have been deemed necessary to make in a minor release.

[Narretz]

The main value of this is that you don't have to set your model options for every single input, but can set it either in a parent or in the provider. It's very common that you want the same input behavior for parts of your application. And you can still customize your ngModels per instance. You can even "reset" a sub-part of your application (if you don't know what kind of inputs appear in it for example). At the moment, you need to know the defaults for that, that's true. We could maybe introduce a special value that "resets" the options to the default.
Can you show an example of your component and why it breaks? Is it because with this change you possibly don't know what ngModelOptions are set for the application?

I'm also not sure introducing a new option will introduce a BC. A new option will be`optional, so users must make sure their apps (and that includes 3rd party code) are compatible with it when they use it. This is a usual procedure.

[fenduru]

The main value of this is that you don't have to set your model options for every single input, but can set it either in a parent or in the provider.

I personally do not feel that this convenience is enough justification for a breaking change.

Can you show an example of your component and why it breaks? Is it because with this change you possibly don't know what ngModelOptions are set for the application?

This breaks my component because the user was able to inject different behavior past my API boundary with that user. We have this all over our library, but for a simple contrived (though realistic) example:

<!-- myReusableComponent template -->
Enter text: <input type="text" ng-model="internalModel" ng-model-options="{ updateOn: 'keypress' }"></input>
This should always update instantly: {{ internalModel }}
<button ng-click="writeToModel(internalModel)"></button>

<!-- Application template -->
<my-reusable-component ng-model="userModel" ng-model-options="{ debounce: 100 }"></my-reusable-component>

The user's intent was to debounce how often that userModel gets written to, however because of the inheritance now they are also affecting how often the internalModel inside of myReusableComponent updates.

So basically I'm (as a library author) forced to guard against an Angular "feature", while at the same time my user (app author) needs to be aware of whether or not the libraries they are using properly guard against this (in order to satisfy users must make sure their apps (and that includes 3rd party code) are compatible with it when they use it - I don't think it is reasonable for users to have to understand the implementation details of the libraries they are using. Seems weird to me if the answer to "can we upgrade to 1.6 safely" includes the statement "I will need to look at ngMaterial's source code to be sure".

I'm also not sure introducing a new option will introduce a BC. A new option will be`optional, so users must make sure their apps (and that includes 3rd party code) are compatible with it when they use it.

If a framework (Angular) changes code that causes my project (library) to no longer work correctly without me doing work, that is by definition a breaking change. If one of my users starts to use a new option somewhere higher up in the DOM, that option will cause my implementation details to behave differently, causing my library to need code changes.

[petebacondarwin]

This is an interesting use case that we had not thought about. We should definitely work something out here before we release 1.6.0.

Can I be devil's advocate for a moment...

You say, @fenduru, that this PR would make you have to set ngModelOptions for all possible values in every directive that you create. Strictly you only need to set the values of the ones that you care about. In the example case, debounce.

With the behaviour that this PR introduces, any new options added to ngModelOptions would most likely need to be considered as a breaking change since they could affect the behaviour of applications that are already in production. This is the same situation as if we changed the default behaviour of an input directive directly.

So if we followed this approach you would only need to update the ngModelOptions values on your own directives at breaking change versions, e.g. 1.7.0, 1.8.0, etc.

I wonder if this restriction on adding values to ngModelOptions would be too restrictive in any case?

I don't like the idea of adding a "reset" option, since it just adds to the API surface area to work around something in a way that is not intuitive without reading the docs.

One option could be that we, somehow, break the inheritance at isolated scope boundaries. Which means that the ngModelOptions will not impact on components that will have complete control over their ngModelOptions values in their templates. I think this could work, although if users could still update values in the global $modelOptions service then we would potentially have the same problem.

If we cannot find a reasonable solution to this then I agree that we should consider reverting for 1.6.0.

[gkalpak]

Another idea would be making the inheritance opt-in. E.g. keep ng-model-options non-inheriting (as it was before 1.6.0-rc.0) and add a new ng-extend-model-options (or something) that does exactly the same except for inheriting (i.e. as it is in 1.6.0-rc.0).

Internally, we will be using the same code with a flag for inheriting vs not inheriting.

[petebacondarwin]

Hmm, @gkalpak, I kind of like it but it makes me feel bit queasy...
Perhaps rather than a new directive we just added a property to the ngModelOptions bag, such as inherit: true?

So then you would have:

<form ng-model-options="{ debounce: true }">
  <div ng-model-options="{ inherit:true, allowInvalid: true }">
    <input ng-model="o.x"> <!-- this will have { debounce:  true, allowInvalid: true } -->
    <input ng-model-options="{ updateOn: 'keypress' }" ng-model="o.y"> <!-- this will only have { updateOn: 'keypress' } -->
  </div>
</form>

Would that work??

[Narretz]

I'm not a fan of putting another option in the bag. The options affects the ngModel directive, not how the ngModelOptions directive works. It also doesn't make much sense to me that you would have to opt in into the inheritance, because that would defeat the purpose of having to write less code.

[gkalpak]

Yeah, I am not a big fan of the extra option either. All other options refer to ngModel, but inherit would refer to ngModelOptions. Also what would happen when you inherit inherit: false 😛

It also doesn't make much sense to me that you would have to opt in into the inheritance, because that would defeat the purpose of having to write less code.

Typically, you can save several characters via inheritance, so an extra few character (like -extend or -ext) doesn't defeat the purpose imo. Most importantly, inheritance comes in handy when you want to have consistent behavior across several instances without having to repeat the same code, which makes it easier to make changes in one place and have them propagate to children (via inheritance). This attribute of the feature remains unaffected: You only write -extend once and then any change you make to an ancestor ngModeOptions propagates automatically.

[petebacondarwin]

inherit: false would not inherit :-P

There was a request to be able to set defaults for ngModelOptions at an application level (see #12644) - but this would also break the use case given by @fenduru.

The original motivation behind making ngModelOptions inheritable was to support potential changes to ngModel regarding how validators and parsers/formatters work. I believe that it is unlikely that we will implement these changes. (Instead it is more likely that we could offer a completely new forms library that people could opt into).

So I think it is simplest just to revert this feature.

WDYT?

[fenduru]

One option could be that we, somehow, break the inheritance at isolated scope boundaries.

This is something that I have desired in the past to prevent transcluded content from being able to require controllers beyond a boundary (I forget the exact use case now though). Even if it is possible, this sort of things seems too high risk to be worth it IMO.

Another idea would be making the inheritance opt-in. E.g. keep ng-model-options non-inheriting (as it was before 1.6.0-rc.0) and add a new ng-extend-model-options (or something)

This seems like probably the best way of accomplishing this goal. I agree it feels weird, but it also seems the most technically correct / least risky way of doing this.

I'm not a fan of putting another option in the bag. The options affects the ngModel directive, not how the ngModelOptions directive works.

I agree philosophically here, but wouldn't hate this if it made the ergonomics better - though I think ng-extend-model-options is probably just as ergonomic

Currently a user can accomplish something very similar by putting a function on their scope and writing ng-model-options="extendDefaults({ debounce: 100 })" - this might be slightly annoying to do if you have many different scope that want this, but perhaps ng-model-options could pass a local function to the expression? So ng-model-options="$extendDefaults({ debounce: 100 })" (or some better name - naming is hard).

So I think it is simplest just to revert this feature.

I don't really want or need this feature, so from my perspective this is the "best" solution. But any of the opt-in solutions proposed seem reasonable to me if this is a desired feature

[Narretz]

There's a valid use-case for this feature, so I'm not happy with simply reverting it. Introducing a new directive / behavior that achieves inheritance is fine with me. It should be easy to replace ng-model-options with it, so maybe ng-model-options-extend or ng-model-options-inherit