Skip to content

Commit

Permalink
fix(backend): cancel prior http calls to avoid inconsistent data shown
Browse files Browse the repository at this point in the history
- when user calls multiple requests, that could happen after typing new input search term in a filter before the previous query is finished and/or calls the (Enter) key after typing his search term. Calling and queueing multiple queries might render the grid multiple times with different dataset and produces a flashing effect, it could also in some cases display incorrect data, that could happen if for example we have 2 queries calle in sequence and query 2 resolves before query 1, that in terms end up displaying the wrong dataset
  • Loading branch information
ghiscoding-SE committed Jan 15, 2020
1 parent 6de44e6 commit f9aaf54
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -477,7 +477,7 @@ describe('FilterService', () => {
expect(service.getColumnFilters()).toEqual({});
expect(spyFilterChange).not.toHaveBeenCalled();
expect(spyEmitter).not.toHaveBeenCalled();
expect(consoleSpy).toHaveBeenCalledWith(expect.toInclude('[Angular-Slickgrid] please note that the "processOnFilterChanged" method signature, from Backend Service'));
expect(consoleSpy).toHaveBeenCalledWith(expect.toInclude('[Angular-Slickgrid] please note that the "processOnFilterChanged" from your Backend Service, should now return a string instead of a Promise.'));
done();
});
});
Expand Down
22 changes: 16 additions & 6 deletions src/app/modules/angular-slickgrid/services/backend-utilities.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { isObservable } from 'rxjs';
import { EMPTY, iif, isObservable, Subject } from 'rxjs';
import { takeUntil } from 'rxjs/operators';

import { BackendServiceApi, EmitterType, GraphqlResult, GraphqlPaginatedResult, GridOption } from '../models';

