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

feat($rootScope): Add custom compare and copy functions to $watch #10096

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat($rootScope): Add custom compare and copy functions to $watch #10096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions src/ng/rootScope.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,11 +353,13 @@ function $RootScopeProvider() {
* - `newVal` contains the current value of the `watchExpression`
* - `oldVal` contains the previous value of the `watchExpression`
* - `scope` refers to the current scope
* @param {boolean=} objectEquality Compare for object equality using {@link angular.equals} instead of
* comparing for reference equality.
* @param {boolean=|function()} objectEquality Compare for object equality using {@link angular.equals} instead of
* comparing for reference equality. If a function is passed, it will be used as a replacement for {@link angular.equals}.
* @param {function()} copier Replacement function for {@link angular.copy} to save value into oldVal for next
change event.
* @returns {function()} Returns a deregistration function for this listener.
*/
$watch: function(watchExp, listener, objectEquality) {
$watch: function(watchExp, listener, objectEquality, copier) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a check:

if (isFunction(objectEquality) && !isFunction(copier)) {
  throw $rootScopeMinErr('eq-cpy',
      'A custom `objectEquals` function must be accompanied by a custom `copier` function');

Copy link
Member

Choose a reason for hiding this comment

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

Why is that ? What is wrong with using default copy() with a custom objectEquality() ?
I will usually not make sense, but why should it be our concern ? 😁

Copy link
Contributor

Choose a reason for hiding this comment

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

To make it clear that if you are going to mess with the way equality works you also have to understand that copy will likely need to change too

Copy link
Member

Choose a reason for hiding this comment

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

Aha, makes sense !
Or we can force them tick a checkbox that says "I have read and understood the implications" 😁

It would be useful to add more docs about it and how to use the feature properly (maybe an example too). I guess something tbd before merging.

I generally like the idea.

Any benchpress experts around ? Would be useful to make sure that performance isn't affected when you don't use tge feature (e.g. core stuff) and that it gets improved (when used correctly) for user stuff.

var get = $parse(watchExp);

if (get.$$watchDelegate) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The $watchDelegate also takes the objectEquality parameter, so this would need to be refactored too. Luckily this is an internal interface so we might be able to get away with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need an extra test for expressions that generate a watchDelegate?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I forgot about this bit. There is a quite a bit of work to do here as each of the delegates in the parser need to be updated. We definitely need to test this.

Expand All @@ -370,7 +372,9 @@ function $RootScopeProvider() {
last: initWatchVal,
get: get,
exp: watchExp,
eq: !!objectEquality
eq: !!objectEquality,
equals: isFunction(objectEquality) ? objectEquality : equals,
copy: isFunction(copier) ? copier : copy
Copy link
Contributor

Choose a reason for hiding this comment

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

if we decide to move forward, then this looks like the wrong approach. We should not add new properties, and change get and listener so they are new functions that do the custom compare and copy.

#10096 (comment) can be used as a template, but in this specific case it can be simplified by using the $watch function closure.

If implemented that way, then there is no need to change any of the delegates

};

lastDirtyWatch = null;
Expand Down Expand Up @@ -763,12 +767,12 @@ function $RootScopeProvider() {
if (watch) {
if ((value = watch.get(current)) !== (last = watch.last) &&
!(watch.eq
? equals(value, last)
? watch.equals(value, last)
: (typeof value === 'number' && typeof last === 'number'
&& isNaN(value) && isNaN(last)))) {
dirty = true;
lastDirtyWatch = watch;
watch.last = watch.eq ? copy(value, null) : value;
watch.last = watch.eq ? watch.copy(value, null) : value;
watch.fn(value, ((last === initWatchVal) ? value : last), current);
if (ttl < 5) {
logIdx = 4 - ttl;
Expand Down
55 changes: 55 additions & 0 deletions test/ng/rootScopeSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,61 @@ describe('Scope', function() {
expect(log).toEqual([]);
}));

it('should use custom equals and copy functions',
inject(function($rootScope) {
var log = [];
var oldLog = [];

function logger(newVal, oldVal) {
log.push(newVal);
oldLog.push(oldVal);
}

var watched = "ABC";
var watchedFn = function() {
return watched;
};

// Comparator returns true if string starts with same character
var comparator = function(newVal, oldVal) {
if (oldVal === newVal) return true;
if (oldVal === undefined || newVal === undefined) return false;
if (newVal.length === oldVal.length === 0) return true;
if (newVal.length > 0 && oldVal.length > 0 && oldVal[0] === newVal[0]) {
return true;
}
return false;
};

// Copier add " copy" to value
var copier = function(value) {
if (value !== undefined) {
return value + " copy";
}
return undefined;
};

$rootScope.$watch(watchedFn, logger, comparator, copier);

$rootScope.$digest();
expect(log).toEqual(['ABC']);
expect(oldLog).toEqual(['ABC']);
log = [];
oldLog = [];

watched = "DEF";
$rootScope.$digest();
expect(log).toEqual(['DEF']);
expect(oldLog).toEqual(['ABC copy']);
log = [];
oldLog = [];

watched = "DZZ";
$rootScope.$digest();
expect(log).toEqual([]);
expect(oldLog).toEqual([]);
}));


describe('$watch deregistration', function() {

Expand Down