adds manualModelWatcher option

kwypchlo commented 8 years ago

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 commented 8 years ago

:+1: 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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

kwypchlo commented 8 years ago

/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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

kentcdodds commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

kwypchlo commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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

kwypchlo commented 8 years ago

@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 :palm_tree:

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 commented 8 years ago

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 commented 8 years ago

I like your first suggestion. I think that we could implement it in a backward compatible way and I don't think that it is terribly confusing:

{
  ...
  watcher: [{
    expression: 'model.type',
    runFieldExpressions: true,
  }]
},
{
  ...
  watcher: [{
    expression: 'model.type',
    listener: () => {},
    runFieldExpressions: true,
  }]
}

I think that both of those would run the field expressions, but the second field would also run the custom listener.

I really don't see anything bad with this approach. The idea is that it totally separates disabling deep model watching (or specifying custom watcher) from custom field watchers which I think is a good call.

kwypchlo commented 8 years ago

@kentcdodds ok, sounds fine however I talked with @skosno at work and he came up with other suggestion. He suggested that we could switch to watching just the expressions rather than specifying additional watcher. In that case simple field like

{
  key: 'type',
  hideExpression: 'model.type === "some-type"'
}

would require no additional work done if you disable the deep model watcher because it would watch the expression itself.

On the other hand, there are cases where specifying single watcher for all expression properties would be beneficial ie:

{
  key: 'type',
  expressionProperties: {
    'templateOptions.label': 'model.type + " label"',
    'templateOptions.additionalOptions': 'data.getOptionsForType(model.type)',
  },
  hideExpression: 'model.type === data.someHeavyMethod()',
  extras: {
    manualModelWatcher: true
  },
  watcher: [{
    expression: 'model.type',
    runFieldExpressions: true
  }]
}

In this example we would disable expressions watchers for this field and just enable custom watchers that have runFieldExpressions: true.

How does it sound?

kentcdodds commented 8 years ago

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 commented 8 years ago

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

kwypchlo commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

This is excellent @kwypchlo. Thank you. If you could address my one comment, I give this a :+1:. 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 :+1: or :-1:

kwypchlo commented 8 years ago

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

kamilkisiela commented 8 years ago

@kwypchlo Great job!

@kentcdodds :+1:

kentcdodds commented 8 years ago

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

kentcdodds commented 8 years ago

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

kwypchlo commented 8 years ago

Cool! :)

formly-js / angular-formly