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

Commit 510021d

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

File tree

2 files changed

+202
-30
lines changed

2 files changed

+202
-30
lines changed

‎src/ng/directive/ngClass.js

+48-30
Original file line numberDiff line numberDiff line change
@@ -8,32 +8,33 @@
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',
1414
link: function(scope, element, attr) {
15-
var oldVal;
15+
var oldClasses;
1616

17-
scope.$watch(attr[name], ngClassWatchAction, true);
17+
var classFn = $parse(attr[name], arrayClasses);
18+
scope.$watch(classFn, ngClassWatchAction, true);
1819

1920
attr.$observe('class', function(value) {
20-
ngClassWatchAction(scope.$eval(attr[name]));
21+
ngClassWatchAction(classFn(scope));
2122
});
2223

2324

2425
if (name !== 'ngClass') {
2526
scope.$watch('$index', function($index, old$index) {
26-
/* eslint-disable no-bitwise */
27+
// eslint-disable-next-line no-bitwise
2728
var mod = $index & 1;
29+
// eslint-disable-next-line no-bitwise
2830
if (mod !== (old$index & 1)) {
29-
var classes = arrayClasses(scope.$eval(attr[name]));
3031
if (mod === selector) {
31-
addClasses(classes);
32+
ngClassWatchAction(classFn(scope));
3233
} else {
33-
removeClasses(classes);
34+
removeClasses(oldClasses);
35+
oldClasses = [];
3436
}
3537
}
36-
/* eslint-enable */
3738
});
3839
}
3940

@@ -80,18 +81,13 @@ function classDirective(name, selector) {
8081
function ngClassWatchAction(newVal) {
8182
// eslint-disable-next-line no-bitwise
8283
if (selector === true || (scope.$index & 1) === selector) {
83-
var newClasses = arrayClasses(newVal || []);
84-
if (!oldVal) {
84+
var newClasses = (newVal || []).join(' ').split(' ');
85+
if (!oldClasses) {
8586
addClasses(newClasses);
86-
} else if (!equals(newVal,oldVal)) {
87-
var oldClasses = arrayClasses(oldVal);
87+
} else if (!arrayEqualClasses(oldClasses, newClasses)) {
8888
updateClasses(oldClasses, newClasses);
8989
}
90-
}
91-
if (isArray(newVal)) {
92-
oldVal = newVal.map(function(v) { return shallowCopy(v); });
93-
} else {
94-
oldVal = shallowCopy(newVal);
90+
oldClasses = shallowCopy(newClasses);
9591
}
9692
}
9793
}
@@ -112,24 +108,46 @@ function classDirective(name, selector) {
112108
}
113109

114110
function arrayClasses(classVal) {
115-
var classes = [];
111+
var classes;
116112
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(' ');
113+
classes = classVal.map(classNames);
123114
} else if (isObject(classVal)) {
124-
forEach(classVal, function(v, k) {
125-
if (v) {
126-
classes = classes.concat(k.split(' '));
115+
classes = [];
116+
forEach(classVal, function(val, key) {
117+
if (val) {
118+
classes.push(key);
119+
} else if (isUndefined(val)) {
120+
// This case is for one time binding:
121+
// ::expression are evaluated until no undefineds are found
122+
// if no undefined is placed inside the array,
123+
// it would assume that the value is fully computed and will fail
124+
classes.push(val);
127125
}
128126
});
129-
return classes;
127+
} else if (isString(classVal)) {
128+
classes = [classVal];
129+
} else {
130+
classes = classVal;
130131
}
131-
return classVal;
132+
133+
return classes;
132134
}
135+
136+
function classNames(values) {
137+
if (isObject(values)) {
138+
return Object.keys(values).filter(function(k) {
139+
return values[k];
140+
}).join(' ');
141+
} else {
142+
return values;
143+
}
144+
}
145+
146+
function arrayEqualClasses(a1, a2) {
147+
return a1 === a2 ||
148+
isArray(a1) && isArray(a2) && a1.join(' ') === a2.join(' ');
149+
}
150+
133151
}];
134152
}
135153

‎test/ng/directive/ngClassSpec.js

+154
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,54 @@ describe('ngClass', function() {
299299
expect(e2.hasClass('D')).toBeFalsy();
300300
}));
301301

302+
it('should compute class names when changing row parity and class value',
303+
inject(function($rootScope, $compile) {
304+
element = $compile('<ul>' +
305+
'<li ng-repeat="i in arr track by i" ng-class-even="cls"></li>' +
306+
'</ul>')($rootScope);
307+
308+
$rootScope.$apply(function() {
309+
$rootScope.arr = ['a','b'];
310+
$rootScope.cls = 'even';
311+
});
312+
var e3 = jqLite(element[0].childNodes[3]);
313+
expect(e3).toHaveClass('even');
314+
315+
$rootScope.$apply(function() {
316+
$rootScope.arr = ['b'];
317+
$rootScope.cls = 'off';
318+
});
319+
var e1 = jqLite(element[0].childNodes[1]);
320+
expect(e3[0]).toBe(e1[0]); // make sure same instance of ngClassEven
321+
expect(e3).not.toHaveClass('even');
322+
expect(e3).not.toHaveClass('off');
323+
324+
$rootScope.$apply(function() {
325+
$rootScope.arr = ['a','b'];
326+
$rootScope.cls = 'on';
327+
});
328+
expect(e3).not.toHaveClass('even');
329+
expect(e3).not.toHaveClass('off');
330+
expect(e3).toHaveClass('on');
331+
332+
// activate both $watch functions (class and $index)
333+
// see https://plnkr.co/edit/W1ck8dS08TCJpirWiGVA
334+
$rootScope.$apply(function() {
335+
$rootScope.arr = ['b'];
336+
$rootScope.cls = 'off';
337+
});
338+
expect(e3).not.toHaveClass('even');
339+
expect(e3).not.toHaveClass('off');
340+
expect(e3).not.toHaveClass('on');
341+
342+
$rootScope.$apply(function() {
343+
$rootScope.arr = ['a','b'];
344+
});
345+
expect(e3).not.toHaveClass('even');
346+
expect(e3).toHaveClass('off');
347+
expect(e3).not.toHaveClass('on');
348+
})
349+
);
302350

