Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat($rootScope): allow $watch to be triggered by an event #14798

Closed
odedniv wants to merge 1 commit into angular:master from odedniv:watch-on

Conversation

@odedniv
Copy link

@odedniv odedniv commented Jun 19, 2016

Note: I did not yet add documentation/tests because I first want to know if this actually makes sense to do this.

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
feat

What is the current behavior? (You can also link to an open issue here)
Watch expressions are always evaluated on every digest.

What is the new behavior (if this is a feature change)?
Allow $watch to be evaluated when an event is triggered (using $on), with a new syntax "eventName: watchExpression".

Example:

HTML:

<div ng-if="sessionChanged: currentUser">
 <!-- Below doesn't really work yet since I don't know where that is -->
 Hi, {{sessionChanged: currentUser.username}}
</div>

JavaScript:

$rootScope.currentUser = { username: "..." };
$rootScope.$broadcast('sessionChanged');

Does this PR introduce a breaking change?
No.

Other information:
This is actually more of a suggestion/question: I read everywhere that watchers are bad, but there is no true solution for this apart from writing directives that basically destroy the templating concept. I understand that using events ($on) should be better, and so I figured it might be a good idea to patch into $watch (which is used everywhere, ng-if etc) the ability to be triggered by an event rather than adding it to the digest cycle.

Allow $watch to support a new syntax of "eventName: watchExp", in
addition to the old syntax of just "watchExp". This means it will not be
added as a regular watcher but rather use $on to decide on when to
evaluate the watchExp and execute the listener.
Since watchers are wasteful and run every digest cycle, its a good idea
to allow the developer to be more mindful and decide when the watcher is
evaluated (including ng-if, ng-class, etc).
Copy link
Member

gkalpak commented Jun 21, 2016

This seems similar to #6354.

Copy link
Author

odedniv commented Jun 22, 2016

@gkalpak they seem to offer a different implementation, asking the community if this makes sense, plus I have a simple implementation ready 😄

Copy link
Contributor

lgalfaso commented Jun 22, 2016
edited
Loading

I would like to know if it is necessary for this to be part of the core. Can this be done thru a third party module? Eg https://github.com/kseamon/fast-bind (or many others)

Copy link
Author

odedniv commented Jun 24, 2016

Well to do this I had to override $rootScope.$watch. Do third party modules usually do this? Kind of compatibility scary.

I already implemented it as a patch first and only then made the angular pull request, so either way is fine by me. Most important for me is hearing your thoughts about it: should it really make it better? Is it implemented properly?

Copy link
Author

odedniv commented Jun 24, 2016

  • I don't really understand the reference (fast-bind), there is no documentation of what it does or how it works, and it looks like they just copied the entire angular code base and left it for 2 years...

This is a good of example of something no-one should do when creating a third party plug in, I don't want to do that... Do you have any other example?

Copy link
Contributor

If you look into http://ngmodules.org/ there are several solutions for this specific issue. Many of these have some shortcomings, and this is why there are some existing PRs that try to solve this issue in different ways. There is #10096 that would be a more generic implementation of what it is proposed here and there is #13524 that would allow a different implementation thru a third party library as this (quite old) RFC

Copy link
Author

odedniv commented Jun 27, 2016

So... can someone advise whether this is a good performance change at all? (whether it's core or third party)

@gkalpak gkalpak modified the milestones: Ice Box, Backlog Jun 27, 2016
Copy link
Member

gkalpak commented Jun 27, 2016

I think this would improve performance in specific usecases. But @lgalfaso is right, this is not something that should be incorporated into core, since this is easily implemented indepently and actually it doesn't need to be tied to scope watchers.

Copy link
Author

odedniv commented Jun 27, 2016

@gkalpak how can it be implemented without patching scope watches? Can you give an example? All I can think of is a directive that recompiles the entire element when an event triggers...

Copy link
Member

gkalpak commented Jun 27, 2016

@odedniv , this is not relying on anything that is not available outside core.

Basically, all you need is an extra method on the Scope prototype that takes an eventName, an expression to $parse, a listener to call when the expression value changes and optionally a 4th argument if you need to support "deep-watching".

Here is a simplistic POC.

Copy link
Contributor

Closing as this can be be achieved outside core easily.

Copy link
Author

odedniv commented Nov 23, 2016

@petebacondarwin this can only be achieved by overriding $watch, not really "outside core" and not really "easily"... unless you know of another way to do it.

I want ng-if to not add a watcher, but subscribe to a broadcast, do you have an example of how to do it without rewriting ng-if (and a ton of other directives).

Copy link
Contributor

@odedniv - perhaps you are right that it is difficult to prevent the watch functions from running on every digest in this way without changing core (or at least patching core). But then decorating RootScope.prototype.$watch is not outside of the scope of what we allow developers to do, hence the module.decorate() method.

Copy link
Author

odedniv commented Nov 24, 2016

@petebacondarwin $watch is not a factory but a scope function, so it cannot be decorated with module.decorator (if that's what you meant). Also, if you'll check the way I implemented it, I need some non-trivial code which in my app I now have to pretty much copy and hope it won't change (like the watchEquals I extracted for reuse, and the initWatchVal).

This is how my app's code looks now (since this PR I added multi-events functionality, CoffeeScript generated):

angular.module('myApp').run([
 '$rootScope', '$parse', function($rootScope, $parse) {
 var $watch, WATCH_ON_PATTERN;
 WATCH_ON_PATTERN = /^*[a-zA-Z][a-zA-Z]+:/;
 $watch = $rootScope.$watch;
 return $rootScope.$watch = function(watchExp, listener, objectEquality) {
 var evaluate, eventNames, events, get, initialValue, last, statement;
 if (typeof watchExp === 'string' && watchExp.match(WATCH_ON_PATTERN)) {
 if (!angular.isFunction(listener)) {
 listener = angular.noop;
 }
 statement = watchExp.split(':');
 eventNames = statement[0];
 get = $parse(statement.slice(1).join(':'));
 last = initialValue = {};
 evaluate = (function(_this) {
 return function() {
 var reallyLast, value;
 value = get(_this);
 if (value !== last && !(objectEquality ? angular.equals(value, last) : typeof value === 'number' && typeof last === 'number' && isNaN(value) && isNaN(last))) {
 reallyLast = last;
 last = objectEquality ? angular.copy(value, null) : value;
 return listener(value, (reallyLast === initialValue ? value : reallyLast), _this);
 }
 };
 })(this);
 evaluate();
 events = [];
 eventNames.split(' ').forEach((function(_this) {
 return function(eventName) {
 eventName = eventName.trim();
 if (eventName) {
 return events.push(_this.$on(eventName, evaluate));
 }
 };
 })(this));
 return (function(_this) {
 return function() {
 return events.forEach(function(event) {
 return event();
 });
 };
 })(this);
 } else {
 return $watch.apply(this, arguments);
 }
 };
 }
 ])

Not exactly pretty on the eyes...

Copy link
Contributor

It doesn't have to be that bad - especially if you don't rely on CoffeeScript :-) - and you can put it in a decorator that can be distributed as a separate module.

See http://plnkr.co/edit/YJuqVXfj66kBw2jm0xyO?p=preview

Copy link
Author

odedniv commented Nov 24, 2016

That looks just as bad... the copy-pasted logic is my issue with it, not the function declarations. My CoffeeScript version looks fine (I don't care what the translated JavaScript looks like), I just didn't want to properly translate it to prettier JavaScript for the sake of the conversation.

angular.module('myApp').run(['$rootScope', '$parse', ($rootScope, $parse) ->
 # Starts with a letter, then only letters and spaces until :
 # Copied to: directives/alias.coffee
 WATCH_ON_PATTERN = /^ *[a-zA-Z][a-zA-Z ]+:/
 $watch = $rootScope.$watch
 $rootScope.$watch = (watchExp, listener, objectEquality) ->
 if typeof watchExp == 'string' and watchExp.match(WATCH_ON_PATTERN)
 listener = angular.noop if not angular.isFunction(listener)
 statement = watchExp.split(':')
 eventNames = statement[0]
 get = $parse(statement.slice(1).join(':'))
 last = initialValue = {}
 evaluate = =>
 value = get(@)
 if value != last and not (if objectEquality then angular.equals(value, last) else (typeof value == 'number' && typeof last == 'number' and isNaN(value) and isNaN(last)))
 reallyLast = last # before updating
 last = if objectEquality then angular.copy(value, null) else value
 listener(value, (if (reallyLast == initialValue) then value else reallyLast), @)
 evaluate()
 events = []
 eventNames.split(' ').forEach (eventName) =>
 eventName = eventName.trim()
 events.push(@$on(eventName, evaluate)) if eventName
 => events.forEach((event) => event())
 else
 $watch.apply(@, arguments)

Copy link
Contributor

The complexity of the actual code is not that big a deal if it is packaged up as a 3rd party module. There is actually not that much copy-pasted logic - only really the if statement in the evaluate function.

I'll reopen this PR but I'm going to leave it in the Ice Box since it is functionality that "can" be achieved outside of core and doesn't help Angular 1 move semantically closer to Angular 2.

Copy link
Contributor

Narretz commented Jul 4, 2017

We are planning to add a different way of stopping watchers: #10658

Copy link
Author

odedniv commented Jul 4, 2017

@Narretz this PR doesn't "stop watchers". This is a syntax to let "watchers" subscribe to events instead of watching for changes.

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

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

Ice Box

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /