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

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

Closed
wants to merge 2 commits into from
+202 −30
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aafb9d9
perf(ngClass): optimize the case of static map of classes with large …
drpicox Apr 8, 2016
01e395a
test(ngClass): test large objects, ::one time expressions, and $index…
drpicox Oct 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 48 additions & 30 deletions src/ng/directive/ngClass.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,32 +8,33 @@

function classDirective(name, selector) {
name = 'ngClass' + name;
return ['$animate', function($animate) {
return ['$animate','$parse', function($animate,$parse) {
return {
restrict: 'AC',
link: function(scope, element, attr) {
var oldVal;
var oldClasses;

scope.$watch(attr[name], ngClassWatchAction, true);
var classFn = $parse(attr[name], arrayClasses);
scope.$watch(classFn, ngClassWatchAction, true);

attr.$observe('class', function(value) {
ngClassWatchAction(scope.$eval(attr[name]));
ngClassWatchAction(classFn(scope));
});


if (name !== 'ngClass') {
scope.$watch('$index', function($index, old$index) {
/* eslint-disable no-bitwise */
// eslint-disable-next-line no-bitwise
var mod = $index & 1;
// eslint-disable-next-line no-bitwise
if (mod !== (old$index & 1)) {
var classes = arrayClasses(scope.$eval(attr[name]));
if (mod === selector) {
addClasses(classes);
ngClassWatchAction(classFn(scope));
} else {
removeClasses(classes);
removeClasses(oldClasses);
oldClasses = [];
}
}
/* eslint-enable */
});
}

Expand Down Expand Up @@ -80,18 +81,13 @@ function classDirective(name, selector) {
function ngClassWatchAction(newVal) {
// eslint-disable-next-line no-bitwise
if (selector === true || (scope.$index & 1) === selector) {
var newClasses = arrayClasses(newVal || []);
if (!oldVal) {
var newClasses = (newVal || []).join(' ').split(' ');
if (!oldClasses) {
addClasses(newClasses);
} else if (!equals(newVal,oldVal)) {
var oldClasses = arrayClasses(oldVal);
} else if (!arrayEqualClasses(oldClasses, newClasses)) {
updateClasses(oldClasses, newClasses);
}
}
if (isArray(newVal)) {
oldVal = newVal.map(function(v) { return shallowCopy(v); });
} else {
oldVal = shallowCopy(newVal);
oldClasses = shallowCopy(newClasses);
}
}
}
Expand All @@ -112,24 +108,46 @@ function classDirective(name, selector) {
}

function arrayClasses(classVal) {
var classes = [];
var classes;
if (isArray(classVal)) {
forEach(classVal, function(v) {
classes = classes.concat(arrayClasses(v));
});
return classes;
} else if (isString(classVal)) {
return classVal.split(' ');
classes = classVal.map(classNames);
} else if (isObject(classVal)) {
forEach(classVal, function(v, k) {
if (v) {
classes = classes.concat(k.split(' '));
classes = [];
forEach(classVal, function(val, key) {
if (val) {
classes.push(key);
} else if (isUndefined(val)) {
// This case is for one time binding:
// ::expression are evaluated until no undefineds are found
// if no undefined is placed inside the array,
// it would assume that the value is fully computed and will fail
classes.push(val);
}
});
return classes;
} else if (isString(classVal)) {
classes = [classVal];
} else {
classes = classVal;
}
return classVal;

return classes;
}

function classNames(values) {
if (isObject(values)) {
return Object.keys(values).filter(function(k) {
return values[k];
}).join(' ');
} else {
return values;
}
}

function arrayEqualClasses(a1, a2) {
return a1 === a2 ||
isArray(a1) && isArray(a2) && a1.join(' ') === a2.join(' ');
}

}];
}

Expand Down
154 changes: 154 additions & 0 deletions test/ng/directive/ngClassSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,54 @@ describe('ngClass', function() {
expect(e2.hasClass('D')).toBeFalsy();
}));

it('should compute class names when changing row parity and class value',
inject(function($rootScope, $compile) {
element = $compile('<ul>' +
'<li ng-repeat="i in arr track by i" ng-class-even="cls"></li>' +
'</ul>')($rootScope);

$rootScope.$apply(function() {
$rootScope.arr = ['a','b'];
$rootScope.cls = 'even';
});
var e3 = jqLite(element[0].childNodes[3]);
expect(e3).toHaveClass('even');

$rootScope.$apply(function() {
$rootScope.arr = ['b'];
$rootScope.cls = 'off';
});
var e1 = jqLite(element[0].childNodes[1]);
expect(e3[0]).toBe(e1[0]); // make sure same instance of ngClassEven
expect(e3).not.toHaveClass('even');
expect(e3).not.toHaveClass('off');

$rootScope.$apply(function() {
$rootScope.arr = ['a','b'];
$rootScope.cls = 'on';
});
expect(e3).not.toHaveClass('even');
expect(e3).not.toHaveClass('off');
expect(e3).toHaveClass('on');

// activate both $watch functions (class and $index)
// see https://plnkr.co/edit/W1ck8dS08TCJpirWiGVA
$rootScope.$apply(function() {
$rootScope.arr = ['b'];
$rootScope.cls = 'off';
});
expect(e3).not.toHaveClass('even');
expect(e3).not.toHaveClass('off');
expect(e3).not.toHaveClass('on');

$rootScope.$apply(function() {
$rootScope.arr = ['a','b'];
});
expect(e3).not.toHaveClass('even');
expect(e3).toHaveClass('off');
expect(e3).not.toHaveClass('on');
})
);

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

it('should do value stabilization as expected when one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class=" :: className"></div>')($rootScope);
$rootScope.$digest();

$rootScope.className = 'foo';
$rootScope.$digest();
expect(element).toHaveClass('foo');

$rootScope.className = 'bar';
$rootScope.$digest();
expect(element).toHaveClass('foo');
})
);

it('should remove the watcher when static array one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::[className]"></div>')($rootScope);
$rootScope.$digest();

$rootScope.className = 'foo';
$rootScope.$digest();
expect(element).toHaveClass('foo');

$rootScope.className = 'bar';
$rootScope.$digest();
expect(element).toHaveClass('foo');
expect(element).not.toHaveClass('bar');
})
);

it('should remove the watcher when static map one-time binding',
inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::{foo: fooPresent}"></div>')($rootScope);
$rootScope.$digest();

$rootScope.fooPresent = true;
$rootScope.$digest();
expect(element).toHaveClass('foo');

$rootScope.fooPresent = false;
$rootScope.$digest();
expect(element).toHaveClass('foo');
})
);

it('should track changes of mutating object inside an array',
inject(function($rootScope, $compile) {
$rootScope.classVar = [{orange: true}];
element = $compile('<div ng-class="classVar"></div>')($rootScope);
$rootScope.$digest();
expect(element).toHaveClass('orange');

$rootScope.classVar[0].orange = false;
$rootScope.$digest();

expect(element).not.toHaveClass('orange');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check that the class was present before changing orange to false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

})
);

describe('large objects', function() {

var verylargeobject, getProp;
beforeEach(function() {
getProp = jasmine.createSpy('getProp');
verylargeobject = {};
Object.defineProperty(verylargeobject, 'prop', {
get: getProp,
enumerable: true
});
});

it('should not be copied if static', inject(function($rootScope, $compile) {
element = $compile('<div ng-class="{foo: verylargeobject}"></div>')($rootScope);
$rootScope.verylargeobject = verylargeobject;
$rootScope.$digest();

expect(getProp).not.toHaveBeenCalled();
}));

it('should not be copied if dynamic', inject(function($rootScope, $compile) {
$rootScope.fooClass = {foo: verylargeobject};
element = $compile('<div ng-class="fooClass"></div>')($rootScope);
$rootScope.$digest();

expect(getProp).not.toHaveBeenCalled();
}));

it('should not be copied if inside an array', inject(function($rootScope, $compile) {
element = $compile('<div ng-class="[{foo: verylargeobject}]"></div>')($rootScope);
$rootScope.verylargeobject = verylargeobject;
$rootScope.$digest();

expect(getProp).not.toHaveBeenCalled();
}));

it('should not be copied when one-time binding', inject(function($rootScope, $compile) {
element = $compile('<div ng-class="::{foo: verylargeobject}"></div>')($rootScope);
$rootScope.verylargeobject = verylargeobject;
$rootScope.$digest();

expect(getProp).not.toHaveBeenCalled();
}));

});

});

describe('ngClass animations', function() {
Expand Down