Validation: refactor and improve typing - #34649
Conversation
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving TypeScript typing and internal type-safety across the DevExtreme validation subsystem (validator, validation engine, validation summary/message/group), including aligning runtime expectations in tests.
Changes:
- Introduced/expanded internal validation types (
ValidationResultInternal,ValidationRuleInternal, adapter/editor structural types) and propagated them through validator/engine/summary/message/group. - Refactored several internal validation components to remove legacy inheritance/
@ts-expect-errorpatterns and replace iterator utilities with native constructs. - Updated a QUnit test expectation for
brokenRuleto beundefinedon valid validation results.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validator.tests.js | Updates expectation for brokenRule value in onValidated params. |
| packages/devextreme/js/__internal/ui/validation/m_default_adapter.ts | Adds typed editor/adapter structural contracts and narrows adapter behavior typings. |
| packages/devextreme/js/__internal/ui/m_validator.ts | Tightens internal validator typings and refactors validation rule/result handling. |
| packages/devextreme/js/__internal/ui/m_validation_summary.ts | Types validation summary items/results and refactors ordering and item update logic. |
| packages/devextreme/js/__internal/ui/m_validation_message.ts | Narrows/overrides several option types and strengthens internal typing of positioning/message rendering. |
| packages/devextreme/js/__internal/ui/m_validation_group.ts | Improves typing for validation group component and instance retrieval. |
| packages/devextreme/js/__internal/ui/m_validation_engine.ts | Introduces internal validation types and refactors rule validation loop and async handling. |
| packages/devextreme/js/__internal/integration/knockout/validation.ts | Aligns KO validator handling with updated validation result typing/async flow. |
Suppressed comments (1)
packages/devextreme/js/__internal/ui/m_validation_engine.ts:803
- This
return truecauses the surrounding.some()call to stop at the first failing rule, so later rules are not evaluated/added tobrokenRules. Validation results are expected to include all broken rules (see tests in validationEngine.tests.js asserting 2–3 broken rules).
if (!rule.isValid) {
return true;
}
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/ui/validation/m_default_adapter.ts:18
ValidationTargetEditorOptions.validationErroris typed asValidationRuleInternal, but editors can setvalidationErrorto an editor-specific object without atypefield (e.g. number box sets{ editorSpecific: true, message: ... }). This makes the adapter/validator typings overly strict and inconsistent with actual runtime values. Consider introducing a dedicated internal type for validation errors (e.g.EditorValidationErrorInternal) and using it forvalidationError(s)/brokenRule(s)across the adapter, validator, and validation engine types.
export interface ValidationTargetEditorOptions {
value?: unknown;
validationError?: ValidationRuleInternal | null;
disabled?: boolean;
rtlEnabled?: boolean;
}
packages/devextreme/js/__internal/ui/m_validation_engine.ts:85
ValidationResultInternal.brokenRule/brokenRulesare typed asValidationRuleInternal, but validator code can populate them from editor-specificvalidationErrorobjects that don't have atype(e.g.{ editorSpecific: true, message: ... }). This makes result typing misleading and can lead to incorrect assumptions about the shape ofbrokenRulesin downstream code. Consider widening the broken-rule type (e.g. a union ofValidationRuleInternaland anEditorValidationErrorInternalshape that includesmessage,editorSpecific,validator,index).
export interface ValidationResultInternal {
name?: string;
value?: unknown;
brokenRule?: ValidationRuleInternal | null;
brokenRules?: ValidationRuleInternal[] | null;
isValid?: boolean;
packages/devextreme/js/__internal/ui/m_validation_message.ts:34
validationErrorscan include editor-specific error objects (and is also typed asRecord<string, unknown>[]in existing wrappers), soValidationRuleInternal[]is too narrow here. Widen the element type to avoid incorrectly implying every error has a validation ruletype.
validationErrors?: ValidationRuleInternal[] | null;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (13)
packages/devextreme/js/ui/validation_engine.js:1
- These public
js/ui/*.jsentry points consistently import implementation via relative../__internal/...paths (e.g.packages/devextreme/js/ui/button.js:1). Introducing the@ts/*alias here is inconsistent with the rest of the folder and may rely on alias resolution for plain.jssources. Consider switching back to a relative../__internal/...import.
import ValidationEngine from '@ts/ui/validation_engine';
packages/devextreme/js/ui/validator.js:1
- These public
js/ui/*.jsentry points consistently import implementation via relative../__internal/...paths (e.g.packages/devextreme/js/ui/button.js:1). Introducing the@ts/*alias here is inconsistent with the rest of the folder and may rely on alias resolution for plain.jssources. Consider switching back to a relative../__internal/...import.
import Validator from '@ts/ui/validator';
packages/devextreme/js/ui/validation_group.js:1
- These public
js/ui/*.jsentry points consistently import implementation via relative../__internal/...paths (e.g.packages/devextreme/js/ui/button.js:1). Introducing the@ts/*alias here is inconsistent with the rest of the folder and may rely on alias resolution for plain.jssources. Consider switching back to a relative../__internal/...import.
import ValidationGroup from '@ts/ui/validation_group';
packages/devextreme/js/ui/validation_message.js:1
- These public
js/ui/*.jsentry points consistently import implementation via relative../__internal/...paths (e.g.packages/devextreme/js/ui/button.js:1). Introducing the@ts/*alias here is inconsistent with the rest of the folder and may rely on alias resolution for plain.jssources. Consider switching back to a relative../__internal/...import.
import ValidationMessage from '@ts/ui/validation_message';
packages/devextreme/testing/tests/DevExpress.ui.widgets/button.tests.js:5
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validator.tests.js:4
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validator.editors.tests.js:4
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/js/ui/validation_summary.js:1
- These public
js/ui/*.jsentry points consistently import implementation via relative../__internal/...paths (e.g.packages/devextreme/js/ui/button.js:1). Introducing the@ts/*alias here is inconsistent with the rest of the folder and may rely on alias resolution for plain.jssources. Consider switching back to a relative../__internal/...import.
import ValidationSummary from '@ts/ui/validation_summary';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validationSummary.tests.js:3
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validationSummary.markup.tests.js:3
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validationGroup.tests.js:5
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/validationGroup.markup.tests.js:3
- Most QUnit tests import internal implementation modules via the
__internal/*alias (e.g.packages/devextreme/testing/tests/DevExpress.ui.widgets.treeList/treeList.tests.js:4). Using@ts/*in plain.jstests is inconsistent with that pattern; consider switching to the equivalent__internal/...import for the new adapter module.
import DefaultAdapter from '@ts/ui/validation/default_adapter';
packages/devextreme/js/__internal/ui/validation/default_adapter.ts:24
ValidationTargetEditoris documented as either an Editor or an R1 wrapper (which uses the standardoption(name)getter, e.g.packages/devextreme/js/__internal/ui/check_box/editor_base/wrapper.ts:29). TheValidationTargetEditorOptionoverloads currently omit the getter signature, making the structural type incomplete.
interface ValidationTargetEditorOption {
(): ValidationTargetEditorOptions;
(name: string, value: unknown): void;
(options: Record<string, unknown>): void;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.
Suppressed comments (2)
packages/devextreme/js/__internal/ui/validation_group.ts:9
- This file imports ValidationEngine/Validator/ValidationSummary via relative paths, while the rest of the refactor consistently uses the @ts/ui/* aliases (e.g. validator.ts, validation_summary.ts). Mixing import specifiers for a stateful singleton like ValidationEngine risks creating multiple module instances (separate
groupsstate) depending on bundler/module resolution, and is also inconsistent with nearby code.
packages/devextreme/js/__internal/ui/validation_engine.ts:35 - The named import list from
@js/core/utils/typehasisNumeric, isObjecton the same line, which is inconsistent with the surrounding one-per-line formatting in this import block and makes future diffs noisier.
isDefined,
isFunction,
isNumeric, isObject,
isPromise,
No description provided.