Skip to content

Commit

Permalink
feat(NgTableController): optimize calls to reload
Browse files Browse the repository at this point in the history
Coalesce updates to parameters and a call to reload into a single call to `getData`

**WARNING**: the *"private"* `$params` field has been removed from `NgTableParams`

This means the following code pattern can be avoided:

```js
/* reset table ... */
if (params.hasFilter() || params.page() > 1) {
    // this will trigger a reload later in the $digest cycle
    params.filter({});
    params.page(1);
    // return a dummy promise to satisfy the api of our function
    // note: we don't want to call reload as this would result in two calls to getData
    return $q.when(null);
} else {
    // just reload current page
    return params.reload();
}
```

Instead the above can be simplified and still yield a single call to `getData`

```js
/* reset table ... */
params.filter({});
params.page(1);
return params.reload({ filter: {}, page: 1});
```
  • Loading branch information
ccrowhurstram committed Aug 2, 2015
1 parent 95b0f2b commit e94ca5f
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 60 deletions.
2 changes: 1 addition & 1 deletion src/scripts/03-params.js
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,7 @@ app.factory('NgTableParams', ['$q', '$log', 'ngTableDefaults', 'ngTableGetDataBc
}, dataFetched);
}

var params = this.$params = {
var params = {
page: 1,
count: 1,
filter: {},
Expand Down
74 changes: 17 additions & 57 deletions src/scripts/04-controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,82 +36,43 @@ function($scope, NgTableParams, $timeout, $parse, $compile, $attrs, $element, ng
};
})();

var pendingReload;

function onParamsChange (newParams, oldParams) {

// We don't want to watch for changes to $params whilst the NgTableParams.reload function is executing
// (ie $loading === true).
// This is important for cases where you have a want to *chain* a subsequent call to reload.
// Take the following code example:
//
// tableParams.reload().then(function(){
// if (!tableParams.total() && _.size(tableParams.filter()) > 0) {
// tableParams.filter({});
// return tableParams.reload();
// }
// });
//
// In the code above, you're checking whether to remove the table filter. When removing the filter
// you want the second reload to execute in the *same promise chain* initiated by the first call
// to reload; you do NOT want the second reload to trigger sometime later because of a $watch
// seeing the change to the filter.
if (newParams === oldParams || $scope.params.settings().$loading) {
function onDataReloadStatusChange (newStatus/*, oldStatus*/) {
if (!newStatus) {
return;
}

$scope.params.settings().$scope = $scope;

var currentParams = $scope.params;

var doReloadNow;
if (currentParams.$$paramsBootstrapping){
doReloadNow = function(){
currentParams.$$paramsBootstrapping = false;
currentParams.reload();
}
} else {
doReloadNow = currentParams.reload.bind(currentParams);
}

if (!angular.equals(newParams.filter, oldParams.filter)) {
var maybeResetPage;
if (currentParams.$$paramsBootstrapping) {
maybeResetPage = angular.noop;
} else {
var maybeResetPage = function resetPage() {
currentParams.$params.page = 1;
};
}
if (currentParams.hasFilterChanges()) {
var applyFilter = function () {
pendingReload = false;
maybeResetPage();
doReloadNow();
currentParams.page(1);
currentParams.reload();
};
if (currentParams.settings().filterDelay && !currentParams.$$paramsBootstrapping) {
pendingReload = true;
if (currentParams.settings().filterDelay) {
delayFilter(applyFilter, currentParams.settings().filterDelay);
} else {
applyFilter();
}
} else {
doReloadNow();
currentParams.reload();
}
}

// watch for changes to $params values (eg when a filter or page changes)
$scope.$watch('params.$params', onParamsChange, true);
// watch for changes to $params reference (eg when a new NgTableParams is bound to the scope)
$scope.$watch('params.$params', onParamsChange, false);

$scope.$watch('params.settings().data', function(newData, oldData){
if (newData === oldData || $scope.params.settings().$loading || pendingReload) {
// watch for when a new NgTableParams is bound to the scope
// CRITICAL: the watch must be for reference and NOT value equality; this is because NgTableParams maintains
// the current data page as a field. Checking this for value equality would be terrible for performance
// and potentially cause an error if the items in that array has circular references
$scope.$watch('params', function(newParams, oldParams){
if (newParams === oldParams || !newParams) {
return;
}

$scope.params.page(1);
$scope.params.reload();
});
newParams.reload();
}, false);

$scope.$watch('params.isDataReloadRequired()', onDataReloadStatusChange);

this.compileDirectiveTemplates = function () {
if (!$element.hasClass('ng-table')) {
Expand Down Expand Up @@ -212,7 +173,6 @@ function($scope, NgTableParams, $timeout, $parse, $compile, $attrs, $element, ng
}
$scope.paramsModel = tableParamsGetter;
$scope.params = params;
$scope.params.$$paramsBootstrapping = true;
}), false);

if ($attrs.showFilter) {
Expand Down
4 changes: 2 additions & 2 deletions test/tableSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ describe('ng-table', function() {
expect(tp.page()).toBe(1);
});

xit('changing filter, orderBy, or page and then calling reload should not invoke getData twice', function(){
it('changing filter, orderBy, or page and then calling reload should not invoke getData twice', function(){

// todo: refactor the watches in ngTableController to handle this case

Expand Down Expand Up @@ -811,7 +811,7 @@ describe('ng-table', function() {
expect(tp.settings().getData.calls.count()).toBe(2);
});

xit('changing filter, orderBy, or page then reload in a callback to reload should re-invoke getData 1 time only', function(){
it('changing filter, orderBy, or page then reload in a callback to reload should re-invoke getData 1 time only', function(){

// todo: refactor the watches in ngTableController to handle this case

Expand Down

0 comments on commit e94ca5f

Please sign in to comment.