adds manualModelWatcher option #599

kwypchlo · 2016-01-08T09:20:26Z

This is a proposal draft of a functionality that speeds up forms with very complex models. It works on my application at work :)

Problem: I have model that is extremely large (over 500KB) and multiple forms that I need this model to be passed to (over 10). There is a deep watcher created for every formly-form instance (so it's over 10) that watches the same model on every digest cycle.

So I did some digging:
This deep watcher is created in the FormlyFormController and is needed to fire formly expressions (hide, validations and custom expressions). I know exactly what fields from the model can trigger these expressions to be changed but formly is watching the whole model anyway, even if I don't have any expression or validation.

Solution:
I added an option to manually define parts of model that should be watched instead of watching the whole model. This is done by passing manualModelWatcher option to formly-form. If you do that, formly will not set watch on whole model but will iterate data.watch array on every field (if there is one) and set multiple small watches.

Example usecase:
I have model that is over 500KB and all I need in my form is to watch for model.type change and show/hide couple of fields depending on that type. in my case, I would just do data: {watch: ['model.type']} on every field that defines the hideExpression and it works, yay! Instead of making a whole model deep watch, I've manually set to just watch what I need.

Tell me what you guys think. If this implementation is cool I will add docs to that :)

skosno · 2016-01-08T10:11:18Z

👍 This is a major problem for me, deep watchers are too greedy and we have BIG models in API that we consume. From my perspective small watchers on needed fields are much better than one big deep watch.
In fact even if you do not need expressions, you still get deep watcher running with every digest.

codecov-io · 2016-01-08T10:22:51Z

Current coverage is `95.98%`

Merging #599 into master will increase coverage by +0.10% as of 4dc4c86

@@            master    #599   diff @@
======================================
  Files           16      16       
  Stmts         1117    1145    +28
  Branches         0       0       
  Methods          0       0       
======================================
+ Hit           1071    1099    +28
  Partial          0       0       
  Missed          46      46

Review entire Coverage Diff as of 4dc4c86

Uncovered Suggestions

+0.35% via ...alidationMessages.js#25...28
+0.26% via ...s/formlyUsability.js#18...20
+0.26% via src/test.utils.js#38...40
See 7 more...

Powered by Codecov. Updated on successful CI builds.

kentcdodds · 2016-01-08T18:49:06Z

src/directives/formly-form.js

