Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

WIP: Feat: watch ngModel with object equality via ngModelOptions #12783

Closed
wants to merge 2 commits into from

Conversation

Narretz
Copy link
Contributor

@Narretz Narretz commented Sep 8, 2015

This is a first draft for watching models with object equality.
Use cases:

  • models are collections (ngList / multiple) (also the next step after this)
  • models are objects, and only part of objects change

Notes:

  • we cannot use watch with third param set to true / watchCollection due to the way the model watch currently works. Basically, all actions happen inside the watch fn, not the action fn. The added test shows why.
  • the current implementation copies the model if the deepWatch is true, so scope model is never === $modelValue; and so changes can be detected with equals(). I'm not sure if this is the most performant way, or actually the same as having a watch with objectEquality = true. (Note: There are some PRs for making copy faster)

For ngList / (select, url, email) multiple to use deepWatch, I can think of two ways:

  • Let it set $$options.deepWatch inside the modelController to true, before the options are evaluated inside ngModelOptions. However, (even with feat(ngModelOptions): allow options to be inherited from ancestors #10922,) we would need to extend the options (once more), so that what is preset on the ctrl gets merged with the actual ngModelOptions
  • Set a special property on the controller, such as $isCollection, that tells the controller to use deepWatch. This could still be overruled if deepWatch is explicitly set to false. This is however not very obvious for people who have their own controls. ngModelOptions would centralize everything.

@Narretz Narretz added this to the 1.5.x - migration-facilitation milestone Sep 8, 2015
// options.deepWatch
// options.collection

getModelValueSetter();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using get in the name is kind of confusing, since it's not returning anything.

@Narretz Narretz force-pushed the feat-modelWatch-clean branch from 9a49432 to 0814a6d Compare September 9, 2015 10:35
@Narretz Narretz changed the title WIP: Feat: watch ngModel with object quality via ngModelOptions WIP: Feat: watch ngModel with object equality via ngModelOptions Sep 9, 2015
@petebacondarwin
Copy link
Contributor

I would like to get away from doing so much work in the watcher function.

How about we move processing of ng-change into the post digest or even $evalAsync?

If we could move everything out then we could solve this problem by letting the input directive decide what watching mechanism to use (i.e. simple watch, deep watch or watch collection).

I think to begin with the input directive ought to be responsible for making the decision about the type of model. But then in my POC, I allowed parsers and formatters to convert the model that was being passed through from simple to collection and back again by setting a flag.

@Narretz
Copy link
Contributor Author

Narretz commented Sep 22, 2015

Putting the burden on ngChange would be great. I'm just a tiny bit worrying about BC, which I wanted to avoid at this point.

Who decides what kind of watching ngModel uses is imo separated from the fact thatit has to be implemented in ngModel anyway.

@petebacondarwin
Copy link
Contributor



<form>
  <input type="text" ng-model="myArray" to-upper ng-model-options="{ deepWatch: true }">

  <select ng-model="selectedOptions" ng-options="getOptions()" multiple>


  <input type="text" ng-model="obj" converter /> 


  <input ng-model="myObj.moo">
          <!-- myObj.moo = $modelValue -->
  <input ng-model="myObj.moo" ng-model-options="{ getterSetter: true }">
          <!-- myObj.moo($modelValue) -->

  <input ng-model="myObj" address-directive ng-model-options="{ deepWatch: false }">
  <input ng-model="myObj" address-directive ng-model-options="{ deepWatch: true }">
</form>

myObj = {
  street: '',
  town: '',
  postalCode: ''
};


myArray = ['a', 'b', 'c'];


function $parser(value) {
  return value.toUppercase();
}



parser(value: Input) => Output
formatter(value: Output) => Input

modelValue : Output
viewValue : Input

$watcher(value : Output)

$parsers.push(parserObj)

parserObj.$$modelType = NgModelTypes.OBJECT;
parserObj.$$viewType = NgModelTypes.SIMPLE;


//inside a directive
modelCtrl.$setWatchType({deepWatch: true});

$setViewValue(array)


MODEL <-> PARSER/FORMATTER <-> PARSER/FORMATTER <-> VIEW (directive)
OBJECT    OBJECT - STRING      STRING  - DATE       DATE
ngModel.$isEmpty
ngModel.$viewType = OBJECT
ngModel.$deepWatch

ngModel.$updateWatcher()


<emailInput ng-multiple="isMultiple">

@dcherman
Copy link
Contributor

An even better idea may be to allow customization of the function used to test equality rather than setting it to deep: true/false.

Consider a moment object (http://momentjs.com/). I would never want to deep watch this for equality since it's needlessly expensive, I would end up writing an equality test that would use moment.isSame(other)

@petebacondarwin
Copy link
Contributor

@dcherman - I hear what you are saying. I wonder if this could be approached by allowing angular.equals, which is what is used by $watch and friends to check for deep equality, to use some $$equals() method on the object if it is available. I don't know if this has been discussed before ( @IgorMinar ?) but it is a concept that is used frequently in other languages.

There is an additional issue, though, which is that when we are not using === to test for equality we must make a clone of the object that is being watched so that we can actually tell if its internal value has changed. So there would still be this additional cost.

Another thing to consider, is that in ngRepeat we can avoid deep equality checks by providing a track by expression.

@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.6.x Nov 23, 2015
@Narretz Narretz modified the milestones: 1.6.x, Ice Box Nov 30, 2017
@Narretz Narretz closed this Mar 21, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants