Grids: move _stateStoringController definition from base dataController - #34752
Grids: move _stateStoringController definition from base dataController#34752anna-shakhova wants to merge 4 commits into
Conversation
bb213c8 to
bb36f4a
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the Grid state storing implementation by extracting the _stateStoringController-related DataController logic into a dedicated state-storing DataController extender, and reorganizes the state-storing module definition into a standalone module file. This aligns the state-storing responsibilities with the module/extender architecture rather than keeping them in the base DataController.
Changes:
- Split the state-storing “data controller” extender into
state_storing/extenders/state_storing_data_controller.tsand updated state-storing module wiring accordingly. - Introduced
state_storing_module.tsas the canonicalstateStoringModuleexport and updated TreeList/DataGrid module registrations to consume it. - Updated various imports/types and tightened several method/property typings (notably around
_refreshDataSource()and loading flags).
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.pivotGrid/dataController.tests.js | Updated PivotGrid test to import the StateStoringController from its new location. |
| packages/devextreme/js/__internal/grids/tree_list/m_state_storing.ts | Switched to the new stateStoringModule export and updated controller type import. |
| packages/devextreme/js/__internal/grids/pivot_grid/data_controller/m_data_controller.ts | Updated StateStoringController import path for PivotGrid’s internal controller usage. |
| packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts | Adjusted _refreshDataSource() typing/behavior and typed stateLoaded usage via a new extender interface. |
| packages/devextreme/js/__internal/grids/grid_core/state_storing/state_storing_module.ts | Added new central stateStoringModule definition (controllers + extenders wiring). |
| packages/devextreme/js/__internal/grids/grid_core/state_storing/m_state_storing.ts | Removed embedded module definition and extracted data-extender logic; exported extenders for module composition. |
| packages/devextreme/js/__internal/grids/grid_core/state_storing/m_state_storing_controller.ts | Tightened typing for _isLoading and isLoading() return type. |
| packages/devextreme/js/__internal/grids/grid_core/state_storing/extenders/state_storing_data_controller.ts | Added new DataController extender that owns state-storing load/refresh coordination and stateLoaded callback. |
| packages/devextreme/js/__internal/grids/grid_core/selection/m_selection.ts | Updated StateStoringController type import path. |
| packages/devextreme/js/__internal/grids/grid_core/m_types.ts | Updated Controllers typing to reference the new StateStoringController import path. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_helper_mixin.ts | Updated _refreshDataSource() signature to return `DeferredObj |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Removed base _stateStoringController initialization and tightened isLoading() return type. |
| packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller.ts | Updated StateStoringController type import path. |
| packages/devextreme/js/__internal/grids/data_grid/module_not_extended/state_storing.ts | Updated DataGrid module registration to use the new stateStoringModule export. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bb36f4a to
171f478
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
packages/devextreme/js/__internal/grids/grid_core/state_storing/extenders/state_storing_data_controller.ts:20
- The comment says
stateLoadedis defined inStateStoringRowsViewExtender.init(), but that method only subscribes tostateLoaded; the callback is actually created by the controller base based oncallbackNames()(seecallbackNames(): string[]in this extender). This comment is misleading and makes the lifecycle harder to understand.
// Defined in StateStoringRowsViewExtender.init()
public stateLoaded!: Callback<[]>;
| // @ts-expect-error | ||
| const baseResult = super._refreshDataSource.apply(this, arguments as any) || new Deferred().resolve().promise(); | ||
| protected _refreshDataSource(): DeferredObj<unknown> { | ||
| const baseResult = super._refreshDataSource() ?? Deferred().resolve(); |
There was a problem hiding this comment.
There’s an inconsistency in the return value here. In the other extender, we return a Promise, while here we return resolve. Should we return a Promise here as well?
There was a problem hiding this comment.
Before we used .promise() as default value, returned it
But had to add expect-error here due to incorrect typings in Deferred
| }; | ||
|
|
||
| export const rowsView = (Base: ModuleType<RowsView>) => class VirtualScrollingRowsViewExtender extends Base { | ||
| protected _dataController!: DataController & Partial<StateStoringDataControllerExtension>; |
There was a problem hiding this comment.
Why is _dataController typed differently?
state_storing rowsView: DataController & StateStoringDataControllerExtension
virtual_scrolling rowsView: DataController & Partial<StateStoringDataControllerExtension>
There was a problem hiding this comment.
StateStoringDataControllerExtension belongs to state_storing module. So in its RowsViewExtension corresponding callback will always present - we define them simultaneously.
VirtualScrolling is another independent module, and here we add type as Partial - in case stateStoring was not registered before virtualScrolling - we never face it, but it is more honest type
|
|
||
| export const stateStoringDataControllerExtender = ( | ||
| Base: ModuleType<DataController>, | ||
| ): ModuleType< |
There was a problem hiding this comment.
The explicit return type erases the Base type. This is important for TreeList, which extends the result:
class TreeListStateStoringDataExtender extends stateStoringModule.extenders.controllers.data(Base)
There was a problem hiding this comment.
there is no issue, TreeListStateStoringDataExtender will have same type as stateStoringModule.extenders.controllers.data - we call here extender function, with proper argument.
Removed m_prefix from TreeList module to make it transparent
171f478 to
f8a231d
Compare
f8a231d to
fc44410
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/grids/grid_core/state_storing/m_state_storing_controller.ts:51
_isLoadingis marked with definite assignment (!) butisLoading()can be called beforeinit()in some lifecycles, which would returnundefinedat runtime while being typed asboolean. Initializing it at declaration makes the runtime behavior match the type and removes reliance on init ordering.
packages/devextreme/js/__internal/grids/grid_core/state_storing/extenders/state_storing_data_controller.ts:60- The Deferred fail callback from
load()is not guaranteed to pass anErrorinstance; it may be a string/undefined depending on the underlying promise/Deferred. Usingerror ?? 'Unknown error'also changes the previous fallback semantics (''would no longer map to the default), which can result in an empty/unclear error being passed toloadErrorHandler.
.fail((error: Error) => {
this.stateLoaded.fire();
this.loadErrorHandler(error ?? 'Unknown error');
deferred.reject();
});
No description provided.