Grids: move _validatingController definition from base dataController - #34761
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the grids validating feature so _validatingController is no longer a base DataController concern, and is instead initialized only by the validating data-controller extender that actually uses it. This keeps the base controller slimmer and reduces unnecessary coupling between core data logic and the validating module.
Changes:
- Removed
_validatingControllerfield initialization fromDataControllerand moved it into the validatingDataControllerextender’sinit(). - Extracted validating module wiring and validating data-controller extender into dedicated files.
- Introduced shared validating constants and a
ValidatingController.getValidationData()accessor to avoid extender access to a private method.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/js/__internal/grids/tree_list/m_validating.ts | Switches TreeList validating module import to the new validating_module entry. |
| packages/devextreme/js/__internal/grids/grid_core/validating/validating_module.ts | New module entry point composing controllers/extenders for validating. |
| packages/devextreme/js/__internal/grids/grid_core/validating/m_validating.ts | Removes inline module/extender exports that were extracted; adds shared const import; adds getValidationData. |
| packages/devextreme/js/__internal/grids/grid_core/validating/extenders/validating_data_controller.ts | New validating data-controller extender with localized _validatingController initialization and updated access via getValidationData. |
| packages/devextreme/js/__internal/grids/grid_core/validating/const.ts | New shared constants/utilities extracted from m_validating. |
| packages/devextreme/js/__internal/grids/grid_core/data_controller/data_controller.ts | Removes validating controller field and lookup from the base data controller. |
| packages/devextreme/js/__internal/grids/data_grid/module_not_extended/validating.ts | Switches DataGrid validating module import to the new validating_module entry. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
f26a9fc to
91bbd4d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/grids/grid_core/validating/extenders/validating_data_controller.ts:58
- Calling the base implementation with explicit parameters is inconsistent with other
_isCellChangedoverrides in the codebase (they delegate viaapply(this, arguments as any)), and it can become incorrect if the base method signature gains additional parameters. For reference, seegrid_core/editing/m_editing.ts:2632andtree_list/data_controller/m_data_controller.ts:67.
return super._isCellChanged(oldRow, newRow, visibleRowIndex, columnIndex, isLiveUpdate);
packages/devextreme/js/__internal/grids/grid_core/validating/extenders/validating_data_controller.ts:33
_getValidationStatuscurrently relies on an eslint suppression (no-unsafe-return) becausevalidationStatusis inferred asany. This can be avoided by typing the parameter and casting the derived status tostring, which keeps the intent clear without disabling lint rules.
private _getValidationStatus(validationResult): string {
const validationStatus = validationResultIsValid(validationResult)
? validationResult.status
: validationResult;
// eslint-disable-next-line @typescript-eslint/no-unsafe-return
return validationStatus ?? VALIDATION_STATUS.valid;
}
| } from './m_validating'; | ||
|
|
||
| export const validatingModule = { | ||
| defaultOptions(): { editing: { texts: { validationCancelChanges: string } } } { |
There was a problem hiding this comment.
lets create type for it, we also may use public types, smth like
type ValidationOptions = { editing: { texts: Pick<EditingTextsBase, 'validationCancelChanges'> } };
| super.init(); | ||
| } | ||
|
|
||
| private _getValidationStatus(validationResult): string { |
There was a problem hiding this comment.
lets try type this method
| return validationStatus ?? VALIDATION_STATUS.valid; | ||
| } | ||
|
|
||
| protected _isCellChanged(oldRow, newRow, visibleRowIndex, columnIndex, isLiveUpdate): boolean { |
There was a problem hiding this comment.
rebase and type the method, please
|
|
||
| export const VALIDATION_CANCELLED = 'cancel'; | ||
|
|
||
| export const validationResultIsValid = (result: unknown): boolean => isDefined(result) |
There was a problem hiding this comment.
lets make it type guard
| export const validationResultIsValid = (result: unknown): boolean => isDefined(result) | |
| export const validationResultIsValid = (result: unknown): result is CellValidationResult => isDefined(result) |
| valid: 'valid', | ||
| invalid: 'invalid', | ||
| pending: 'pending', | ||
| }; |
There was a problem hiding this comment.
| }; | |
| } as const; | |
| export type ValidationStatus = typeof VALIDATION_STATUS[keyof typeof VALIDATION_STATUS]; |
| } from '../const'; | ||
|
|
||
| interface CellValidationResult { | ||
| status?: string; |
There was a problem hiding this comment.
| status?: string; | |
| status?: ValidationStatus; |
- move to upper level, where guard and ValidationStatus live
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Suppressed comments (3)
packages/devextreme/js/__internal/grids/grid_core/validating/extenders/validating_data_controller.ts:22
cellOptions.validationStatuscan be set toVALIDATION_CANCELLED(see validating controller logic), soValidatedCell.validationStatusshould include that sentinel. Otherwise the typing here is narrower than the runtime values and can hide errors when refactoring.
type ValidatedCell = Cell & {
validationStatus?: ValidationStatus;
cellElement?: Element;
};
packages/devextreme/js/__internal/grids/grid_core/validating/extenders/validating_data_controller.ts:13
ValidationResultis typed asCellValidationResult | string | undefined, butgetCellValidationResult()appears to store either a result object or theVALIDATION_CANCELLEDsentinel. Keeping this as a broadstringmakesvalidationResultIsValid()a potentially unsafe type guard (any non-"cancel" string would be treated as an object). Consider narrowing the string case totypeof VALIDATION_CANCELLED.
This issue also appears on line 19 of the same file.
type ValidationResult = CellValidationResult | string | undefined;
packages/devextreme/js/__internal/grids/grid_core/validating/const.ts:22
validationResultIsValidis declared as a type guard forCellValidationResult, but the current implementation returns true for any defined non-VALIDATION_CANCELLEDvalue (including primitives). Tightening it to require an object keeps the type guard accurate and prevents accidental mis-narrowing if other sentinel strings ever appear.
export const validationResultIsValid = (
result: unknown,
): result is CellValidationResult => isDefined(result) && result !== VALIDATION_CANCELLED;
What
Relocated the
_validatingControllerreference from the baseDataControllerto the validating data-controller extenderHow
Moved the field and its lookup into a new
init()on the extender, extracted the extender and module into their own files, narrowed the reference to a read-only port, and added a publicgetValidationDataaccessor so the extender no longer reaches a private method