TreeList - DataSourceAdapter - Extract branch loading util - #35136
TreeList - DataSourceAdapter - Extract branch loading util#35136Tucchhaa wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The refactor preserves the existing behavior, adds targeted Jest coverage for the extracted logic (including the race-condition scenario), and the typing changes are consistent with current runtime usage.
Pull request overview
Refactors TreeList’s data source adapter to extract the “load missing parent/child branches” logic into a dedicated utility, while tightening internal typing around RawItemData for nodes and cached store data.
Changes:
- Extracted branch loading logic into
utils/load_branches.tsand reused it fromDataSourceAdapterTreeListto simplify the adapter implementation. - Extracted
createIdFilterhelper intoutils/create_id_filter.tsand reused it for request filter construction. - Strengthened internal types (
TreeNode.data,convertItemToNodeinput,_cachedStoreData/setCachedStoreData) and added Jest coverage for branch-loading behavior and race-condition protection.
File summaries
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/utils/nodes.ts | Tightens convertItemToNode input type to RawItemData for safer internal node construction. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/utils/load_branches.ts | New extracted utility implementing parent/child branch loading with operation-staleness guarding and cache updates. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/utils/create_id_filter.ts | New shared helper to build an or-combined ID filter for store loading. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/utils/tests/load_branches.test.ts | Adds focused Jest coverage for parent/child loading, caching behavior, long-filter fallback, and failure paths. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/types.ts | Narrows TreeNode.data type to RawItemData. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts | Replaces in-class branch loading with loadBranches, and reuses createIdFilter; adds a TS suppression for an array meta-property. |
| packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.test.ts | Updates the existing race-condition test to target the extracted loadBranches path. |
| packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts | Tightens _cachedStoreData and setCachedStoreData typing and updates a concat site with an explicit cast. |
Review details
- Files reviewed: 8/8 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.
| keyExpr: this.getKeyExpr(), | ||
| _parentIdGetter: this._parentIdGetter, | ||
| _keyGetter: this._keyGetter, | ||
| isRowExpanded: (key) => this.isRowExpanded(key), |
There was a problem hiding this comment.
nitpick: lets use same approach for isRowExpanded, setCachedData, getNodeByKey - arrow function or bind
|
|
||
| const concatLoadedData = (loadedData: RawItemData[]): RawItemData[] => { | ||
| if (isRemoteFiltering) { | ||
| const cachedData = context.getCachedData() as RawItemData[]; |
There was a problem hiding this comment.
we defined it as possibly undefined, lets handle this case without casting and possible issues
| const cachedData = context.getCachedData() as RawItemData[]; | |
| const cachedData = context.getCachedData() ?? []; |
|
|
||
| private getLoadBranchesContext(): LoadBranchesContext { | ||
| return { | ||
| dataSource: this._dataSource, |
There was a problem hiding this comment.
dataSource and customLoader are grabbed once per load. If the data source is swapped mid-load, the in-flight recursion keeps loading from the dead store. It only needs dataSource for one cancel() call — pass cancel: (id) => this.cancel(id)
| }); | ||
|
|
||
| test('T1311885 - _loadParentsOrChildren should NOT throw concat error when _cachedStoreData is cleared', async () => { | ||
| test('T1311885 - loading branches should NOT throw concat error when _cachedStoreData is cleared', async () => { |
There was a problem hiding this comment.
from agent: The T1311885 test no longer touches the adapter at all — it reaches in for the context and calls the free function. The new unit test already asserts the same thing
| return; | ||
| } | ||
|
|
||
| loadParentsOrChildren(context, parentsData, options, true) |
There was a problem hiding this comment.
A failed children load now rejects. On main it silently hung forever, because the old .fail(d.reject) was wired to the parents call only. Lets add a test.
No description provided.