Scope#$watchCollection oldValue == newValue fix #6736
Conversation
|
Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
| } else { // if object | ||
| veryOldValue = {}; | ||
| for (var key in newValue) { | ||
| if (hasOwnProperty.call(newValue, key)) { |
There was a problem hiding this comment.
Why not use shallowCopy()? do we need to hang onto $$hashKey for the veryOldValue?
There was a problem hiding this comment.
I looked at shallowCopy, but since it doesn't support arrays I didn't use it.
I haven't thought much about $-prefixed properties. I'm not sure why they should be excluded in this case. Do you have some opinions on that?
There was a problem hiding this comment.
Not really, I was just thinking it would be (marginally) less code. But it's true that it doesn't support arrays (but arrays are using a different codepath anyways), and not caring about the '$' should improve perf by some negligible amount
There was a problem hiding this comment.
I don't think it's worth it. besides the $$hashkey might actually be useful for people trying to figure out what changed.
|
@Narretz yes, I'll update the docs |
|
I think this is good to be merged (after squashing). any objections? |
|
lgtm, basically |
|
Yay! |
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue by the time we called the watchCollection listener. The fix creates a copy of the newValue each time a change is detected and then uses that copy *the next time* a change is detected. To make `$watchCollection` behave the same way as `$watch`, during the first iteration the listener is called with newValue and oldValue being identical. Since many of the corner-cases are already covered by existing tests, I refactored the test logging to include oldValue and made the tests more readable. Closes angular#2621 Closes angular#5661 Closes angular#5688 Closes angular#6736
`log.empty()` is the same as `log.reset()`, except thati `empty()` also returns the current array with messages instead of: ``` // do work expect(log).toEqual(['bar']); log.reset(); ``` do: ``` // do work expect(log.empty()).toEqual(['bar']); ```
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue by the time we called the watchCollection listener. The fix creates a copy of the newValue each time a change is detected and then uses that copy *the next time* a change is detected. To make `$watchCollection` behave the same way as `$watch`, during the first iteration the listener is called with newValue and oldValue being identical. Since many of the corner-cases are already covered by existing tests, I refactored the test logging to include oldValue and made the tests more readable. Closes angular#2621 Closes angular#5661 Closes angular#5688 Closes angular#6736
Originally we destroyed the oldValue by incrementaly copying over portions of the newValue into the oldValue during dirty-checking, this resulted in oldValue to be equal to newValue by the time we called the watchCollection listener. The fix creates a copy of the newValue each time a change is detected and then uses that copy *the next time* a change is detected. To make `$watchCollection` behave the same way as `$watch`, during the first iteration the listener is called with newValue and oldValue being identical. Since many of the corner-cases are already covered by existing tests, I refactored the test logging to include oldValue and made the tests more readable. Closes #2621 Closes #5661 Closes #5688 Closes #6736
|
johnsoftek: even with the extra write overhead, algorithmically it would be more complicated to do a set of reads and then copy if a change is detected. This might be improved a little bit, but how about we wait and see how necessary it is? That's just my opinion, though. |
|
Caitlin, Well, I had a pull request that did just that, but it seems it was not John M On 19 March 2014 10:32, Caitlin Potter [email protected] wrote:
|
|
@johnsoftek your change was too complex and didn't allow the optimization for the case when old value is not needed. we need the optimization to keep ngRepeat fast. |
|
Igor, No idea why you think my change complex. And it would be trivial to On 19 March 2014 13:37, Igor Minar [email protected] wrote:
|
|
Igor, No idea why you think my change complex. And it would be trivial to Anyway, your decision. John M On 19 March 2014 13:37, Igor Minar [email protected] wrote:
|
No description provided.