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

Commit 5fe61c2

Browse files
drpicoxdrpicox
committed
perf(ngClass): optimize the case of static map of classes with large objects by refactor
1 parent 51a2eb7 commit 5fe61c2

File tree

2 files changed

+207
-83
lines changed

2 files changed

+207
-83
lines changed

‎src/ng/directive/ngClass.js

Lines changed: 102 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -8,92 +8,93 @@
88

99
function classDirective(name, selector) {
1010
name = 'ngClass' + name;
11-
return ['$animate', function($animate) {
11+
return ['$animate','$parse', function($animate,$parse) {
1212
return {
1313
restrict: 'AC',
14-
link: function(scope, element, attr) {
15-
var oldVal;
16-
17-
scope.$watch(attr[name], ngClassWatchAction, true);
18-
19-
attr.$observe('class', function(value) {
20-
ngClassWatchAction(scope.$eval(attr[name]));
14+
compile: function(tElement, tAttr) {
15+
var classFn = $parse(tAttr[name], function(value) {
16+
return arrayClassesL1(value);
2117
});
18+
return function(scope, element, attr) {
19+
var oldClasses;
2220

2321

24-
if (name !== 'ngClass') {
25-
scope.$watch('$index', function($index, old$index) {
26-
/* eslint-disable no-bitwise */
27-
var mod = $index & 1;
28-
if (mod !== (old$index & 1)) {
29-
var classes = arrayClasses(scope.$eval(attr[name]));
30-
if (mod === selector) {
31-
addClasses(classes);
32-
} else {
33-
removeClasses(classes);
34-
}
35-
}
36-
/* eslint-enable */
37-
});
38-
}
22+
scope.$watch(classFn, ngClassWatchAction, classArrayEquals);
3923

40-
function addClasses(classes) {
41-
var newClasses = digestClassCounts(classes, 1);
42-
attr.$addClass(newClasses);
43-
}
24+
attr.$observe('class', function(value) {
25+
ngClassWatchAction(classFn(scope));
26+
});
4427

45-
function removeClasses(classes) {
46-
var newClasses = digestClassCounts(classes, -1);
47-
attr.$removeClass(newClasses);
48-
}
4928

50-
function digestClassCounts(classes, count) {
51-
// Use createMap() to prevent class assumptions involving property
52-
// names in Object.prototype
53-
var classCounts = element.data('$classCounts') || createMap();
54-
var classesToUpdate = [];
55-
forEach(classes, function(className) {
56-
if (count > 0 || classCounts[className]) {
57-
classCounts[className] = (classCounts[className] || 0) + count;
58-
if (classCounts[className] === +(count > 0)) {
59-
classesToUpdate.push(className);
29+
if (name !== 'ngClass') {
30+
scope.$watch('$index', function($index, old$index) {
31+
// eslint-disable-next-line no-bitwise
32+
var mod = $index & 1;
33+
// eslint-disable-next-line no-bitwise
34+
if (mod !== (old$index & 1)) {
35+
var classes = classFn(scope).join(' ').split(' ');
36+
if (mod === selector) {
37+
addClasses(classes);
38+
} else {
39+
removeClasses(classes);
40+
}
6041
}
61-
}
62-
});
63-
element.data('$classCounts', classCounts);
64-
return classesToUpdate.join(' ');
65-
}
42+
});
43+
}
6644

67-
function updateClasses(oldClasses, newClasses) {
68-
var toAdd = arrayDifference(newClasses, oldClasses);
69-
var toRemove = arrayDifference(oldClasses, newClasses);
70-
toAdd = digestClassCounts(toAdd, 1);
71-
toRemove = digestClassCounts(toRemove, -1);
72-
if (toAdd && toAdd.length) {
73-
$animate.addClass(element, toAdd);
45+
function addClasses(classes) {
46+
var newClasses = digestClassCounts(classes, 1);
47+
attr.$addClass(newClasses);
7448
}
75-
if (toRemove && toRemove.length) {
76-
$animate.removeClass(element, toRemove);
49+
50+
function removeClasses(classes) {
51+
var newClasses = digestClassCounts(classes, -1);
52+
attr.$removeClass(newClasses);
7753
}
78-
}
7954

80-
function ngClassWatchAction(newVal) {
81-
// eslint-disable-next-line no-bitwise
82-
if (selector === true || (scope.$index & 1) === selector) {
83-
var newClasses = arrayClasses(newVal || []);
84-
if (!oldVal) {
85-
addClasses(newClasses);
86-
} else if (!equals(newVal,oldVal)) {
87-
var oldClasses = arrayClasses(oldVal);
88-
updateClasses(oldClasses, newClasses);
55+
function digestClassCounts(classes, count) {
56+
// Use createMap() to prevent class assumptions involving property
57+
// names in Object.prototype
58+
var classCounts = element.data('$classCounts') || createMap();
59+
var classesToUpdate = [];
60+
forEach(classes, function(className) {
61+
if (count > 0 || classCounts[className]) {
62+
classCounts[className] = (classCounts[className] || 0) + count;
63+
if (classCounts[className] === +(count > 0)) {
64+
classesToUpdate.push(className);
65+
}
66+
}
67+
});
68+
element.data('$classCounts', classCounts);
69+
return classesToUpdate.join(' ');
70+
}
71+
72+
function updateClasses(oldClasses, newClasses) {
73+
var toAdd = arrayDifference(newClasses, oldClasses);
74+
var toRemove = arrayDifference(oldClasses, newClasses);
75+
toAdd = digestClassCounts(toAdd, 1);
76+
toRemove = digestClassCounts(toRemove, -1);
77+
if (toAdd && toAdd.length) {
78+
$animate.addClass(element, toAdd);
79+
}
80+
if (toRemove && toRemove.length) {
81+
$animate.removeClass(element, toRemove);
8982
}
9083
}
91-
if (isArray(newVal)) {
92-
oldVal = newVal.map(function(v) { return shallowCopy(v); });
93-
} else {
94-
oldVal = shallowCopy(newVal);
84+
85+
function ngClassWatchAction(newVal) {
86+
// eslint-disable-next-line no-bitwise
87+
if (selector === true || (scope.$index & 1) === selector) {
88+
var newClasses = (newVal || []).join(' ').split(' ');
89+
if (!oldClasses) {
90+
addClasses(newClasses);
91+
} else if (!classArrayEquals(oldClasses, newClasses)) {
92+
updateClasses(oldClasses, newClasses);
93+
}
94+
oldClasses = shallowCopy(newClasses);
95+
}
9596
}
96-
}
97+
};
9798
}
9899
};
99100

@@ -111,25 +112,43 @@ function classDirective(name, selector) {
111112
return values;
112113
}
113114

114-
function arrayClasses(classVal) {
115-
var classes = [];
115+
function arrayClassesL1(classVal) {
116+
var classes;
116117
if (isArray(classVal)) {
117-
forEach(classVal, function(v) {
118-
classes = classes.concat(arrayClasses(v));
119-
});
120-
return classes;
121-
} else if (isString(classVal)) {
122-
return classVal.split(' ');
118+
classes = classVal.map(arrayClassesL2);
123119
} else if (isObject(classVal)) {
124-
forEach(classVal, function(v, k) {
125-
if (v) {
126-
classes = classes.concat(k.split(' '));
120+
classes = [];
121+
forEach(classVal, function(val, key) {
122+
if (val) {
123+
classes.push(key);
124+
} else if (isUndefined(val)) {
125+
classes.push(val);
127126
}
128127
});
129-
return classes;
128+
} else if (isString(classVal)) {
129+
classes = [classVal];
130+
} else {
131+
classes = classVal;
132+
}
133+
134+
return classes;
135+
}
136+
137+
function arrayClassesL2(values) {
138+
if (isObject(values)) {
139+
return Object.keys(values).filter(function(k) {
140+
return values[k];
141+
}).join(' ');
142+
} else {
143+
return values;
130144
}
131-
return classVal;
132145
}
146+
147+
function classArrayEquals(a1, a2) {
148+
return a1 === a2 ||
149+
isArray(a1) && isArray(a2) && a1.join(' ') === a2.join(' ');
150+
}
151+
133152
}];
134153
}
135154

‎test/ng/directive/ngClassSpec.js

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,111 @@ describe('ngClass', function() {
424424
})
425425
);
426426

427+
it('should do value stabilization as expected when one-time binding',
428+
inject(function($rootScope, $compile) {
429+
element = $compile('<div ng-class=" :: className"></div>')($rootScope);
430+
$rootScope.$digest();
431+
432+
$rootScope.className = 'foo';
433+
$rootScope.$digest();
434+
expect(element).toHaveClass('foo');
435+
436+
$rootScope.className = 'bar';
437+
$rootScope.$digest();
438+
expect(element).toHaveClass('foo');
439+
})
440+
);
441+
442+
it('should do value remove the watcher when static array one-time binding',
443+
inject(function($rootScope, $compile) {
444+
element = $compile('<div ng-class="::[className]"></div>')($rootScope);
445+
$rootScope.$digest();
446+
447+
$rootScope.className = 'foo';
448+
$rootScope.$digest();
449+
expect(element).toHaveClass('foo');
450+
451+
$rootScope.className = 'bar';
452+
$rootScope.$digest();
453+
expect(element).toHaveClass('foo');
454+
expect(element).not.toHaveClass('bar');
455+
})
456+
);
457+
458+
it('should do value remove the watcher when static map one-time binding',
459+
inject(function($rootScope, $compile) {
460+
element = $compile('<div ng-class="::{foo: fooPresent}"></div>')($rootScope);
461+
$rootScope.$digest();
462+
463+
$rootScope.fooPresent = true;
464+
$rootScope.$digest();
465+
expect(element).toHaveClass('foo');
466+
467+
$rootScope.fooPresent = false;
468+
$rootScope.$digest();
469+
expect(element).toHaveClass('foo');
470+
})
471+
);
472+
473+
it('should track changes of mutating object inside an array',
474+
inject(function($rootScope, $compile) {
475+
$rootScope.classVar = [{orange: true}];
476+
element = $compile('<div ng-class="classVar"></div>')($rootScope);
477+
$rootScope.$digest();
478+
479+
$rootScope.classVar[0].orange = false;
480+
$rootScope.$digest();
481+
482+
expect(element).not.toHaveClass('orange');
483+
})
484+
);
485+
486+
describe('large objects', function() {
487+
488+
var verylargeobject, getProp;
489+
beforeEach(function() {
490+
getProp = jasmine.createSpy('getProp');
491+
verylargeobject = {};
492+
Object.defineProperty(verylargeobject, 'prop', {
493+
get: getProp,
494+
enumerable: true
495+
});
496+
});
497+
498+
it('should not be copied if static', inject(function($rootScope, $compile) {
499+
element = $compile('<div ng-class="{foo: verylargeobject}"></div>')($rootScope);
500+
$rootScope.verylargeobject = verylargeobject;
501+
$rootScope.$digest();
502+
503+
expect(getProp).not.toHaveBeenCalled();
504+
}));
505+
506+
it('should not be copied if dynamic', inject(function($rootScope, $compile) {
507+
$rootScope.fooClass = {foo: verylargeobject};
508+
element = $compile('<div ng-class="fooClass"></div>')($rootScope);
509+
$rootScope.$digest();
510+
511+
expect(getProp).not.toHaveBeenCalled();
512+
}));
513+
514+
it('should not be copied if inside an array', inject(function($rootScope, $compile) {
515+
element = $compile('<div ng-class="[{foo: verylargeobject}]"></div>')($rootScope);
516+
$rootScope.verylargeobject = verylargeobject;
517+
$rootScope.$digest();
518+
519+
expect(getProp).not.toHaveBeenCalled();
520+
}));
521+
522+
it('should not be copied when one-time binding', inject(function($rootScope, $compile) {
523+
element = $compile('<div ng-class="::{foo: verylargeobject}"></div>')($rootScope);
524+
$rootScope.verylargeobject = verylargeobject;
525+
$rootScope.$digest();
526+
527+
expect(getProp).not.toHaveBeenCalled();
528+
}));
529+
530+
});
531+
427532
});
428533

429534
describe('ngClass animations', function() {

0 commit comments

Comments
 (0)