Library does not play well with Angular 1.6 ngModelOptions

[wesleycho]

If a parent component is using ngModel with ngModelOptions in Angular 1.6, then the options will leak to the child using ngModelOptions (i.e. could be a datepicker, datepicker popup, or typeahead), and cause it to be set as the default to be used instead of the fallbacks of the binding specified options or the global constant specified options.

Cross-linking issue in Angular here angular/angular.js#15497

[petebacondarwin]

@wesleycho - can you provide a concrete reproduction of the problem? Then I will try to help find a solution.

[wesleycho]

There are two potential issues here that I'm not sure how to address.

The first one is that ngModelOptions.$options is always present, so https://github.com/angular-ui/bootstrap/blob/master/src/datepicker/datepicker.js#L164-L166 would always default to the ngModelOptionsCtrl version - this breaks existing behavior with falling back to the other provided ways of configuring ngModelOptions in the datepicker (for that example).

The other situation deals with the inheritance aspect of the new ngModelOptions. If I wrap a component - for example, at work, we wrap the datepicker component and use ng-model & ng-model-options. With this new behavior, ngModelOptions.$options will use the options from the parent ngModelOptions if present, and not the override version if I were to provide it via config object through directive bindings.

The solution for both these situations will likely be the same, but I'm a bit mystified as to what is a good way around this that handles Angular 1.5 & 1.6 amicably.

[petebacondarwin]

Is the problem that in 1.6 there is always a $options object on the ngModel controller?

If what you want to do is know whether someone has actually put a real ngModelOptions above the current ngModel then you could explicitly require the ngModelOptions controller.

Would that help?

[wesleycho]

Hm, I guess that'd work - guess I got some refactoring to do...

Thanks for the advice!

[petebacondarwin]

@wesleycho - I believe that the new inheritance of ngModelOptions should not be a problem for you. The way that we have implemented it means that you must "opt-in" to inheritance via use of "$inherit" as a property value.

Even before 1.6 it was always the case that a ngModel directive would "inherit" from the nearest ngModelOptions ancestor. What has changed in 1.6 is that you can now also inherit properties for one ngModelOptions directive from an ancestor ngModelOptions (but only via opt-in).

[wesleycho]

Ah, so basically it will only be a problem that ngModelOptions.$options is always present only when one explicitly is either using it (in which case the override is fine) or if they opted into the inheritance?

[petebacondarwin]

The problem for you, I believe, is that ngModel.$options is now always present.

[wesleycho]

Right, that was my main worry with 1.6.

[petebacondarwin]

Also, from running the tests I see that bootstrap is falling foul of the possibly unhandled rejection that now throws.

[sygnowskip]

Here is the breaking change described, please check: angular/angular.js@296cfce

[dgsmith2]

@wesleycho since $options is always present but you want the fall back, why not use the createChild part of the API. I encountered this issue and began work on a PR before I came across this issue. I've tried this code and the specs pass.

datepicker.js -- init() -- before
ngModelOptions = ngModelCtrl_.$options || $scope.datepickerOptions.ngModelOptions || datepickerConfig.ngModelOptions;

datepicker.js --init() -- after
ngModelOptions = ngModelCtrl_.$options.createChild($scope.datepickerOptions.ngModelOptions).createChild(datepickerConfig.ngModelOptions);

And a bunch of ngModelOptions.timezone become ngModelOptions.getOption('timezone'), etc.

Outside of that, the most failures are the missing promise rejections. Thus far adding angular.noop as the reject function whenever $q is used is working.

I've only just began this work, so there may be some holes

[wesleycho]

Interesting - does createChild create a system for inheritance?

[dgsmith2]

This is the documentation:

/**
   * @ngdoc method
   * @name ModelOptions#createChild
   * @param {Object} options a hash of options for the new child that will override the parent's options
   * @return {ModelOptions} a new `ModelOptions` object initialized with the given options.
   */

Given that, the order in which createChild is called will need to start with the lowest precedence. Using createChild also hinges on the object types of the fallbacks as well. Unless you voice otherwise, I am going to continue to working on creating a PR.

Any insight/gotchas that you know of off the top of you head as I familiarize myself with the code would be appreciated.

[dgsmith2]

The first step I'm taking is to address the promise rejections, but I've hit a bit of a wall and could use some help. (I'm trying to tackle the modal tests right now).

In $modal.open the modalInstance object is created with the property result: modalResultDeferred.promise. This is one of the places rejections need to be addressed. I have it changed to result: modalResultDeferred.promise.catch(angular.identity). This solves the failures due to a missing rejection.

However, there are still some failures due to the way a Jasmine matcher is configured. The method toBeRejectedWith takes in the result promise and chains off of it to handle pass/fail. When the defer associated to the result promise has its reject() method called, the fact I have called .catch causes the successCallback to be fired in the matcher.

I'm thinking it has to do with the fact that since I am calling catch I end up with a new promise chained from the first. Since the original promise handles/eats the rejection, when we move along the promise chain to the 'catch' promise, the one that ends up in the matcher, it is in a successful state, thus the successCallback is fired and the test fails.

Based on that, I think something needs to change in the matcher. How can I determine if a pass/fail has occurred further up a promise chain? That way all the pass/fail logic can be in the successCallback instead of split between it and an errorCallback?

[dgsmith2]

Given this behavior, I think I am heading in the wrong direction. Updating to result: modalResultDeferred.promise.catch(angular.identity) breaking the test points out that it will also break all app code that attempt to do $modalInstance.result.catch(...).

Instead, I'm now thinking that it's only the specs that need changing...