perf(ngStyleDirective): use $watchCollection

[jbedard]

This re-commits 4c8d8ad which was reverted (with tests added). This is possible now because of 189461f, although note that change is currently causing #15905 so maybe we should hold off on merging this one for now?

FYI @gdi2290

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[googlebot]

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

[gkalpak]

👍 for waiting until #15905 is resolved.

[Narretz]

Interestingly, there's no test for normal literal bindings:

  it('should support binding for object literals', inject(function($rootScope, $compile) {
    element = $compile('<div ng-style="{height: heightStr}"></div>')($rootScope);
    $rootScope.$digest();
    expect(parseInt(element.css('height') + 0, 10)).toEqual(0); // height could be '' or '0px'
    $rootScope.$apply('heightStr = "40px"');
    expect(element.css('height')).toBe('40px');

    $rootScope.$apply('heightStr = "100px"');
    expect(element.css('height')).toBe('100px');
  }));

[jbedard]

@Narretz I've added a commit with that test. Also changed to 1.7 because this depends on 189461f which is going to be reverted in 1.6 (#15958).

[jbedard]

I think this can go into 1.7 now @gkalpak?

[jbedard]

Note: this will break cases where people are relying on a value being an object and automatically invoking .toString() such as {color: obj} where obj.toString() is stateful. This can be fixed by changing the expression to {color: obj.toString()}. Should add a breaking change notice for this.

[gkalpak]

As discussed "offline", this is good-to-go for 1.7 (with the breaking change notice) 👍

[jbedard]

I've updated the commit message with a "BREAKING CHANGE" message and changed the "Closes #" to this PR. Look good?

[Narretz]

Should there be a test for the case that is described in the BC notice?

[jbedard]

A test verifying that styles don't get re-applied when an object sub-property changes? Could...

[Narretz]

Just because we are setting an expectation for this behavior

[jbedard]

I've added a test. The other option is verifying that ng-style="x" invokes $watchCollection(x)...?

[Narretz]

LGTM

[PatrickJS]

nice

[gkalpak]

A tiny change with potentially big gains 😃
(It also took some serious poking around the $parse service - kudos to @jbedard 💯)