Expand Down Expand Up @@ -44,7 +45,7 @@ main.onBackendError = function backendError(e: any, backendApi: BackendServiceAp
* Execute the backend callback, which are mainly the "process" & "postProcess" methods.
* Also note that "preProcess" was executed prior to this callback
*/
main.executeBackendCallback = function exeBackendCallback(backendServiceApi: BackendServiceApi, query: string, args: any, startTime: Date, totalItems: number, emitActionChangedCallback?: (type: EmitterType) => void) {
main.executeBackendCallback = function exeBackendCallback(backendServiceApi: BackendServiceApi, query: string, args: any, startTime: Date, totalItems: number, emitActionChangedCallback?: (type: EmitterType) => void, httpCancelRequests$?: Subject<void>) {
if (backendServiceApi) {
// emit an onFilterChanged event when it's not called by a clear filter
if (args && !args.clearFilterTriggered) {
Expand All @@ -57,10 +58,19 @@ main.executeBackendCallback = function exeBackendCallback(backendServiceApi: Bac
process.then((processResult: GraphqlResult | GraphqlPaginatedResult | any) => main.executeBackendProcessesCallback(startTime, processResult, backendServiceApi, totalItems))
.catch((error: any) => main.onBackendError(error, backendServiceApi));
} else if (isObservable(process)) {
process.subscribe(
(processResult: GraphqlResult | GraphqlPaginatedResult | any) => main.executeBackendProcessesCallback(startTime, processResult, backendServiceApi, totalItems),
(error: any) => main.onBackendError(error, backendServiceApi)
);
// this will abort any previous HTTP requests, that were previously hooked in the takeUntil, before sending a new request
if (isObservable(httpCancelRequests$)) {
httpCancelRequests$.next();
}

process
// the following takeUntil, will potentially be used later to cancel any pending http request (takeUntil another rx, that would be httpCancelRequests$, completes)
// but make sure the observable is actually defined with the iif condition check before piping it to the takeUntil
.pipe(takeUntil(iif(() => isObservable(httpCancelRequests$), httpCancelRequests$, EMPTY)))
.subscribe(
(processResult: GraphqlResult | GraphqlPaginatedResult | any) => main.executeBackendProcessesCallback(startTime, processResult, backendServiceApi, totalItems),
(error: any) => main.onBackendError(error, backendServiceApi)
);
}
}
};
Expand Down
23 changes: 15 additions & 8 deletions src/app/modules/angular-slickgrid/services/filter.service.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { Injectable } from '@angular/core';
import { Subject } from 'rxjs';
import { isObservable, Subject } from 'rxjs';

import * as isequal_ from 'lodash.isequal';
const isequal = isequal_; // patch to fix rollup to work

Expand Down Expand Up @@ -46,6 +47,7 @@ export class FilterService {
private _dataView: any;
private _grid: any;
private _onSearchChange: SlickEvent;
private httpCancelRequests$: Subject<void> = new Subject<void>(); // this will be used to cancel any pending http request
onFilterChanged = new Subject<CurrentFilter[]>();
onFilterCleared = new Subject<boolean>();

Expand Down Expand Up @@ -90,6 +92,9 @@ export class FilterService {
if (this._eventHandler && this._eventHandler.unsubscribeAll) {
this._eventHandler.unsubscribeAll();
}
if (isObservable(this.httpCancelRequests$)) {
this.httpCancelRequests$.next(); // this cancels any pending http requests
}
}

/**
Expand Down Expand Up @@ -222,8 +227,8 @@ export class FilterService {
const queryResponse = backendApi.service.processOnFilterChanged(undefined, callbackArgs as FilterChangedArgs);
if (queryResponse instanceof Promise && queryResponse.then) {
// @deprecated, processOnFilterChanged in the future should be returned as a query string NOT as a Promise
console.warn(`[Angular-Slickgrid] please note that the "processOnFilterChanged" method signature, from Backend Service, should return a string instead of a Promise,
returning a Promise will be deprecated in the future.`);
console.warn(`[Angular-Slickgrid] please note that the "processOnFilterChanged" from your Backend Service, should now return a string instead of a Promise.
Returning a Promise will be deprecated in the future.`);
queryResponse.then((query: string) => {
const totalItems = this._gridOptions && this._gridOptions.pagination && this._gridOptions.pagination.totalItems;
executeBackendCallback(backendApi, query, callbackArgs, new Date(), totalItems, this.emitFilterChanged.bind(this));
Expand Down Expand Up @@ -426,25 +431,27 @@ export class FilterService {
let debounceTypingDelay = 0;
const isTriggeredByClearFilter = args && args.clearFilterTriggered; // was it trigger by a "Clear Filter" command?

if (!isTriggeredByClearFilter && event && event.keyCode !== KeyCode.ENTER && (event.type === 'input' || event.type === 'keyup' || event.type === 'keydown')) {
const eventType = event && event.type;
const eventKeyCode = event && event.keyCode;
if (!isTriggeredByClearFilter && eventKeyCode !== KeyCode.ENTER && (eventType === 'input' || eventType === 'keyup' || eventType === 'keydown')) {
debounceTypingDelay = backendApi.hasOwnProperty('filterTypingDebounce') ? backendApi.filterTypingDebounce as number : DEFAULT_FILTER_TYPING_DEBOUNCE;
}

// query backend, except when it's called by a ClearFilters then we won't
if (args && args.shouldTriggerQuery) {
// call the service to get a query back
// TODO: remove async/await on next major change, refer to processOnFilterChanged in BackendService interface (with @deprecated)
// @deprecated TODO: remove async/await on next major change, refer to processOnFilterChanged in BackendService interface (with @deprecated)
clearTimeout(timer);
if (debounceTypingDelay > 0) {
clearTimeout(timer);
timer = setTimeout(async () => {
const query = await backendApi.service.processOnFilterChanged(event, args);
const totalItems = this._gridOptions && this._gridOptions.pagination && this._gridOptions.pagination.totalItems;
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitFilterChanged.bind(this));
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitFilterChanged.bind(this), this.httpCancelRequests$);
}, debounceTypingDelay);
} else {
const query = await backendApi.service.processOnFilterChanged(event, args);
const totalItems = this._gridOptions && this._gridOptions.pagination && this._gridOptions.pagination.totalItems;
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitFilterChanged.bind(this));
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitFilterChanged.bind(this), this.httpCancelRequests$);
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions src/app/modules/angular-slickgrid/services/sort.service.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Subject } from 'rxjs';
import { isObservable, Subject } from 'rxjs';

import {
Column,
Expand All @@ -25,6 +25,7 @@ export class SortService {
private _dataView: any;
private _grid: any;
private _isBackendGrid = false;
private httpCancelRequests$: Subject<void> = new Subject<void>(); // this will be used to cancel any pending http request
onSortChanged = new Subject<CurrentSorter[]>();
onSortCleared = new Subject<boolean>();

Expand Down Expand Up @@ -133,6 +134,9 @@ export class SortService {
if (this._eventHandler && this._eventHandler.unsubscribeAll) {
this._eventHandler.unsubscribeAll();
}
if (isObservable(this.httpCancelRequests$)) {
this.httpCancelRequests$.next(); // this cancels any pending http requests
}
}

/**
Expand Down Expand Up @@ -236,7 +240,7 @@ export class SortService {
// query backend
const query = backendApi.service.processOnSortChanged(event, args);
const totalItems = gridOptions && gridOptions.pagination && gridOptions.pagination.totalItems;
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitSortChanged.bind(this));
executeBackendCallback(backendApi, query, args, startTime, totalItems, this.emitSortChanged.bind(this), this.httpCancelRequests$);
}

onLocalSortChanged(grid: any, dataView: any, sortColumns: ColumnSort[], forceReSort = false) {
Expand Down

0 comments on commit f9aaf54

Please sign in to comment.