303351
it('should reapply ngClass when interpolated class attribute changes', inject(function($rootScope, $compile) {
304352
element = $compile('<div class="one {{cls}} three" ng-class="{four: four}"></div>')($rootScope);
@@ -424,6 +472,112 @@ describe('ngClass', function() {
424472
})
425473
);
426474

475+
it('should do value stabilization as expected when one-time binding',
476+
inject(function($rootScope, $compile) {
477+
element = $compile('<div ng-class=" :: className"></div>')($rootScope);
478+
$rootScope.$digest();
479+
480+
$rootScope.className = 'foo';
481+
$rootScope.$digest();
482+
expect(element).toHaveClass('foo');
483+
484+
$rootScope.className = 'bar';
485+
$rootScope.$digest();
486+
expect(element).toHaveClass('foo');
487+
})
488+
);
489+
490+
it('should remove the watcher when static array one-time binding',
491+
inject(function($rootScope, $compile) {
492+
element = $compile('<div ng-class="::[className]"></div>')($rootScope);
493+
$rootScope.$digest();
494+
495+
$rootScope.className = 'foo';
496+
$rootScope.$digest();
497+
expect(element).toHaveClass('foo');
498+
499+
$rootScope.className = 'bar';
500+
$rootScope.$digest();
501+
expect(element).toHaveClass('foo');
502+
expect(element).not.toHaveClass('bar');
503+
})
504+
);
505+
506+
it('should remove the watcher when static map one-time binding',
507+
inject(function($rootScope, $compile) {
508+
element = $compile('<div ng-class="::{foo: fooPresent}"></div>')($rootScope);
509+
$rootScope.$digest();
510+
511+
$rootScope.fooPresent = true;
512+
$rootScope.$digest();
513+
expect(element).toHaveClass('foo');
514+
515+
$rootScope.fooPresent = false;
516+
$rootScope.$digest();
517+
expect(element).toHaveClass('foo');
518+
})
519+
);
520+
521+
it('should track changes of mutating object inside an array',
522+
inject(function($rootScope, $compile) {
523+
$rootScope.classVar = [{orange: true}];
524+
element = $compile('<div ng-class="classVar"></div>')($rootScope);
525+
$rootScope.$digest();
526+
expect(element).toHaveClass('orange');
527+
528+
$rootScope.classVar[0].orange = false;
529+
$rootScope.$digest();
530+
531+
expect(element).not.toHaveClass('orange');
532+
})
533+
);
534+
535+
describe('large objects', function() {
536+
537+
var verylargeobject, getProp;
538+
beforeEach(function() {
539+
getProp = jasmine.createSpy('getProp');
540+
verylargeobject = {};
541+
Object.defineProperty(verylargeobject, 'prop', {
542+
get: getProp,
543+
enumerable: true
544+
});
545+
});
546+
547+
it('should not be copied if static', inject(function($rootScope, $compile) {
548+
element = $compile('<div ng-class="{foo: verylargeobject}"></div>')($rootScope);
549+
$rootScope.verylargeobject = verylargeobject;
550+
$rootScope.$digest();
551+
552+
expect(getProp).not.toHaveBeenCalled();
553+
}));
554+
555+
it('should not be copied if dynamic', inject(function($rootScope, $compile) {
556+
$rootScope.fooClass = {foo: verylargeobject};
557+
element = $compile('<div ng-class="fooClass"></div>')($rootScope);
558+
$rootScope.$digest();
559+
560+
expect(getProp).not.toHaveBeenCalled();
561+
}));
562+
563+
it('should not be copied if inside an array', inject(function($rootScope, $compile) {
564+
element = $compile('<div ng-class="[{foo: verylargeobject}]"></div>')($rootScope);
565+
$rootScope.verylargeobject = verylargeobject;
566+
$rootScope.$digest();
567+
568+
expect(getProp).not.toHaveBeenCalled();
569+
}));
570+
571+
it('should not be copied when one-time binding', inject(function($rootScope, $compile) {
572+
element = $compile('<div ng-class="::{foo: verylargeobject}"></div>')($rootScope);
573+
$rootScope.verylargeobject = verylargeobject;
574+
$rootScope.$digest();
575+
576+
expect(getProp).not.toHaveBeenCalled();
577+
}));
578+
579+
});
580+
427581
});
428582

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

0 commit comments

Comments
 (0)