+      const stringWatchers = {}
+
+      angular.forEach($scope.fields, function(field, index) {
+        if (field.data && Array.isArray(field.data.watch)) {


If it's part of the core API of angular-formly, then it shouldn't go in data, but it could go in extras.

kentcdodds · 2016-01-08T18:50:47Z

This looks pretty good. Please just use extras as I mention in the inline comment and I'd prefer that this new API is tested. Thanks!

kentcdodds · 2016-01-08T18:50:59Z

P.S. Let me know when you update the PR (github wont notify me).

kwypchlo · 2016-01-10T14:39:22Z

/me pokes @kentcdodds
I've added tests and refactored my code a bit to work with fields with custom model or relative model defined by string. How does it look now? I may add some docs as a cherry on top if you want to accept this feature.

kwypchlo · 2016-01-10T14:43:06Z

I'm just not sure about my 'watcher grouping' magic. It seems to be working fine but I was wondering if it was so easy to implement why angular team didn't implement it globally in the core...

kentcdodds · 2016-01-10T15:57:52Z

Thanks for doing this. The tests are great. I wonder if it might be easier if manualModelWatcher could be a string or function itself. This way you could have a single watcher that applies to all fields. I feel like this could accomplish the need and we already have an API for specific watchers on fields.

kwypchlo · 2016-01-10T16:29:01Z

@kentcdodds it seems like a valid case, I have updated the PR to incorporate your suggestion

kentcdodds · 2016-01-10T16:32:21Z

With that, do we really need the extras watcher? We already have a mechanism for adding a watcher to a field... I'd rather avoid adding another way to do the same thing.

kwypchlo · 2016-01-10T16:40:14Z

We already have a mechanism for adding a watcher to a field

@kentcdodds we don't, that's why I wanted to be able to add watchers through extras.watch.

Scenarios:

if you do not turn manualModelWatcher on you will end up with a deep model watcher.
if you switch 'manualModelWatcher' to true, the deep model watcher will not get created and you will be able to define number of custom watchers like [model.type, () => model.type, 'model.collection']. This way you do not have the overhead of watching whole model if you know that your form field should only react to couple of changes
if you define 'manualModelWatcher' as a function you can have additional form watcher that is not dependant on any field, for example manualModelWatcher: () => ctrl.loading but you still would like to define small watches on your fields just like on the second scenario

kentcdodds · 2016-01-10T16:43:54Z

Right. What I'm saying is for that second bullet point, you can use the watcher property to add custom watchers. It basically does the same thing as the watch is extras

kwypchlo · 2016-01-10T16:47:55Z

@kentcdodds ok this is a gamechanger... why did I even work on this if this was already available 👻

kwypchlo · 2016-01-10T16:51:07Z

@kentcdodds OH WAIT! You have to manually define listener for each of these expressions in your implementation of watcher property. What I propose is that you just define expressions that should be checked to run the internal functions in formly. I do not need any callback for that.

These two seem to be similar but in fact do 2 different things.

kentcdodds · 2016-01-10T16:51:47Z

Sorry. I guess I didn't understand your problem well enough. I still think that it would be valuable to be able to disable the deep watch and provide a custom watch function for the whole form.

kentcdodds · 2016-01-10T16:52:49Z

Ah, indeed... Hmmm.... I wonder if there's a way we could enhance the existing API to account for the use case

kwypchlo · 2016-01-10T17:02:43Z

@kentcdodds well, I might try but it will be more complicated. I however agree that adding extras.watch when we already have watcher property will be confusing for developers - 'which one should I use?' Worst case scenario we could rename extras.watch to extras.manualModelWatcher to make these two functionalities separate but that's a cheap trick 🌴

Btw your api is broken for a field with custom model and string expression in watcher. model in the expression will be referenced by the global model, not the one from your field. I fixed this case in my PR for my watchers by using $parse.

kwypchlo · 2016-01-10T18:19:21Z

We could consider doing 2 things:

add watcher.runFieldExpressions property to single watcher from watcher array
empty listener watcher would not throw error if runFieldExpressions is set to true and expression is defined

This would mean that instead of:

{
  ...
  extras: {
    watch: ['model.type']
  }
}

we would have:

{
  ...
  watcher: [{
    expression: 'model.type',
    runFieldExpressions: true
  }]
}

That said, this can be done but it may seem a bit confusing. It could be less confusing if runFieldExpressions would be true by default but this would mean that all the people that used watch expressions with listeners and did not expect them to run the field expressions will get field expressions running (this adds a bit of performance overhead but well... it isn't that bad of idea to run the field expressions on custom watchers).
WIth that in mind we would end up with:

{
  ...
  watcher: [{expression: 'model.type'}]
}

HOWEVER this still doesn't look right. For me, this watcher feature would seem to be overloaded with second functionality that doesn't quite fit in there. I'm wondering if mixing these two is a good idea and I'm even closer to saying that we should rename extras.watch to extras.manualModelWatcher and it will seem like a valid feature.

@kentcdodds what do you think? I'm torn apart :) One way or the other, I'd like this feature pushed as soon as possible because it gives enormous boost to me app at work.

kentcdodds · 2016-01-11T14:24:17Z

That's probably a little bit too magical I think. Maybe if we added another property to options that was watchAllExpressions in addition to disableModelWatcher or something like that. I just want the API to describe what's going on.

kwypchlo · 2016-01-11T15:00:41Z

@kentcdodds that sounds fine, I will let you know when I'm ready

kwypchlo · 2016-01-16T14:51:01Z

Ok @kentcdodds, I think I did it. However I went a little overboard so we might consider reverting one functionality but more on it later. Basically I did:

add optional manualModelWatcher to form config - it can be function or boolean - it disables global model watchers for that form
add optional watchAllExpressions to form config - when turned on, formly will add a deep watcher for each expression (templateExpressions and hideExpression). This option really makes sense only when manualModelWatcher is enabled but I didn't know how to validate it in apiCheck so we may let it be just optional or you may help me out with this conditional apiCheck validation
add optional runFieldExpressions to existing watcher functionality. If runFieldExpressions is enabled, it will run all field expressions after the watcher listener is finished. Also, now the listener property is optional due to fact that it can be omitted when runFieldExpressions is turned on. It could be validated in apiCheck but again, I didn't know how to do that.
renamed internal function refrencesCurrentlyWatchedModel to referencesCurrentlyWatchedModel - it had typeo
fixed hideExpression to be evaluated in the same model context that templateProperties are. This is breaking change but if you had nested or custom model on a field you had different evaluation context for basic expressionProperties (model referenced correct field model) and for hideExpression (model referenced always the model that was passed into form, not the one that the field defines). That being said, I can revert this if this is "too much" :)

I have written some tests and smoke tested these features on my work project and it works fine.
On very complex page with numerous forms, it turned out I have 2 hideExpressions and 1 expressionProperty which takes around 0.05ms in total for each digest cycle :) Not bad I'd say.

kentcdodds · 2016-01-17T04:46:02Z

Haven't looked at the code yet, but I can tell you right now that there's a reason that hideExpression isn't evaluated the same as expressionProperties and that's because a common scenario for the hide property. That is: If a field is initialized as hide: true, then it will not be compiled. Therefore there will be no scope to evaluated this expression and the field will never be shown (because the expression can't be evaluated).

So, if you could removed that piece, then I'll give the code a look. Let me know if you have any questions. Thanks a ton!

kwypchlo · 2016-01-17T08:39:12Z

@kentcdodds I know about that reason (however this is valid only when you ng-if the field when hide: true, it could work fine if you use ng-hide for that) but it's not about the scope, it's just about what the model property is referring to (idea seems simple enough and works fine). If you could take a look at this anyway and worst case scenario I change it back to what it was.

example:

our model: {
  game: 'scrabbles',
  player: {
    name: 'Karol',
    type: 'casual'
  }
}

This is how it works in the current latest formly release:

{
  key: 'name',
  model: 'model.player',
  expressionProperties: {
    'templateOptions.label': 'model.type + " player name"'
  },
  hideExpression: 'model.player.type === "casual"'
}

This is how it works with my PR (notice model in hideExpression refers to model.player)

{
  key: 'name',
  model: 'model.player',
  expressionProperties: {
    'templateOptions.label': 'model.type + " player name"'
  },
  hideExpression: 'model.type === "casual"'
}

kentcdodds · 2016-01-19T04:03:09Z

If you wanted to fix the model property in those expressions, you could add to this object. Would require a breaking change though. Feel free to do that (though make sure your commit message follows the convention)

kwypchlo · 2016-01-19T08:34:12Z

@kentcdodds the problem is that this object is used to evaluate the field.model itself so if I do:

return {
  model: field.model || $scope.model,
  options: field,
  index,
  formState: $scope.options.formState,
  formId: $scope.formId,
}

It will evaluate the model the wrong way in initModel when field.model is a string. It seems that if I do:

return {
  model: angular.isObject(field.model) ? field.model : $scope.model,
  options: field,
  index,
  formState: $scope.options.formState,
  formId: $scope.formId,
}

it all works fine but it seems a little bit hacky to me... it would take $scope.model as a model until the field.model is initialized (if it is defined as a string). What do you think?

Anyway, I will revert the fix for model property for now and I will leave this PR just with the watcher stuff and push the model fix in another PR when this gets accepted.
How does the code look to you besides of that issue?

kentcdodds · 2016-01-19T13:49:11Z

src/directives/formly-form.js

          $scope.$watch(() => field.model, onModelOrFormStateChange, true)
          watchedModels.push(field.model)
        }
      })
    }

