Grids - DataController: Extract scrolling knowledge to extenders - #35130
Grids - DataController: Extract scrolling knowledge to extenders#35130bit-byte0 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The newly added test’s ExposedDataController extends DataController redeclares protected members as public, which can cause a TypeScript inheritance error and break compilation.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR refactors grid paging logic so the base DataController no longer depends on scrolling options, delegating paging and total-count decisions to overridable hooks implemented by the virtual scrolling extender.
Changes:
- Added
resolvePaginate()andrequiresTotalCount()hooks toDataControllerand routed paging decisions through them. - Moved scrolling-specific paging/total-count behavior into the virtual scrolling data controller extender (including handling
scrollingoption changes). - Updated unit tests by removing
resolvePaginateutility coverage and adding extender-focused paging tests.
File summaries
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/extenders/virtual_scrolling_data_controller.ts | Implements paging/total-count hook overrides and handles scrolling option changes within the virtual scrolling extender. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/extenders/tests/virtual_scrolling_data_controller.paging.test.ts | Adds new Jest coverage for virtual scrolling’s paging/total-count hook behavior and scrolling option reset behavior. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/utils/paging.ts | Removes resolvePaginate utility to keep paging utils independent of scrolling knowledge. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/utils/tests/paging.test.ts | Drops tests for the removed resolvePaginate utility, leaving syncPaging coverage. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Removes scrolling handling from optionChanged and introduces overridable paging/total-count hooks used by base paging logic. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- 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
The newly added Jest test introduces a TypeScript type/visibility mismatch (widening protected hooks to public) that can break compilation.
Review details
Suppressed comments (1)
packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/extenders/tests/virtual_scrolling_data_controller.paging.test.ts:16
- The test declares
ExposedDataController extends DataControllerand makesresolvePaginate/requiresTotalCountpublic, but those hooks areprotectedonDataController. TypeScript will treat this as an invalid inheritance (visibility widening) and can fail compilation for the test.
declare class ExposedDataController extends DataController {
public resolvePaginate(enabled: boolean | undefined): boolean | undefined;
public requiresTotalCount(): boolean;
}
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
6efe76f to
3ffb530
Compare
What
Moves the
scrollingoption knowledge out of the base gridDataControllerpaging logic into the virtual scrolling module, so the base data layer no longer depends on scrollingHow
Base paging now delegates the paginate and total-count decisions to overridable
resolvePaginateandrequiresTotalCounthooks, and thescrollingoption reset tooptionChanged, which the virtual scrolling extender implements using its own scrolling-mode predicates