GridCore data: Type DataSourceAdapter public methods and props - #35013
GridCore data: Type DataSourceAdapter public methods and props#35013bit-byte0 wants to merge 4 commits into
DataSourceAdapter public methods and props#35013Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The changes are type-safety improvements that consistently replace prior suppressions with explicit, compatible typings without introducing apparent behavioral changes.
Pull request overview
This PR strengthens TypeScript typing around the grids’ DataSourceAdapter surface (including TreeList and virtual scrolling), replacing prior @ts-expect-error suppressions with explicit adapter types and typed public members so downstream grid code can use these members safely.
Changes:
- Introduced concrete adapter instance types (e.g.,
VirtualScrollingDataSourceAdapter,SummaryDataSourceAdapter,GroupingDataSourceAdapter) and used them to type_dataSourcein relevant controller extenders. - Promoted/typed adapter members that are accessed across modules (e.g.,
_virtualScrollController,_renderTime,_isNodesInitializing, grouping helpers) to eliminate type suppressions. - Tightened signatures for commonly used adapter/controller methods (
isLoaded,pageIndex,push, count getters), removing unnecessary casts at call sites.
File summaries
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/tree_list/m_virtual_scrolling.ts | Types TreeList virtual-scrolling controller _dataSource and removes prior suppressions. |
| packages/devextreme/js/__internal/grids/tree_list/m_focus.ts | Removes @ts-expect-error by relying on a typed _isNodesInitializing flag. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts | Makes _isNodesInitializing a typed public boolean for TreeList adapter coordination. |
| packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts | Removes suppression around _isNodesInitializing usage when deciding to reload on option change. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts | Exposes a virtual-scrolling adapter instance type and makes accessed members (_virtualScrollController, _renderTime) public/typed. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling_core.ts | Makes getItemIndexByPosition public to support typed cross-component calls. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/index.ts | Re-exports the new virtual-scrolling adapter instance type. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/extenders/virtual_scrolling_data_controller.ts | Uses the exported adapter instance type and removes the prior local alias + suppression. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts | Adds explicit return/parameter types for adapter methods used by consumers (counts, paging, loading, expand). |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/types.ts | Removes an unused DataSourceAdapterLike type definition. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Removes unnecessary casts/suppressions by relying on the typed adapter surface; refines push signature. |
| packages/devextreme/js/__internal/grids/data_grid/summary/m_summary.ts | Adds a summary adapter instance type and adjusts extender typing to enable it. |
| packages/devextreme/js/__internal/grids/data_grid/summary/extenders/summary_data_controller.ts | Types _dataSource as the summary adapter and removes suppressions around totalAggregates(). |
| packages/devextreme/js/__internal/grids/data_grid/grouping/m_grouping.ts | Adds a grouping adapter instance type; exposes grouping members/methods needed by controllers. |
| packages/devextreme/js/__internal/grids/data_grid/grouping/extenders/grouping_data_controller.ts | Types _dataSource as the grouping adapter and removes suppressions for grouping adapter calls. |
| packages/devextreme/js/__internal/grids/data_grid/focus/m_focus.ts | Types _dataSource for focus logic and replaces loose runtime checks with safer array handling. |
Review details
- Files reviewed: 16/16 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
TreeList virtual-scrolling changeRowExpand now uses optional chaining on .done(...), widening the return type to possibly undefined even though callers expect a Deferred.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/devextreme/js/__internal/grids/tree_list/m_virtual_scrolling.ts:55
changeRowExpandnow uses optional chaining on the Deferred (?.done(...)), which widens the inferred return type toDeferredObj | undefined. TreeList callers expect a Deferred (e.g.tree_list/data_controller/m_data_controller.tscalls.done(...)on the result), so returningundefinedhere would break that contract and also prevents the viewport index update callback from running in that case.
Since the TreeList adapter’s changeRowExpand implementation always returns a Deferred, drop the optional chaining so this override consistently returns a Deferred too.
const dataSourceAdapterExtender = (Base: ModuleType<DataSourceAdapter>) => class VirtualScrollingDataSourceAdapterExtender extends virtualScrollingDataSourceAdapterExtender(Base) {
public changeRowExpand() {
return super.changeRowExpand.apply(this, arguments as any)?.done(() => {
const viewportItemIndex = this.getViewportItemIndex();
viewportItemIndex >= 0 && this.setViewportItemIndex(viewportItemIndex);
});
- Files reviewed: 16/16 changed files
- Comments generated: 0 new
- Review effort level: Lite
| } | ||
|
|
||
| if (value === undefined) { | ||
| return dataSource[optionName]() as number; |
There was a problem hiding this comment.
Let's add return type to DataController.dataSource() :
|
|
||
| // @ts-expect-error badly typed DataSourceAdapter | ||
| const groupPath = this._getGroupPath(data, group.length); | ||
| const groupPath = this._getGroupPath(data, gridCore.normalizeSortingInfo(group).length); |
There was a problem hiding this comment.
was adding normalizeSortingInfo intentional? Why is it needed?
There was a problem hiding this comment.
yes - with the @ts-expect-error gone, group() is typed GroupDescriptor | GroupDescriptor[] | undefined, which has no .length. normalizeSortingInfo(group) gives an array to count from, same as in m_grouping.ts
There was a problem hiding this comment.
public method, please add types
There was a problem hiding this comment.
Also some other public methods weren't typed (e.g. customizeLoadResult, lastLoadOptions, totalCountCorrection, etc). Please add types to public methods of DataSourceAdapter
| const dataSourceAdapterExtender = (Base: ModuleType<DataSourceAdapter>) => class VirtualScrollingDataSourceAdapterExtender extends virtualScrollingDataSourceAdapterExtender(Base) { | ||
| public changeRowExpand() { | ||
| return super.changeRowExpand.apply(this, arguments as any).done(() => { | ||
| return super.changeRowExpand.apply(this, arguments as any)?.done(() => { |
There was a problem hiding this comment.
Explicitly pass args to changeRowExpand:
return super.changeRowExpand(path)
…ntroller.dataSource()
There was a problem hiding this comment.
🟡 Changes recommended
There is a confirmed behavior risk in TreeList editing where parentKey can be silently overwritten to undefined, potentially inserting rows under the wrong parent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts:518
_getKeyInfo()returns a minimal{ key, keyOf }object, but it is cast toStore(a full store type). Narrow the cast to just the members actually provided/required (e.g.Pick<Store, 'key' | 'keyOf'>) to avoid treating this object as a full store elsewhere.
- Files reviewed: 19/19 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
A few newly introduced public TypeScript signatures are currently unsound relative to their implementations (notably getDataIndexGetter, lastLoadOptions, and cachedData.items indexing), which undermines the PR’s stated goal of reliable consumer typing.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts:317
getDataIndexGettercan returnundefinedwhen the key is not present in_dataIndexByKey(see return expression inside the getter). The current return type forces callers to assume anumber, which is not guaranteed by the implementation.
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts:779lastLoadOptions()returns{}when_lastLoadOptionsis not initialized, but the declared return type (NonNullable<LoadOperation['lastLoadOptions']>) implies required paging fields are always present. This makes the public typing unsound for callers that read e.g..skip/.filter/.pageIndex.
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:61cachedData.itemsis accessed throughout the cache helpers using numeric indices (e.g.cachedItems[cacheItemIndex],cacheItems[globalIndex]). Typing it asRecord<string, unknown>makes numeric indexing ill-typed and encouragesanycasts; a numeric index signature better matches actual usage.
- Files reviewed: 19/19 changed files
- Comments generated: 0 new
- Review effort level: Lite
What
Adds explicit TypeScript types to the public methods and props of the grids
DataSourceAdapter, so consumers no longer fall back to untyped accessHow
Typed the adapter surface consumers rely on (
pageIndex,isLoaded,changeRowExpand,push, and the count getters) and exposed the virtual-scrolling, grouping, summary and TreeList adapter members through their concrete types, removing the temporary type suppressions at the call sites