+    function setupExpressionWatchers(field, index) {


This only sets up a watcher for hideExpression. Perhaps the function should be called setupHideExpressionWatcher

sure, I have refactored the name

kentcdodds · 2016-01-19T13:51:26Z

This is excellent @kwypchlo. Thank you. If you could address my one comment, I give this a 👍. I would like someone from @formly-js/angular-formly-collaborators and another person from @formly-js/angular-formly-collaborators-read to also review this PR and give a 👍 or 👎

- add manualModelWatcher option that accepts boolean or custom watcher function for more flexibilty - add watchAllExpressions option (boolean) that puts angular watcher on every field expression - add runFieldExpressions option to custom field watcher that runs field expressions on expression change

kwypchlo · 2016-01-19T15:23:19Z

@kentcdodds cool! When this is accepted I could prepare the fix for the model reference in hide expression and watchers.

kamilkisiela · 2016-01-19T16:04:41Z

@kwypchlo Great job!

@kentcdodds 👍

kentcdodds · 2016-01-19T20:30:15Z

Thanks @kamilkisiela! I'll go ahead and merge this in then.

adds manualModelWatcher option

kentcdodds · 2016-01-19T20:30:42Z

This will be auto-released in a few minutes. Thanks a bunch @kwypchlo!

kwypchlo · 2016-01-19T21:46:55Z

Cool! :)

kwypchlo force-pushed the cheap-model-watcher branch 2 times, most recently from f4a1836 to 94215a3 Compare January 8, 2016 10:07

kwypchlo force-pushed the cheap-model-watcher branch from 94215a3 to 7c76daa Compare January 8, 2016 10:21

benoror added the needs investigation label Jan 8, 2016

kentcdodds reviewed Jan 8, 2016
View reviewed changes

kwypchlo force-pushed the cheap-model-watcher branch 7 times, most recently from 1f24080 to 1c7bc62 Compare January 10, 2016 14:34

kwypchlo force-pushed the cheap-model-watcher branch from 1c7bc62 to 6551b1d Compare January 10, 2016 15:56

kwypchlo force-pushed the cheap-model-watcher branch 4 times, most recently from 5c125fc to 61dfd04 Compare January 16, 2016 13:47

kwypchlo force-pushed the cheap-model-watcher branch from 8c0bb8b to 4b5fca3 Compare January 16, 2016 15:18

kwypchlo force-pushed the cheap-model-watcher branch 4 times, most recently from eb3f020 to 10905d6 Compare January 19, 2016 10:31

kentcdodds reviewed Jan 19, 2016
View reviewed changes

kwypchlo force-pushed the cheap-model-watcher branch from 10905d6 to 8167abd Compare January 19, 2016 15:21

benoror mentioned this pull request Jan 19, 2016

formly: agGrid circular-json issue (junctionTable) panaxit/panaxui-angularjs#74

Closed

kentcdodds pushed a commit that referenced this pull request Jan 19, 2016

Merge pull request #599 from kwypchlo/cheap-model-watcher

a983c18

adds manualModelWatcher option

kentcdodds merged commit a983c18 into formly-js:master Jan 19, 2016

kwypchlo deleted the cheap-model-watcher branch January 19, 2016 21:43

kwypchlo mentioned this pull request Feb 18, 2016

fix relative string models in hideExpression and watchers #639

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds manualModelWatcher option #599

adds manualModelWatcher option #599

kwypchlo commented Jan 8, 2016

skosno commented Jan 8, 2016

codecov-io commented Jan 8, 2016

kentcdodds Jan 8, 2016

kentcdodds commented Jan 8, 2016

kentcdodds commented Jan 8, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 11, 2016

kwypchlo commented Jan 11, 2016

kwypchlo commented Jan 16, 2016

kentcdodds commented Jan 17, 2016

kwypchlo commented Jan 17, 2016

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

kentcdodds Jan 19, 2016

kwypchlo Jan 19, 2016

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

kamilkisiela commented Jan 19, 2016

kentcdodds commented Jan 19, 2016

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

adds manualModelWatcher option #599

adds manualModelWatcher option #599

Conversation

kwypchlo commented Jan 8, 2016

skosno commented Jan 8, 2016

codecov-io commented Jan 8, 2016

Current coverage is 95.98%

Uncovered Suggestions

kentcdodds Jan 8, 2016

Choose a reason for hiding this comment

kentcdodds commented Jan 8, 2016

kentcdodds commented Jan 8, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kentcdodds commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kwypchlo commented Jan 10, 2016

kentcdodds commented Jan 11, 2016

kwypchlo commented Jan 11, 2016

kwypchlo commented Jan 16, 2016

kentcdodds commented Jan 17, 2016

kwypchlo commented Jan 17, 2016

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

kentcdodds Jan 19, 2016

Choose a reason for hiding this comment

kwypchlo Jan 19, 2016

Choose a reason for hiding this comment

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

kamilkisiela commented Jan 19, 2016

kentcdodds commented Jan 19, 2016

kentcdodds commented Jan 19, 2016

kwypchlo commented Jan 19, 2016

Current coverage is `95.98%`