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

RELEASE: Document behavior change in watchGroup in master #16024

Closed
1 of 3 tasks
Narretz opened this issue May 31, 2017 · 3 comments
Closed
1 of 3 tasks

RELEASE: Document behavior change in watchGroup in master #16024

Narretz opened this issue May 31, 2017 · 3 comments
Assignees
@jbedard
Labels
component: scopes severity: confusing type: chore type: docs
Milestone
1.7.0

Comments

@Narretz
Copy link
Contributor

Narretz commented May 31, 2017 •
edited
Loading

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:
In 1.6, a watchGroup, every expression is watched individually, and this means the oldValue in the actionFn only changes if the underlying expression has changed. This means if expression 2 changes, but expression 1 does not, in the next actionFn invocation, the oldVal / newVal for expression 1 will be the same as in the last invocation. This is logical, but unexpected since watch and watchCollection don't have this behavior. It also makes it impossible to see which expression has changed before the current invocation.

In master, the behavior is consistent with watch / watchCollection in a way that the oldVal / newVal diff is based on the invocation of the actionFn. However, now the oldVal can be the same as the newVal once the expression has changed at least once, which will never happen in watchC or watch after the first invocation.

This was changed accidentially (I assume) in c2b8fab

The validity of the 1.6 behavior has been discussed and approved here #8671 (comment)

