GridCore - DataController - Add types (processItems pipeline)- #34693 - #34719
GridCore - DataController - Add types (processItems pipeline)- #34693#34719Tucchhaa wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR strengthens TypeScript typings across the GridCore DataController “processItems → generate/process data item” pipeline, and updates several grid mixins to use the new typed contracts.
Changes:
- Introduces
RowGenerationOptions,GeneratedDataItem, andProcessedDataItemtypes and updatesDataControllermethods to use them. - Updates GridCore features (virtual scrolling, selection, master-detail, adaptivity, editing, filter sync) to use the typed pipeline methods instead of
apply(this, arguments as any). - Adds/adjusts return types for Deferred-based APIs (
_applyFilter,DataSource.store).
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts | Adds a TS suppression on TreeList’s _generateDataItem override (needs proper signature typing). |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts | Types _processItems/_afterProcessItems and _applyFilter for virtual scrolling pipeline. |
| packages/devextreme/js/__internal/grids/grid_core/selection/m_selection.ts | Types _processDataItem to operate on GeneratedDataItem/ProcessedDataItem. |
| packages/devextreme/js/__internal/grids/grid_core/master_detail/m_master_detail.ts | Types master-detail _processItems/_processDataItem and inserts detail rows. |
| packages/devextreme/js/__internal/grids/grid_core/filter/m_filter_sync.ts | Types _applyFilter to Deferred-based return and removes arguments-based calls. |
| packages/devextreme/js/__internal/grids/grid_core/editing/types.ts | Replaces Item with ProcessedDataItem in editing-related types. |
| packages/devextreme/js/__internal/grids/grid_core/editing/m_editing.ts | Types editing controller hooks (processItems, processDataItem) and data-controller overrides. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/types.ts | Adds new pipeline types (RowGenerationOptions, GeneratedDataItem, ProcessedDataItem). |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Refactors core pipeline methods to return typed processed items and updates Deferred return types. |
| packages/devextreme/js/__internal/grids/grid_core/ai_column/utils.ts | Updates helper signature to accept ProcessedDataItem[]. |
| packages/devextreme/js/__internal/grids/grid_core/ai_column/utils.test.ts | Updates test typing to match ProcessedDataItem. |
| packages/devextreme/js/__internal/grids/grid_core/adaptivity/m_adaptivity.ts | Types _processItems and preserves values when inserting adaptive detail rows. |
| packages/devextreme/js/__internal/data/data_source/m_data_source.ts | Types store() return and aligns Deferred usage in load flow. |
💡 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.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts:26
- The
nodeparameter is marked optional, but the implementation dereferences it (node.key,node.data) without null checks. Making it optional weakens type safety and suggests callers may passundefined, which would crash at runtime.
protected _generateDataItem(node?: any, options?: any): any {
packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts:769
newRowsis typed asany[], which reintroducesanyinto an otherwise typedProcessedDataItempipeline and makesloadIndexmutations unchecked. It can be typed precisely asProcessedDataItem[].
let newRows: any = [];
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts:713
- Avoid suppressing the type error here:
changeType: 'append'is a real runtime value (see virtual_data_loader fireChanged with changeType 'append'), so it should be represented in the method’s parameter type instead of using@ts-expect-error. This keeps the typing accurate and removes the need for the suppression.
protected getDataIndex(change: DataChange | { changeType: 'loadingAll' }): number {
const visibleItems = this._items;
// @ts-expect-error changeType can be 'append' only when virtual scrolling with scrolling.legacyMode are enabled
const lastVisibleItem = change.changeType === 'append' && visibleItems.length > 0 ? visibleItems[visibleItems.length - 1] : null;
packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts:26
- This override is part of the DataController item-processing pipeline, but the new signature still uses optional
anyparameters and returnsany, which undermines the purpose of adding pipeline types. It’s also misleading to makenodeoptional since the implementation immediately dereferences it. Consider typing it to the shared ItemProcessingOptions/GeneratedItem types (even ifnodeitself remainsany).
protected _generateDataItem(node?: any, options?: any): any {
| protected getDataIndex(change: DataChange | { changeType: 'loadingAll' }): number { | ||
| const visibleItems = this._items; | ||
| // @ts-expect-error changeType can be 'append' only when virtual scrolling with scrolling.legacyMode are enabled | ||
| const lastVisibleItem = change.changeType === 'append' && visibleItems.length > 0 ? visibleItems[visibleItems.length - 1] : null; |
There was a problem hiding this comment.
looks like lastVisibleItem may be not null only in virtual scrolling with legacy mode. Shouldn't it then be moved there? and in base it should always return 0?
nitpick:
| const lastVisibleItem = change.changeType === 'append' && visibleItems.length > 0 ? visibleItems[visibleItems.length - 1] : null; | |
| const lastVisibleItem = change.changeType === 'append' && visibleItems.length > 0 ? visibleItems.at(-1) : null; |
|
|
||
| for (let index = 0; index < columnCount; index++) { | ||
| for (let index = 0; index < columnCount; index += 1) { | ||
| columnsController.columnOption(index, optionName, undefined); |
There was a problem hiding this comment.
each columnOption method call will trigger columnsChanged event
the whole clearColumnOption looks like candidate to be extracted as columnsController method, and here later we will be able to introduce some decision how to trigger event just once (I think it is ok just to highlight it inside columnsController and fix it later during columnsController refactoring )
There was a problem hiding this comment.
each columnOption method call will trigger columnsChanged event
No, it won't because of updateLockCount in fireColumnsChanged and beginUpdate/endUpdate wrapping
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts:26
_generateDataItemis now typed with optional params and ananyreturn type, which effectively disables the type-safety this PR is trying to introduce. Since this override always expects a node and returns a well-known object shape, it should have a concrete signature (at least avoidingany).
protected _generateDataItem(node?: any, options?: any): any {
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devextreme/js/__internal/grids/tree_list/data_controller/m_data_controller.ts:26
nodeis marked optional, but the implementation dereferences it unconditionally (node.key,node.data,node.parent). Keeping it optional weakens type-safety and can mask real call-site issues. Makenodea required parameter.
protected _generateDataItem(node?: any, options?: any): any {
No description provided.