GridCore - DataController - Add types (Part 3)

[Tucchhaa]

No description provided.

[Copilot]

Pull request overview

This PR continues the GridCore typing effort by introducing/propagating stronger TypeScript types across the grid data pipeline (DataController, ColumnsController, DataSourceAdapter) and aligning handler naming for customizeStoreLoadOptions.

Changes:

• Introduces shared LoadOperation / StoreLoadOptions type definitions for the internal DataSource layer and extends them for grid-specific needs.
• Tightens types in GridCore controllers/adapters (e.g., ColumnsChanges, HandleDataChangedEvent) and updates related event handler signatures.
• Renames the DataSourceAdapter customization hook to _customizeStoreLoadOptionsHandler and updates TreeList override accordingly.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts	Renames TreeList adapter hook to match the new base handler name.
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts	Adds grid-specific load operation types and remote operation typing helpers.
packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/m_data_source_adapter.ts	Types the customize-store-load-options pipeline and renames the handler.
packages/devextreme/js/__internal/grids/grid_core/data_controller/types.ts	Renames the data-changed event type to HandleDataChangedEvent and updates unions.
packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts	Applies new types to DataController handlers and improves some guard logic.
packages/devextreme/js/__internal/grids/grid_core/columns_controller/types.ts	Introduces ColumnsChanges shape for columns change notifications.
packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller.ts	Types columnsChanged and _columnChanges with ColumnsChanges.
packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller_utils.ts	Types updateColumnChanges inputs and normalizes option-name typing.
packages/devextreme/js/__internal/grids/grid_core/ai_column/controllers/m_ai_column_controller.ts	Updates AI column controller handler types for renamed data-changed event shape.
packages/devextreme/js/__internal/data/data_source/types.ts	Adds internal DataSource load operation/store load option type definitions.
packages/devextreme/js/__internal/data/data_source/m_data_source.ts	Adds the return type for _createLoadOperation and imports the new type.

[Tucchhaa]

couldn't remove this proxy property, because _customizeStoreLoadOptionsHandler is extended by treelist, so it has to be a method and not an arrow func

[Copilot]

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/devextreme/js/__internal/grids/tree_list/data_source_adapter/m_data_source_adapter.ts:340

• The DataSource customizeStoreLoadOptions event fires with a single argument (the load operation), so calling the base handler via apply(this, arguments as any) is unnecessary and forces an any cast. Calling the base handler directly keeps the override typed/cleaner and avoids relying on arguments.

    super._customizeStoreLoadOptionsHandler.apply(this, arguments as any);

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:31

• RemoteOperationsOptions is introduced but LoadOperation.remoteOperations is still typed as the wider RemoteOperations union (which can include boolean/Mode). Since the adapter logic uses .filtering/.sorting/... on this value, typing it as RemoteOperationsOptions makes the contract clearer and avoids propagating an unusably-wide type.

  remoteOperations?: RemoteOperations;

packages/devextreme/js/__internal/grids/grid_core/columns_controller/m_columns_controller_utils.ts:617

• ColumnsChanges['changeTypes'] includes payload-like keys (event, virtualColumnsScrolling) that are not boolean flags, but updateColumnChanges treats every changeType as a boolean toggle (changeTypes[changeType] = true). Narrow the changeType parameter to only the boolean flag keys to prevent accidentally overwriting payload values in the future.

  changeType: Exclude<keyof ColumnsChanges['changeTypes'], 'length'>,

[Copilot]

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/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512

• The cleanup in the finally block unsubscribes dataSource._dataLoadingHandler, but this property is not defined anywhere (only _customizeStoreLoadOptionsHandlerProxy exists). Passing undefined to .off(...) can remove all handlers for the event and makes the test cleanup unreliable; it should remove the proxy handler that was attached for the test and restore the original handler reference.

            dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
            dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
        }

[anna-shakhova]

Please, check, after moving types RemoteOperations, RemoteOperationsOptions should we also move to data_source_adapter utilities isCustomStore, isLocalStore, normalizeRemoteOperations or their proper place inside data_controller

[Tucchhaa]

Looks like a valid improvement to me, I have moved them in the separate commit please check it

[anna-shakhova]

This type has grown quite complicated
A bit confusing that it is not discriminated union - changeType may be optional, also for 'refresh' it has 3 different signatures.
Please, review, whether some of options can be combined and optional marks

[Tucchhaa]

Yep, it's quite complicated, but it reflects the real usage of of calling dataController.updateItems.

changeType=='refresh' appears across different signature, because it is a default value in case changeType is not set:

DevExtreme/packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts

Line 1117 in 44ef744

change.changeType ??= 'refresh';

I think it's hard to make this type much simpler at the current stage. But I can try to make changeType be not optional

[Tucchhaa]

So, I have made changeType to be always required + removed HandleDataChangedEvent type and added ChangedEvent to dataSource types instead

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Suppressed comments (3)

packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512

• The test re-subscribes customizeStoreLoadOptions with dataSource._customizeStoreLoadOptionsHandlerProxy, but the finally block unsubscribes dataSource._dataLoadingHandler (which does not exist anywhere else in the codebase). This leaves the test-installed handler subscribed and can cause later tests to observe the modified handler.

            dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
            dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
        }

packages/devextreme/js/__internal/grids/grid_core/virtual_scrolling/m_virtual_scrolling.ts:240

• Leftover marker comment // MYTODO should be removed (it can fail text-lint rules and doesn’t provide actionable context).

    // MYTODO
    const callBase = super._dataChangedHandler.bind(this);

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53

