ngClass[Odd/Even] overhaul #15228

gkalpak · 2016-10-08T07:37:19Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Fix, Test, Refactor, Perf

What is the current behavior? (You can also link to an open issue here)
Broken, inefficient.

What is the new behavior (if this is a feature change)?
Fixed, efficient.

Does this PR introduce a breaking change?
No (to the best of my knowledge).

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
~~Docs have been added / updated (for bug fixes / features)~~

Other information:
Supercedes (and partly based on) #14404. Mad props to @drpicox for the initial idea and implementation.

This PR has been broken up in smaller, focused commits for easier reviewing. Better to review commit by commit (and probably in order). Most commits are (more or less) trivial - except for the last one.

Refactoring commits:

8822006: refactor(ngClass): remove unnecessary dependency on $animate
0409589: refactor(ngClass): remove redundant $observer
2871a11: refactor(ngClass): simplify conditions
a7e69f7: refactor(ngClass): move helper functions outside the closure
37cec7a: refactor(ngClass): exit arrayDifference() early if an input is empty

Test commits:

2705085: test(ngClass): add some tests about one-time bindings and objects inside arrays

Fix commits:

0ba4be6: fix(ngClassOdd/Even): keep track of classes even if odd/even do not match index

Perf commits:

89268af: perf(ngClass): only access the element's data once
5708424: perf(ngClass): avoid deep-watching (if possible) and unnecessary copies

Closes #14404

The code was added in b41fe9f in order to support having both `ngClass` and interpolation in `class` work together. `ngClass` has changed considerably since b41fe9f and for simple cases to work the `$observe`r is no longer necessary (as indicated by the expanded test still passing). That said, it is a [documented known issue][1] that `ngClass` should not be used together with interpolation in `class` and more complicated cases do not work anyway. [1]: https://docs.angularjs.org/api/ng/directive/ngClass#known-issues

…ide arrays Mostly taken from angular#14404.

…atch index

petebacondarwin · 2016-10-08T10:35:29Z

src/ng/directive/ngClass.js

-          }
+
+          attr.$addClass(toAdd);
+          attr.$removeClass(toRemove);


I never knew about these helpers!!

petebacondarwin · 2016-10-08T10:35:37Z

test/ng/directive/ngClassSpec.js

@@ -1,6 +1,6 @@
 'use strict';

-describe('ngClass', function() {
+fdescribe('ngClass', function() {


Naught naughty

I already have a fixup commit at the end 😛

petebacondarwin · 2016-10-08T10:38:29Z

test/ng/directive/ngClassSpec.js

+      expect(e2).not.toHaveClass('four');
+      expect(e2).toHaveClass('five');
+
+      $rootScope.$apply('two = "too"');


although this is very clever "two" and "too" are very similar and actually make the test harder to follow.

The cleverness credits go to the original test 😃
I can change it.

petebacondarwin · 2016-10-08T10:40:27Z

test/ng/directive/ngClassSpec.js

+    })
+  );
+
+  it('should do value remove the watcher when static map one-time binding',


the test description here seems like a hybrid of the two tests above :-)

petebacondarwin · 2016-10-08T10:41:52Z

test/ng/directive/ngClassSpec.js

+    inject(function($rootScope, $compile) {
+      $rootScope.classVar = [{orange: true}];
+      element = $compile('<div ng-class="classVar"></div>')($rootScope);
+      $rootScope.$digest();


Is it worth adding an expectation that it should have the class "orange" here?

petebacondarwin · 2016-10-08T10:49:25Z

test/ng/directive/ngClassSpec.js

@@ -462,6 +462,47 @@ fdescribe('ngClass', function() {
    expect(e2.hasClass('odd')).toBeFalsy();
  }));

+
+  it('should keep track of old classes even if odd/even does not match `$index` atm',


This description makes hard to understand what is actually being tested.

Are you saying that ngClass is not removing old classes when both the $index and the expression value changes?

It might be because I was trying to solve it differently at first. Here is what was happening before this change:

Assume ng-class-odd="foo", $index = 0, foo = 'foo'. The element has className === 'foo'.

If the $index and foo change simultaneously, say $index = 1, foo = 'bar', then:

The attr[name] watch kicks in first and it checks if scope.$index & 1 === selector (i.e. it uses the current value of $index to determine if it needs to update the classes). It decides it doesn't need to do anything, so at this point the element still has className === 'foo'.

The $index watch kicks in. Since it determines that $index & 1 !== old$index & 1, it decides to remove the classes. But it tries to scope.$eval(attr[name]) which now evaluates to 'bar'. So the element will still have className === 'foo'.

Basically, the problem is that both watch-actions check against the current value of each other to determine what to do, which fails when both change in the same digest.
I.e. the attr[name] watch uses scope.$index (while it should use the previous index) and the $index watch uses scope.$eval(attr[name]) (while it should use the previous value - the "old classes").

The commit fxes it by:

Moving the $index watch before the attr[name] watch.

Using oldVal from the $index watch to add/remove.

This way, when the $index & 1 changes, the $index watch will correctly add/remove the classes and then the attr[name] watch will update the classes (i.e. add the new ones) if necessary.

I hope this makes sense 😃

How about:

It should add/remove the correct classes when the expression and $index change simultaneously

petebacondarwin

When using large objects as keys (e.g.: {loaded: $ctrl.data}).

I think you mean "When using large objects as values...", right?

petebacondarwin · 2016-10-08T11:02:19Z

test/ng/directive/ngClassSpec.js

@@ -1,6 +1,6 @@
 'use strict';

-fdescribe('ngClass', function() {
+describe('ngClass', function() {


petebacondarwin · 2016-10-08T11:15:02Z

A few minor comments but generally looks good to me.

@drpicox

The cases that should benefit most are: 1. When using large objects as values (e.g.: `{loaded: $ctrl.data}`). 2. When using objects/arrays and there are frequent changes. 3. When there are many `$index` changes (e.g. addition/deletion/reordering in large `ngRepeat`s). The differences in operations per digest include: 1. `Regular expression (when not changed)` **Before:** `equals()` **After:** `toClassString()` 2. `Regular expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** 2 x `split()` 3. `One-time expression (when not changed)` **Before:** `equals()` **After:** `toFlatValue()` + `equals()`* 4. `One-time expression (when changed)` **Before:** `copy()` + 2 x `arrayClasses()` + `shallowCopy()` **After:** `copy()`* + `toClassString()`* + 2 x `split()` 5. `$index modulo changed` **Before:** `arrayClasses()` **After:** - (*): on flatter structure In large based on angular#14404. Kudos to @drpicox for the initial idea and a big part of the implementation. Closes angular#14404

…cts inside arrays

…do not match index Change commit message to: fix(ngClassOdd/Even): add/remove the correct classes when expression/`$index` change simultaneously

gkalpak · 2016-10-08T13:36:05Z

Thx for the review @petebacondarwin, @jbedard. I have added some fixup/squash commits addressing the comments (except for the parse interceptor for $index & 1).

PTAL

petebacondarwin · 2016-10-08T14:13:27Z

LGTM

…ry copies

w-sz · 2016-10-09T11:53:39Z

src/ng/directive/ngClass.js

-    function arrayDifference(tokens1, tokens2) {
-      var values = [];
+  // Helpers
+  function arrayDifference(tokens1, tokens2) {


Maybe this will be better ?

function arrayDifference(tokens1, tokens2) { var tokens1Length = tokens1.length, tokens2Length = tokens2.length; if (!tokens1 || !tokens1Length) return []; if (!tokens2 || !tokens2Length) return tokens1; var values = []; outer: for (var i = 0; i < tokens1Length; i++) { var token = tokens1[i]; for (var j = 0; j < tokens2Length; j++) { if (token === tokens2[j]) continue outer; } values.push(token); } return values; }

I thought about that too. The "problem" is that tokens1/tokens2 can be undefined. So, "caching" their length gets more complicated (to the point it shouldn't make any difference).

I also thought about just caching it for the for loop (e.g. for (var i = 0, ii = tokens1.length; i < ii; i++)), but I think the VM will detect that there is nothing mutating .length inside the loop and make that optimisation itself (this is just an assumption - maybe I am wrong).

gkalpak · 2016-10-11T18:40:56Z

Closing in favor of #15246 (which is just a clean-up version of this).
_{All comments and discussions here apply to #15246 as well.}

Thx for the reviews everyone!

gkalpak added 8 commits October 7, 2016 18:58

refactor(ngClass): remove unnecessary dependency on $animate

8822006

test(ngClass): add some tests about one-time bindings and objects ins…

2705085

…ide arrays Mostly taken from angular#14404.

fix(ngClassOdd/Even): keep track of classes even if odd/even do not m…

0ba4be6

…atch index

perf(ngClass): only access the element's data once

89268af

refactor(ngClass): simplify conditions

2871a11

refactor(ngClass): move helper functions outside the closure

a7e69f7

refactor(ngClass): exit arrayDifference() early if an input is empty

37cec7a

gkalpak added branch: 1.2.x branch: 1.3.x browser: Firefox browser: chrome browser: FirefoxOS labels Oct 8, 2016

gkalpak added this to the 1.5.x milestone Oct 8, 2016

googlebot added the cla: yes label Oct 8, 2016

gkalpak added type: bug frequency: high needs: review type: refactor component: misc core type: perf type: test severity: broken expected use severity: inconvenient and removed branch: 1.2.x branch: 1.3.x labels Oct 8, 2016

gkalpak removed branch: 1.5.x browser: chrome browser: Firefox browser: FirefoxOS labels Oct 8, 2016

gkalpak mentioned this pull request Oct 8, 2016

perf(ngClass): refactor to optimize the case of static map of classes with large objects #14404

Closed

3 tasks

petebacondarwin reviewed Oct 8, 2016

View reviewed changes

gkalpak added 5 commits October 8, 2016 16:24

fixup! refactor(ngClass): remove unnecessary dependency on $animate

739f723

fixup! refactor(ngClass): remove redundant $observer

181c51b

fixup! test(ngClass): add some tests about one-time bindings and obje…

16e666b

…cts inside arrays

squash! fix(ngClassOdd/Even): keep track of classes even if odd/even …

dfe4307

…do not match index Change commit message to: fix(ngClassOdd/Even): add/remove the correct classes when expression/`$index` change simultaneously

gkalpak force-pushed the ngClass-overhaul branch from 516fdb3 to dfe4307 Compare October 8, 2016 13:32

fixup! perf(ngClass): avoid deep-watching (if possible) and unnecessa…

f766698

…ry copies

w-sz reviewed Oct 9, 2016

View reviewed changes

fixup! refactor(ngClass): simplify conditions

a9742f9

drpicox mentioned this pull request Oct 10, 2016

chore(benchpress): add a ngClass benchmark #15243

Closed

3 tasks

gkalpak mentioned this pull request Oct 11, 2016

ngClass[Odd/Even] overhaul (2) #15246

Closed

3 tasks

gkalpak closed this Oct 11, 2016

gkalpak deleted the ngClass-overhaul branch December 9, 2016 10:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ngClass[Odd/Even] overhaul #15228

ngClass[Odd/Even] overhaul #15228

gkalpak commented Oct 8, 2016 •

edited

Loading

petebacondarwin Oct 8, 2016

petebacondarwin Oct 8, 2016

gkalpak Oct 8, 2016

petebacondarwin Oct 8, 2016

gkalpak Oct 8, 2016

petebacondarwin Oct 8, 2016

petebacondarwin Oct 8, 2016

petebacondarwin Oct 8, 2016

gkalpak Oct 8, 2016

petebacondarwin left a comment

petebacondarwin Oct 8, 2016

petebacondarwin commented Oct 8, 2016

gkalpak commented Oct 8, 2016

petebacondarwin commented Oct 8, 2016

w-sz Oct 9, 2016 •

edited

Loading

gkalpak Oct 10, 2016

gkalpak commented Oct 11, 2016

ngClass[Odd/Even] overhaul #15228

ngClass[Odd/Even] overhaul #15228

Conversation

gkalpak commented Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 8, 2016

gkalpak commented Oct 8, 2016

petebacondarwin commented Oct 8, 2016

w-sz Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 11, 2016

gkalpak commented Oct 8, 2016 •

edited

Loading

w-sz Oct 9, 2016 •

edited

Loading