Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

External loading anand #2575

Draft
wants to merge 2 commits into
base: feat-provide-external-loading-triggers
Choose a base branch
from
Draft

External loading anand #2575

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
f86c0a7
refactor: adding triggers to datasource
anandtiwary Dec 11, 2023
50bf809
refactor: add take(1)
anandtiwary Dec 12, 2023
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
48 changes: 39 additions & 9 deletions projects/components/src/table/data/table-cdk-data-source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { Dictionary, forkJoinSafeEmpty, isEqualIgnoreFunctions, RequireBy, sortUnknown } from '@hypertrace/common';
import { isEqual, isNil } from 'lodash-es';
import { BehaviorSubject, combineLatest, NEVER, Observable, of, Subject, Subscription, throwError } from 'rxjs';
import { catchError, debounceTime, map, mergeMap, startWith, switchMap, tap } from 'rxjs/operators';
import { catchError, debounceTime, map, mergeMap, startWith, switchMap, take, tap } from 'rxjs/operators';
import { PageEvent } from '../../paginator/page.event';
import { PaginationProvider } from '../../paginator/paginator-api';
import { RowStateChange, StatefulTableRow, StatefulTreeTableRow, TableFilter, TableRow } from '../table-api';
Expand All @@ -25,6 +25,7 @@
Dictionary<unknown>,
TableColumnConfigExtended | undefined,
StatefulTableRow | undefined,
unknown | undefined,
];

export class TableCdkDataSource implements DataSource<TableRow> {
Expand Down Expand Up @@ -63,8 +64,17 @@
* Below debounce is needed to handle multiple emission from buildChangeObservable.
*/
debounceTime(100),
mergeMap(([columnConfigs, pageEvent, filters, queryProperties, changedColumn, changedRow]) =>
this.buildDataObservable(columnConfigs, pageEvent, filters, queryProperties, changedColumn, changedRow),
mergeMap(
([columnConfigs, pageEvent, filters, queryProperties, changedColumn, changedRow, secondaryDataTriggers]) =>
this.buildDataObservable(
columnConfigs,
pageEvent,
filters,
queryProperties,
changedColumn,
changedRow,
secondaryDataTriggers,
),
),
)
.subscribe(this.rowsChange$);
Expand Down Expand Up @@ -174,7 +184,12 @@
this.filtersProvider.queryProperties$,
this.columnStateChangeProvider.columnState$,
this.rowStateChangeProvider.rowState$,
]).pipe(map(values => this.detectRowStateChanges(...values)));
]).pipe(
switchMap(values =>
combineLatest([of(values), this.tableDataSourceProvider.data?.secondaryDataTriggers$ ?? of(undefined)]),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we are using combineLatest, in terms of emitting the loading state value, it will have affect the same way as 2 subscriptions, one to the existing change observable and one to secondaryDataTriggers$.

Correct me if I am wrong.

),
map(([values, triggers]) => this.detectRowStateChanges(...values, triggers)),
);
}

private columnConfigChange(): Observable<TableColumnConfigExtended[]> {
Expand Down Expand Up @@ -204,8 +219,17 @@
queryProperties: Dictionary<unknown>,
changedColumn: TableColumnConfigExtended | undefined,
changedRow: StatefulTableRow | undefined,
secondaryDataTriggers?: unknown,
): WatchedObservables {
return [columnConfigs, pageEvent, filters, queryProperties, changedColumn, this.buildRowStateChange(changedRow)];
return [
columnConfigs,
pageEvent,
filters,
queryProperties,
changedColumn,
this.buildRowStateChange(changedRow),
secondaryDataTriggers,
];
}

