Skip to content

Data source adapter, virtual data loader, TreeList data controller and data source adapter - type grid_core data source adapter and utils - #35490

Open
bit-byte0 wants to merge 4 commits into
DevExpress:mainfrom
bit-byte0:refactor/type-data-source-adapter-main
Open

bit-byte0 wants to merge 4 commits into
DevExpress:mainfrom
bit-byte0:refactor/type-data-source-adapter-main

Conversation

@bit-byte0

Copy link
Copy Markdown
Contributor

What

Added strict types to the grid_core data source adapter and its helper utilities, so m_data_source_adapter.ts and m_data_source_adapter_utils.ts pass the strict lint rules while keeping the m_ prefix

How

Annotated fields, parameters and return types and replaced loose any with real types, keeping the widely used public getters unchanged

@bit-byte0
bit-byte0 requested a review from a team as a code owner October 5, 2026 23:43
Copilot AI balanced review requested due to automatic review settings October 5, 2026 23:43
@bit-byte0 bit-byte0 added the 26_2 label Oct 5, 2026
@bit-byte0 bit-byte0 self-assigned this Oct 5, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The type-focused changes preserve existing control flow and remain consistent with related adapters and callers.

Review effort: Balanced
Findings: None

What changed in this PR

Adds strict typing to the grid-core data source adapter and cache utilities without changing public behavior.

Changes:

  • Typed adapter state, handlers, and load operations.
  • Typed cache, paging, and grouping helpers.
  • Added grouped paging metadata to LoadOperation.
File Description
types.ts Adds grouped paging fields.
m_data_source_adapter.ts Types adapter fields and methods.
m_data_source_adapter_utils.ts Types cache and operation utilities.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI balanced review requested due to automatic review settings October 6, 2026 18:40

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The lastLoadOptions() return type incorrectly asserts required fields when returning an empty object.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid asserting required pagination fields before initial load

packages/​devextreme/​js/​__internal/​grids/​grid_core/​data_source_adapter/​m_data_source_adapter.ts:823

This cast makes the getter claim that pageIndex and pageSize always exist, but before the first load _lastLoadOptions is undefined and this branch returns {}. That leaves callers with an unsound numeric contract. Return a partial shape here, matching data_source_controller.ts:182, instead of asserting required fields.

private _currentTotalCount!: number;

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- virtual_scrolling override
protected _items: any;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

replace to unknown[] if we cannot properly type items right now

*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected _changeRowExpandCore(path?: any) {}
protected _changeRowExpandCore(path?: unknown): void {}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lets try get better type from usage

public totalCount(): number {
// eslint-disable-next-line radix
return parseInt((this._currentTotalCount || this._dataSourceTotalCount()) + this._totalCountCorrection);
const count = (this._currentTotalCount || this._dataSourceTotalCount())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lets introduce variable for 1st part, it is difficult to read now

Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The data-index getter incorrectly promises a number although uncached keys return undefined.

Review effort: Balanced
Findings: None

Previously missed (1)

In code that hasn't changed since last review

Medium severity Allow data index getter to return undefined

packages/​devextreme/​js/​__internal/​grids/​grid_core/​data_source_adapter/​m_data_source_adapter.ts:330

The callback can return undefined when store.keyOf(data) is absent from _dataIndexByKey, but the new annotation promises a number. This hides a real result that sort consumers can receive. Please type _dataIndexGetter, this callback, and getDataIndexGetter() as returning number | undefined.

Copilot AI balanced review requested due to automatic review settings October 7, 2026 09:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Two new type contracts promise capabilities or defined results that their implementations do not guarantee.

Review effort: Balanced
Findings: None

Previously missed (2)

In code that hasn't changed since last review

Medium severity Allow data index getter to return undefined

packages/​devextreme/​js/​__internal/​grids/​grid_core/​data_source_adapter/​m_data_source_adapter.ts:68

The getter can return undefined: when _cachedStoreData is unset or does not contain the requested key, the record lookup in getDataIndexGetter has no value. Declaring this callback as always returning number hides that valid outcome from callers. Please use number | undefined consistently in this field, getDataIndexGetter, and the delegating controller signature.

Medium severity Narrow extension-point return type to key-info shape

packages/​devextreme/​js/​__internal/​grids/​grid_core/​data_source_adapter/​m_data_source_adapter.ts:351

This extension-point contract is broader than the object it actually guarantees. The TreeList override returns only key and keyOf (and currently has to cast that object to Store), while this method is consumed solely as applyBatch key information. Typing it as a full Store can let future callers invoke methods that are absent at runtime; narrow the return type to the key-info shape.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants