RELEASE: Add BC note for ngClass deep watch change

[Narretz]

Introduced in this commit: 189461f

Needs to be added before 1.7 is released

[jbedard]

Normally this would be done in the commit message. How should this be done when the commit already exists (and doesn't have a "BREAKING CHANGE" message...)?

[Narretz]

We need to add this to the changelog once we create it for 1.7.0 or the first beta. You can post the text here so we have it.

[jbedard]

This one was rare and a little confusing, but here's a shot at a BC message:

ng-class no longer uses deepWatch when one-time binding (::) is used. Previously ng-class required deepWatch for one-time bindings (::) to prevent infdig errors with array/object literals. This caused classes to be re-applied when the state of a nested object changed. If other sources were also changing classes this may have had a noticeable effect.

[Narretz]

Hm, it doesn't say anything about things that aren't supported anymore, does it? I.e. it should say what the devs cannot do anymore, and what they should do instead

[jbedard]

I'm starting to question if this actually effects ng-class beyond just a performance improvement from avoiding deepWatch=true.

Previously it did a deep watch if isOneTime, essentially working around what 189461f fixed. After 189461f the deepWatch=true is gone, but due to the fact that we are using an interceptor any expressions with non-primitive or changed inputs will invoke that interceptor and then proceed like usual. So I think this change had no effect on ng-class other then avoiding the expensive deepWatch=true? Or maybe I'm forgetting something 🤔

The rest of this change was allowing one-time literals to use input-watching. Previously without input-watching object/array literals would cause an infdig due to the new instance created each time. Since this case previously crashed I don't think there's any breaking change.

I do have some tests to add which fail in v1.6.x but pass in master though...

[Narretz]

As far as I can remember, this is the issue I was thinking about: #15905 - which was fixed for 1.6.5, and also still works in master. So I assume if #15960 has not been implemented on master anywhere, this issue is indeed obsolete :)

[gkalpak]

@jbedard, are you saying that the following is not true:

Previously ng-class required deepWatch for one-time bindings (::) to prevent infdig errors with array/object literals. This caused classes to be re-applied when the state of a nested object changed. If other sources were also changing classes this may have had a noticeable effect.

[jbedard]

@gkalpak correct. While the deepWatch will cause the ng-class watcher to be re-executed, I think the updateClasses avoids any unnecessary writes to the dom. So any extra changes detected due to deepWatch get stopped in updateClasses.

I would still like to add more tests around the $parse side of 189461f though.

[Narretz]

Ok, then we agree that this is not a BC. :)

[jbedard]

Here are some examples that pass in both 1.6 and 1.7. The "should not re-apply" cases are what I originally thought would re-apply in 1.6.

  fit('should not deep watch objects', inject(function($rootScope, $compile, $animate) {
    $rootScope.classVar = {orange: {}};
    element = $compile('<div ng-class="classVar"></div>')($rootScope);
    var addSpy = spyOn($animate, "addClass").and.callThrough();

    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should not re-apply class when a deep property changes
    $rootScope.classVar.orange.foo = 42;
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should re-apply class when a direct property changes
    $rootScope.classVar.apple = {};
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(element).toHaveClass("apple");
    expect(addSpy).toHaveBeenCalledTimes(2);
  }));

  fit('should not deep watch array literals', inject(function($rootScope, $compile, $animate) {
    $rootScope.classVar = {orange: {}};
    element = $compile('<div ng-class="[classVar, classVar2]"></div>')($rootScope);
    var addSpy = spyOn($animate, "addClass").and.callThrough();

    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should not re-apply class when a deep property changes
    $rootScope.classVar.orange.foo = 42;
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should re-apply class when a direct property changes
    $rootScope.classVar.apple = {};
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(element).toHaveClass("apple");
    expect(addSpy).toHaveBeenCalledTimes(2);
  }));

  fit('should not deep watch array literals (one-time)', inject(function($rootScope, $compile, $animate) {
    $rootScope.classVar = {orange: {}};
    element = $compile('<div ng-class="::[classVar, classVar2]"></div>')($rootScope);
    var addSpy = spyOn($animate, "addClass").and.callThrough();

    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should not re-apply class when a deep property changes
    $rootScope.classVar.orange.foo = 42;
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(addSpy).toHaveBeenCalledTimes(1);

    //Should re-apply class when a direct property changes
    $rootScope.classVar.apple = {};
    $rootScope.$digest();
    expect(element).toHaveClass("orange");
    expect(element).toHaveClass("apple");
    expect(addSpy).toHaveBeenCalledTimes(2);
  }));