Expected / new behavior:
Either:

  • revert the change
  • mark the new behavior as intended.
    • This needs new tests (the commit didn't break any tests, so there must be some missing)
    • identying possible BCs and workarounds
    • an update of the changelog

Minimal reproduction of the problem with instructions:
http://plnkr.co/edit/sFO6B2zUhRwwF9bkrxe3?p=preview

Angular version: master

Browser: [all | Chrome XX | Firefox XX | Edge XX | IE XX | Safari XX | Mobile Chrome XX | Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

Anything else:
@Narretz Narretz added this to the 1.7.0 milestone May 31, 2017
@gkalpak
Copy link
Member

gkalpak commented Jun 2, 2017

FWIW, here are my thoughts on this:

  • I believe the new behavior (introduced in c2b8fab) is slightly better/more intuitive.
  • No behavior is 100% correct (unless we define it to be 😃).
  • Both behaviors will (in a sense) violate one of $watch guarantees (ignoring the first call, which is special):
    1. The listener will only be called if there has been a change.
    2. The old value will always be different than the new value.

More on that last point:
With the old behavior, the listener was called with the last old/new pair for each value (as in the old/new pair for the last time that particular value changed). With the new behavior, the listener is called wth the current values as "new" and the values from the last call of the listener as "old".

In the first case (old behavior), you are not able to tell which value has changed. In the second case (new behavior) you are able to tell, by comparing the old and new values.

All that being said, when using $watchGroup you shouldn't care about the old value. You just care about when any of the values changes and what the latest values are. Both behaviors (old and new) cover these requirements (they only fire the listener when there has been a change and the "new" values are up-to-date).

The only valid usecase I can think of where the "old" values are important, is when you want to calculate the "diff" (i.e. what has changed wrt to the combined result of all the values). In that case, where for example you need to combine the "old" values and the "new" values and compare the results of each combination, the new behavior makes more sense.

In any case, I don't feel strongly about going any direction (since both have their pros and cons and both are good enough for most usecases involving $watchGroup()).

@jbedard jbedard self-assigned this Jun 12, 2017
@gkalpak gkalpak mentioned this issue Jun 29, 2017
Closed
3 tasks
@Narretz
Copy link
Contributor Author

Narretz commented Nov 30, 2017

The 1.6.x docs update has been added in #16005, and we've changed the behavior for 1.7.0. Technically we can close this. But I wonder why this change made it only in 1.7? If it's no BC then it could go into 1.6.x. If it's technically a BC, we need to add a BC notice in the changelog.

@gkalpak
Copy link
Member

gkalpak commented Nov 30, 2017

I think we agreed the new behavior is better and yes it is technically a BC. It would be good to add a mention in the changelog. This issue is for tracking that 😛:

Either:

  • revert the change
  • mark the new behavior as intended.
    • This needs new tests (the commit didn't break any tests, so there must be some missing)
    • identying possible BCs and workarounds
    • an update of the changelog

We should also add some tests, indeed.

@Narretz Narretz changed the title Behavior change in watchGroup in master Document behavior change in watchGroup in master Nov 30, 2017
@Narretz Narretz mentioned this issue Feb 5, 2018
Closed
3 tasks
@petebacondarwin petebacondarwin changed the title Document behavior change in watchGroup in master RELEASE: Document behavior change in watchGroup in master Feb 19, 2018
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 18, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
96d7438 
Closes angular#16024
@jbedard jbedard mentioned this issue Mar 18, 2018
Merged
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 19, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
87c5785 
Closes angular#16024
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 19, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
96dcafe 
Closes angular#16024

BREAKING CHANGE

(caused by c2b8fab)

Previously the entries in `newValues` and `oldValues` reflected the
differences in each *individual* espression, and not the difference of the
values between each call of the listener.

Now the entries in `oldValues` will equal the `newValues` of the previous
call of the listener. This means comparing the entries in `newValues` and
`oldValues` can be used to determine which individual expressions changed.
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 28, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
aebf6b0 
Closes angular#16024

BREAKING CHANGE

(caused by c2b8fab)

Previously the entries in `newValues` and `oldValues` reflected the
differences in each *individual* espression, and not the difference of the
values between each call of the listener.

Now the entries in `oldValues` will equal the `newValues` of the previous
call of the listener. This means comparing the entries in `newValues` and
`oldValues` can be used to determine which individual expressions changed.

For example `$scope.$watchGroup(['a', 'b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [1, undefined] |

Now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [2, undefined] |

Note the last call now shows `a === 2` in the `oldValues` array.

This also makes the `oldValue` of one-time watchers more clear. Previously
the `oldValue` of a one-time watcher would remain `undefined` forever. For
example `$scope.$watchGroup(['a', '::b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [undefined, undefined] |
|  `a=b=3` | [3, 2] | [1, undefined] |

Where now the `oldValue` will will lways equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [1, undefined] |
|  `a=b=3` | [3, 2] | [1, 2] |
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 28, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
a8ddb03 
Closes angular#16024

BREAKING CHANGE:

(caused by c2b8fab)

Previously when using `$watchGroup` the entries in `newValues` and
`oldValues` represented the *most recent change of each entry*.

Now the entries in `oldValues` will always equal the `newValues` of the previous
call of the listener. This means comparing the entries in `newValues` and
`oldValues` can be used to determine which individual expressions changed.

For example `$scope.$watchGroup(['a', 'b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [1, undefined] |

Now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [2, undefined] |

Note the last call now shows `a === 2` in the `oldValues` array.

This also makes the `oldValue` of one-time watchers more clear. Previously
the `oldValue` of a one-time watcher would remain `undefined` forever. For
example `$scope.$watchGroup(['a', '::b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [undefined, undefined] |
|  `a=b=3` | [3, 2] | [1, undefined] |

Where now the `oldValue` will will lways equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [1, undefined] |
|  `a=b=3` | [3, 2] | [1, 2] |
@jbedard jbedard mentioned this issue Mar 28, 2018
Closed
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 30, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
8f13833 
Closes angular#16024

BREAKING CHANGE:

(caused by c2b8fab)

Previously when using `$watchGroup` the entries in `newValues` and
`oldValues` represented the *most recent change of each entry*.

Now the entries in `oldValues` will always equal the `newValues` of the previous
call of the listener. This means comparing the entries in `newValues` and
`oldValues` can be used to determine which individual expressions changed.

For example `$scope.$watchGroup(['a', 'b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [1, undefined] |

Now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [2, undefined] |

Note the last call now shows `a === 2` in the `oldValues` array.

This also makes the `oldValue` of one-time watchers more clear. Previously
the `oldValue` of a one-time watcher would remain `undefined` forever. For
example `$scope.$watchGroup(['a', '::b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [undefined, undefined] |
|  `a=b=3` | [3, 2] | [1, undefined] |

Where now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [1, undefined] |
|  `a=b=3` | [3, 2] | [1, 2] |
jbedard added a commit to jbedard/angular.js that referenced this issue Mar 31, 2018
@jbedard
test($rootScope): add tests clarifying $watchGroup oldValues behavior
5d5f411 
Closes angular#16024

BREAKING CHANGE:

(caused by c2b8fab)

Previously when using `$watchGroup` the entries in `newValues` and
`oldValues` represented the *most recent change of each entry*.

Now the entries in `oldValues` will always equal the `newValues` of the previous
call of the listener. This means comparing the entries in `newValues` and
`oldValues` can be used to determine which individual expressions changed.

For example `$scope.$watchGroup(['a', 'b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [1, undefined] |

Now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `a=2`   | [2, undefined] | [1, undefined] |
|  `b=3`   | [2, 3] | [2, undefined] |

Note the last call now shows `a === 2` in the `oldValues` array.

This also makes the `oldValue` of one-time watchers more clear. Previously
the `oldValue` of a one-time watcher would remain `undefined` forever. For
example `$scope.$watchGroup(['a', '::b'], fn)` would previously:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [undefined, undefined] |
|  `a=b=3` | [3, 2] | [1, undefined] |

Where now the `oldValue` will always equal the previous `newValue`:

|  Action  |  newValue  |  oldValue  |
|----------|------------|------------|
|  (init)  | [undefined, undefined] | [undefined, undefined] |
|  `a=1`   | [1, undefined] | [undefined, undefined] |
|  `b=2`   | [1, 2] | [1, undefined] |
|  `a=b=3` | [3, 2] | [1, 2] |
Narretz pushed a commit that referenced this issue Apr 3, 2018
@jbedard @Narretz
test($rootScope): add tests clarifying $watchGroup oldValues behavior
4492db3 
Closes #16024 
Closes #16494
See also #16514
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jbedard jbedard

Labels
component: scopes severity: confusing type: chore type: docs
Projects
None yet
Milestone
1.7.0
Development

No branches or pull requests
4 participants
@petebacondarwin @jbedard @Narretz @gkalpak