• ChangedEvent.changeType is typed only as 'loadError', but grid internals (e.g. legacy virtual scrolling) emit and consume other values like 'append', 'prepend', and 'pageIndex', and also rely on flags like isDelayed and payload like items. The current type/comment is incomplete and will keep forcing ts-expect-error or unsafe casts.

export interface ChangedEvent extends BaseChangedEvent {
  // When  virtual scrolling with scrolling.legacyMode, changeType
  // also can be 'append', 'prepend', 'pageIndex' in case of
  changeType?: 'loadError';
  error?: unknown;
}

[Copilot]

Pull request overview

Copilot reviewed 21 out of 24 changed files in this pull request and generated no new comments.

Suppressed comments (2)

packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512

• The finally cleanup detaches dataSource._dataLoadingHandler, but this property isn’t defined on the adapter (and isn’t the handler you attached). This leaves the temporary customizeStoreLoadOptions handler subscribed and then re-subscribes the original handler, resulting in two active handlers.

            dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
            dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
        }

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:52

• ChangedEvent.changeType is documented here as also being 'append' | 'prepend' | 'pageIndex' (and grid_core/virtual_data_loader/m_virtual_data_loader.ts actually fires these values), but the type only allows 'loadError'. This makes the type misleading and forces downstream code to cast/ignore type checks in legacy virtual scrolling paths.

  // When  virtual scrolling with scrolling.legacyMode, changeType
  // also can be 'append', 'prepend', 'pageIndex' in case of
  changeType?: 'loadError';
  error?: unknown;

[Copilot]

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (4)

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/utils/tests/remoteOperations.test.ts:35

• The test expects normalizeRemoteOperations(false, …) to return false, but the implementation normalizes falsy values (including false) to an empty options object ({}). As written, this test will fail and also conflicts with the function's RemoteOperationsOptions return type.
  packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512
• The cleanup unsubscribes using dataSource._dataLoadingHandler, but that property is not defined anywhere in the codebase. With DevExtreme's EventsStrategy, passing an undefined handler empties all callbacks for the event, which is brittle and obscures intent. Unsubscribe the wrapper handler explicitly and restore the original proxy reference.

        } finally {
            dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
            dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
        }

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53

• ChangedEvent.changeType is typed as only 'loadError', but virtual scrolling (legacy mode) emits additional change types like 'append' and 'prepend' (see grid_core/virtual_data_loader/m_virtual_data_loader.ts). The current typing contradicts actual runtime values and the comment is incomplete.

export interface ChangedEvent extends BaseChangedEvent {
  // When  virtual scrolling with scrolling.legacyMode, changeType
  // also can be 'append', 'prepend', 'pageIndex' in case of
  changeType?: 'loadError';
  error?: unknown;

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

• This comment is inaccurate: e.changeType is also set by _handleLoadError() (to 'loadError') even when scrolling.legacyMode is disabled. This can mislead future changes around isDataLoading.

    // At this stage e.changeType can be defined only if virtual scrolling and scrolling.legacyMode is true
    const isDataLoading = !e || isDefined(e.changeType);

[Copilot]

Pull request overview

Copilot reviewed 23 out of 25 changed files in this pull request and generated no new comments.

Suppressed comments (5)

packages/devextreme/testing/tests/DevExpress.ui.widgets.dataGrid/dataSource.tests.js:7512

• The test replaces the customizeStoreLoadOptions handler with dataSource._customizeStoreLoadOptionsHandlerProxy, but the finally block unsubscribes dataSource._dataLoadingHandler (which is not defined anywhere in this file). This leaves the overridden handler subscribed and can leak state into subsequent tests.

        } finally {
            dataSource._dataSource.off('customizeStoreLoadOptions', dataSource._dataLoadingHandler);
            dataSource._dataSource.on('customizeStoreLoadOptions', originalDataLoadingHandler);
        }

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

• DataSourceAdapter.init declares dataSource? as optional, but the implementation immediately dereferences it (dataSource.store(), dataSource.isLastPage()). This can crash if init() is ever called without an argument; the signature should reflect that dataSource is required (or add a guard).

  public init(dataSource?) {
    const that = this;

    that._dataSource = dataSource;
    that._remoteOperations = normalizeRemoteOperations(
      this.option('remoteOperations'),
      dataSource.store(),
    );

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

• DataSource._fireChanged() can fire with no argument (it calls _fireChanged() after a successful load), so the changed handler proxy should accept an optional event. Keeping it required makes the handler type inaccurate and encourages unsafe assumptions about e being always defined.

  private _dataChangedHandlerProxy!: (e: ChangedEvent) => void;

  private _customizeStoreLoadOptionsHandlerProxy!: (e: LoadOperation) => void;

packages/devextreme/js/__internal/grids/grid_core/data_source_adapter/types.ts:53

• ChangedEvent.changeType is documented here as possibly including values like append/prepend/pageIndex, but the type currently only allows 'loadError'. This is inconsistent with the comment and with virtual scrolling code paths that use other changeType values.

export interface ChangedEvent extends BaseChangedEvent {
  // When  virtual scrolling with scrolling.legacyMode, changeType
  // also can be 'append', 'prepend', 'pageIndex' in case of
  changeType?: 'loadError';
  error?: unknown;
}

packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts:159

• DataSource can raise the changed event without an argument, so the proxied handler type should accept an optional event. Having it required is misleading and can hide cases where e is undefined.

  private _dataPushedHandler!: (changes: unknown) => void;

  private _dataChangedHandlerProxy!: (e: ChangedEvent) => void;