private buildRowStateChange(changedRow: StatefulTableRow | undefined): StatefulTableRow | undefined {
Expand All @@ -232,6 +256,7 @@
queryProperties: Dictionary<unknown>,
changedColumn: TableColumnConfigExtended | undefined,
changedRow: StatefulTableRow | undefined,
secondaryDataTriggers?: unknown,
): Observable<StatefulTableRow[]> {
if (changedRow !== undefined) {
return of(this.cachedData.rows).pipe(
Expand All @@ -246,7 +271,7 @@
TableCdkColumnUtil.unsortOtherColumns(changedColumn, columnConfigs);
}

return this.fetchData(columnConfigs, pageEvent, filters, queryProperties);
return this.fetchData(columnConfigs, pageEvent, filters, queryProperties, secondaryDataTriggers);
}

private fetchAndAppendNewChildren(stateChanges: RowStateChange[]): Observable<StatefulTableRow[]> {
Expand Down Expand Up @@ -277,14 +302,17 @@
pageEvent: PageEvent,
filters: TableFilter[],
queryProperties: Dictionary<unknown>,
secondaryDataTriggers?: unknown,
): Observable<StatefulTableRow[]> {
if (this.tableDataSourceProvider.data === undefined) {
return of([]);
}

const request = this.buildRequest(columnConfigs, pageEvent, filters, queryProperties);

return this.hasCacheForRequest(request) ? this.fetchCachedData(request) : this.fetchNewData(request);
return this.hasCacheForRequest(request)
? this.fetchCachedData(request)

Check warning on line 314 in projects/components/src/table/data/table-cdk-data-source.ts

View check run for this annotation

Codecov / codecov/patch

projects/components/src/table/data/table-cdk-data-source.ts#L314 

Added line #L314 was not covered by tests
: this.fetchNewData(request, secondaryDataTriggers);
}

private haveColumConfigsChanged(request: TableDataRequest): boolean {
Expand All @@ -296,6 +324,7 @@
}

private hasCacheForRequest(request: TableDataRequest): boolean {
return false; //TBD: why do we need this method?
if (
!isEqual(this.cachedData.request?.sort, request.sort) ||
!isEqual(this.cachedData.request?.filters, request.filters) ||
Expand Down Expand Up @@ -325,14 +354,15 @@
return currentOffset - cachedOffset;
}

private fetchNewData(request: TableDataRequest): Observable<StatefulTableRow[]> {
private fetchNewData(request: TableDataRequest, secondaryDataTriggers?: unknown): Observable<StatefulTableRow[]> {
if (this.tableDataSourceProvider.data === undefined) {
return of([]);
}

let total = 0;

return this.tableDataSourceProvider.data.getData(request).pipe(
return this.tableDataSourceProvider.data.getData(request, secondaryDataTriggers).pipe(
take(1),
tap(response => (total = response.totalCount)),
tap(response => this.updatePaginationTotalCount(response.totalCount)),
map(response => response.data),
Expand Down
8 changes: 6 additions & 2 deletions projects/components/src/table/data/table-data-source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
import { Observable } from 'rxjs';
import { TableColumnConfig, TableFilter, TableSortDirection } from '../table-api';

export interface TableDataSource<TResult, TCol extends TableColumnConfig = TableColumnConfig> {
getData(request: TableDataRequest<TCol>): Observable<TableDataResponse<TResult>>;
export interface TableDataSource<TResult, TCol extends TableColumnConfig = TableColumnConfig, TDataTriggers = unknown> {
secondaryDataTriggers$?: Observable<TDataTriggers>; // TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, this is fine how it is used here. Adding it to watched observables in the data source is sound logic. However, does it give us the intended effect. Do we show a loader while the new data is being resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

getData(
request: TableDataRequest<TCol>,
secondaryDataTriggers?: TDataTriggers,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to avoid this by making new request happen on a trigger change within the table. Is that something we can do? Getting the triggers as part of data source and then the consumer having to implement some logic dependent on them seems wierd.

We should not have to emit filters via these triggers again. These triggers can play the simple role of being signals while the getData method's request argument having the desired state always.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should be able to avoid this by making new request happen on a trigger change within the table

This is exactly what we are doing. For requests triggered externally, we do need some state (e.g show inactive boolean). The purpose of this argument is to pass on that state to the getData method so that it can be used to build the correct gql query.

): Observable<TableDataResponse<TResult>>;
getScope?(): string | undefined;
}

Expand Down
Loading