Skip to content

Commit

Permalink
feat(core): Reduce digest triggers when using $timeout
Browse files Browse the repository at this point in the history
Angular's $timeout takes a boolean 3rd parameter which
causes the wrapped function to not run inside $apply.
Since $timeout is used extensively in scope, this parameter
has been passed where possible in order to improve
performance, since the grid otherwise triggers a large
number of digests while functioning normally.

See issue: #5007
  • Loading branch information
csvan committed Nov 23, 2016
1 parent b35fca4 commit 7e25a9b
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
2 changes: 1 addition & 1 deletion src/js/core/directives/ui-grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ function uiGridDirective($compile, $templateCache, $timeout, $window, gridUtil,
sizeChecks++;
}
else {
$timeout(init);
$timeout(init, 0, false);
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/js/core/factories/Grid.js
Original file line number Diff line number Diff line change
Expand Up @@ -2244,13 +2244,13 @@ angular.module('ui.grid')
}

p.resolve();
});
}, 0, false);
}
else {
// Timeout still needs to be here to trigger digest after styles have been rebuilt
$timeout(function() {
p.resolve();
});
}, 0, false);
}

return p.promise;
Expand Down
8 changes: 4 additions & 4 deletions src/js/core/services/ui-grid-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -831,7 +831,7 @@ module.service('gridUtil', ['$log', '$window', '$document', '$http', '$templateC
} else {
s.logWarn('[focus.byId] Element id ' + elementID + ' was not found.');
}
});
}, 0, false);
this.queue.push(promise);
return promise;
},
Expand All @@ -856,7 +856,7 @@ module.service('gridUtil', ['$log', '$window', '$document', '$http', '$templateC
if (element){
element[0].focus();
}
});
}, 0, false);
this.queue.push(promise);
return promise;
},
Expand Down Expand Up @@ -886,8 +886,8 @@ module.service('gridUtil', ['$log', '$window', '$document', '$http', '$templateC
};
this._purgeQueue();
if (aSync){ //Do this asynchronysly
var promise = $timeout(focusBySelector);
this.queue.push($timeout(focusBySelector));
var promise = $timeout(focusBySelector, 0, false);
this.queue.push($timeout(focusBySelector), 0, false);
return promise;
} else {
return focusBySelector();
Expand Down

3 comments on commit 7e25a9b

@mportuga
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this change seems to have introduced some unforeseen issues and will have to be reverted,

@csvan
Copy link
Contributor Author

@csvan csvan commented on 7e25a9b Dec 15, 2016 via email

Choose a reason for hiding this comment

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

@MichalJakubeczy
Copy link

Choose a reason for hiding this comment

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

Yes please @mportuga tell us what it broke.

This commit seems to improve performance so we're curious what it breaks to consider whether to risk usage or not.

Please sign in to comment.