External loading anand #2575
External loading anand #2575
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feat-provide-external-loading-triggers #2575 +/- ##
==========================================================================
- Coverage 82.20% 82.17% -0.03%
==========================================================================
Files 927 927
Lines 20879 20882 +3
Branches 3316 3319 +3
==========================================================================
- Hits 17163 17160 -3
- Misses 3579 3584 +5
- Partials 137 138 +1 ☔ View full report in Codecov by Sentry. |
| 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 |
There was a problem hiding this comment.
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?
| secondaryDataTriggers$?: Observable<TDataTriggers>; // TBD | ||
| getData( | ||
| request: TableDataRequest<TCol>, | ||
| secondaryDataTriggers?: TDataTriggers, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| ]).pipe(map(values => this.detectRowStateChanges(...values))); | ||
| ]).pipe( | ||
| switchMap(values => | ||
| combineLatest([of(values), this.tableDataSourceProvider.data?.secondaryDataTriggers$ ?? of(undefined)]), |
There was a problem hiding this comment.
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.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.