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

$watch - group watcher callbacks that have the same watch expression and context #13731

Open
kwypchlo opened this issue Jan 10, 2016 · 5 comments

Comments

@kwypchlo
Copy link
Contributor

I was wondering if you guys ever considered grouping watchers that have same watch expression and context into one watcher with multiple concurrent callback. I have implemented it for angular-formly and I'm wondering if this was good idea and maybe I missed something.

Check the example below how I've done it manually. It could be probably implemented in the core in similar way. Of course there are some holes as it should know if the watcher is deep or shallow and I didn't implement unwatching of single watcher because I had no use case for that but it all could be managed.

const groupedWatchers = []

// iterate through all form fields
angular.forEach($scope.fields, function(field, index) {
  // check if this field has watchers
  if (field.extras && Array.isArray(field.extras.watch)) {
    // iterate through field watchers
    angular.forEach(field.extras.watch, function(expression) {
      // model can be global modal (from $scope) or one that is manually passed to field (field.model)
      const model = field.model || $scope.model

      // find an already existing watcher with the same expression and context
      let watcherGroup = groupedWatchers.find((watcher) => {
        return watcher.expression === expression && watcher.model === model
      })

      // if this is unique or first of it's kind expression, create a group for it
      if (!watcherGroup) {
        watcherGroup = {model, expression, callbacks: []}
        groupedWatchers.push(watcherGroup)
      }

      // if this is string expression and the field overrides model, parse this expression to use model from field
      if (angular.isString(expression) && field.model) {
        watcherGroup.parsedExpression = $parse(expression).bind(null, $scope, {model: field.model})
      }

      // push a callback to callbacks list
      watcherGroup.callbacks.push(runFieldExpressionProperties.bind(null, field, index))
    })
  }
})

// iterate through watcher groups
angular.forEach(groupedWatchers, function(watcher) {
  // set up single watcher for each group
  $scope.$watch(expression, function manualFieldModelWatcher() {
    // run registered callbacks one by one
    angular.forEach(watcher.callbacks, (callback) => callback())
  }, true)
})
@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

This sounds like an interesting idea (not without some caveats though).

Unfortunately, the $watch mechanism has grown to be quite more complex than it seems from the outside. There are several moving parts that might complicate things (e.g. deep vs shallow watch, one-time-bindings, $$watchDelegates etc). It probably won't be a trivial change.

Further more, there would be some watcher registration overhead (having to loop through all watchers and compare the expressions), especially on scopes with many watchers. The good thing is that there is no global "registry" of watchers, rather every scopemaintains its own list, so the watchers are already grouped per context (context --> scope).

It would be definitely interesting if we could have some benchmarks with a prototype implementation and see if it's indeed an improvement.

@kwypchlo
Copy link
Contributor Author

@gkalpak I might try to come up with some prototypical implementation in my spare time if this sounds like a valid idea. I was wondering if I am first one to come up with it or maybe it was discussed internally at some point. I cannot give eta on this but I'll try to make some time :)

@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

I'm not aware of any such discussion. Maybe @petebacondarwin or @lgalfaso now something more :)

@lgalfaso
Copy link
Contributor

it is not the first time this idea came up. In fact, a similar idea is what triggered the Angular 2 watch tower implementation. At the time (this was some time ago, so the ideas/conclusions can be revisited) the ideas were that this should work automagically. This is, if the same expression is registered multiple times, then the expression would be evaluated less times. I am skipping many details as the conversation was extremely long and involved several design docs that are present in the Angular 2 design docs folder. At the time, the conversation revolved around performance and not making things easier for the developer, as it is strait-forward to implement the idea of one expression to multiple listeners if performance is not an issue.

The conclusion was that, if performance was not an issue, then it was better not to add any extra complexity to the core. If performance is an issue, then to do it right, it would need some BC that would be better to avoid.

@kwypchlo
Copy link
Contributor Author

@lgalfaso well the idea seems to be very simple so I was wondering what kept you from implementing it. I will check out the design docs and the Angular 2 watch tower you are referring to. I might try to implement a prototype anyway :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants