Numberbox: Refactor and improve typing by Raushen · Pull Request #34779 · DevExpress/DevExtreme

Raushen · 2026-08-14T14:25:09Z

No description provided.

Copilot

Pull request overview

This PR refactors the internal NumberBox implementation by removing legacy m_* modules, introducing new shared utilities, and tightening TypeScript typings across NumberBox-related components and their consumers.

Changes:

Replaced legacy m_number_box* imports with the new number_box* module structure across internal widgets and tests.
Extracted NumberBox helper logic into new reusable modules (utils.ts, number_box.caret.ts) and removed obsolete ones.
Improved TypeScript typing for NumberBox base/mask/spin/spins implementations and updated downstream consumers (e.g., ColorBox, DateBox).

Reviewed changes

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

Show a summary per file

File	Description
packages/devextreme/testing/tests/DevExpress.ui.widgets/toolbar.kbn.tests.js	Updates NumberBox class import to new internal module path.
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/numberBoxParts/mask.caret.tests.js	Updates caret helper import to new module path.
packages/devextreme/testing/tests/DevExpress.ui.widgets.editors/numberBoxParts/common.tests.js	Updates SpinButton import to new module path.
packages/devextreme/js/ui/number_box.js	Updates public entry reexport to point at new internal implementation.
packages/devextreme/js/__internal/ui/text_box/text_editor.mask.ts	Adjusts typing to allow `char` to be `undefined` in replace logic.
packages/devextreme/js/__internal/ui/number_box/utils.ts	Adds new shared NumberBox utility helpers (separator parsing, percent adjustment, etc.).
packages/devextreme/js/__internal/ui/number_box/number_box.ts	Switches NumberBox to use the renamed mask module.
packages/devextreme/js/__internal/ui/number_box/number_box.spins.ts	Refactors spin buttons to stronger typing and cleaner option handling.
packages/devextreme/js/__internal/ui/number_box/number_box.spin.ts	Strengthens typing around spin change events and pointer handling.
packages/devextreme/js/__internal/ui/number_box/number_box.mask.ts	Major typing improvements + refactors using new caret/utils modules.
packages/devextreme/js/__internal/ui/number_box/number_box.caret.ts	Adds a new caret utility module (replacing legacy caret implementation).
packages/devextreme/js/__internal/ui/number_box/number_box.base.ts	Improves typing for NumberBox base behavior (spin events, parsing, ARIA).
packages/devextreme/js/__internal/ui/number_box/m_utils.ts	Removes legacy NumberBox utilities (replaced by `utils.ts`).
packages/devextreme/js/__internal/ui/number_box/m_number_box.caret.ts	Removes legacy caret implementation (replaced by `number_box.caret.ts`).
packages/devextreme/js/__internal/ui/date_box/time_view.ts	Updates DateBox TimeView to import the new NumberBox module/types.
packages/devextreme/js/__internal/ui/color_box/color_view.ts	Updates ColorBox to import the new NumberBox module/constants.

Suppressed comments (1)

packages/devextreme/js/__internal/ui/number_box/number_box.mask.ts:451

In _updateFormat, _format() is typed to return string | undefined, but the LDML callback passes its result directly to number.convertDigits and returns it as a string. If a custom formatter ever returns undefined, this will propagate (or become a non-string) and can break formatting/caret logic at runtime. Normalize the formatted text to a string before calling convertDigits and returning it.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

EugeniyKiyashko · 2026-08-17T12:27:30Z

-    const moveToFloat = (this._lastKey === decimalSeparator || this._lastKey === '.' || this._lastKey === ',') && isDecimalSeparatorNext;
+  _shouldMoveCaret(text: string, caret: CaretRange | undefined): boolean {
+    const decimalSeparator: string = number.getDecimalSeparator();
+    const isDecimalSeparatorNext = text.charAt(caret?.end ?? 0) === decimalSeparator;


[blocker] This ?? 0 isn't a neutral fallback, it's a semantically wrong one: 0 is a real index, so
"there is no caret" becomes "the caret is at the start", and the method can answer true and
move the caret. The original was text.charAt(caret.end), which would have thrown instead.
Honest handling:
_shouldMoveCaret(text: string, caret: CaretRange | undefined): boolean {
if (!caret) {
return false;
}
...
}
This is one of five new places where an absent caret is silently substituted with zero
(see also mask.ts, caret.ts). The root cause is that
_caret() (text_editor.mask.ts) fuses getter and setter into one method dispatching on
arguments.length, so the getter is typed CaretRange | undefined - even though undefined only
happens there when !$input.length, at which point _getInputVal() would already have thrown.
Overloads at the source:
_caret(): CaretRange | undefined;
_caret(position: CaretRange, force?: boolean): void;
or splitting into _getCaret()/_setCaret() would remove most of those five fallbacks and let
the remaining case be handled once.

EugeniyKiyashko · 2026-08-17T12:28:40Z

-  _getInputVal() {
-    return number.convertDigits(this._input().val(), true);
+  _getInputVal(): string {
+    const inputValue: string = number.convertDigits(this._input().val(), true);


[blocker] number is any here, not a typed API. Root cause: numberLocalization is built through
dependencyInjector, and function injector(object) in core/utils/m_dependency_injector.ts is
untyped and returns object, which under noImplicitAny: false collapses to any. I probed it -
convertDigits / getSign / parse are all any via both import paths, @js/common/... and
@ts/core/... alike.
So the added annotations are unchecked, and three of them are demonstrably false:
const inputValue: string = number.convertDigits(this._input().val(), true);
the real signature is (value: string | number) => string | number
const convertedText: string = number.convertDigits(text, true); // in _updateFormat
text = this._format(...), which this PR itself declares as string | undefined -
the annotation contradicts its own return type two lines up
const formattedValue: string = number.convertDigits(this._formattedValue, true); // _formatValue
_formattedValue?: string, so the input can be undefined and convertDigits returns it
unchanged, yet the result is declared string
This is worse than leaving them unannotated: the falsehood is now asserted explicitly, and
turning on no-unsafe-assignment later won't catch it.
Options: type injector generically -
function injector(object: T): T & { inject(o: object): void; resetInjection(): void }
which fixes this for the whole codebase but belongs in its own PR; or, locally, a narrow typed
wrapper such as
const toStandardDigits = (value: string): string => String(number.convertDigits(value, true));
Either way, please drop the hand-written annotations on any.

EugeniyKiyashko · 2026-08-17T12:29:54Z

  }

-  _getFormatPattern() {
+  _getFormatPattern(): Format {


This is technically not a lie - I checked, Format already includes undefined
(const x: Format = undefined compiles) - but it doesn't say what the method actually
returns: either an LDML pattern string, or the user's format passed through as-is.
The price is four identical guards at the call sites, all of the shape
isString(format) ? format : '', in _getTextSeparatorIndex, _isPercentFormat,
_isValueIncomplete, and getCaretAfterFormat in number_box.caret.ts.
Behaviour matches the old code (I verified that getRealSeparatorIndex('') and
getRealSeparatorIndex(object) both yield { occurrence: 1, index: -1 }), but it's "I don't know
what this is" written four times.
Modelling the return explicitly - e.g.
type FormatPattern = { kind: 'ldml'; pattern: string } | { kind: 'raw'; format: Format }
or at minimum returning the LDML string and handling the raw case once - collapses all four.
Side note: because Format already admits undefined, the | undefined in
_getEffectiveFormatOption() and in asFormatObject is redundant, and _currentFormat?: Format
is optional twice over.

EugeniyKiyashko · 2026-08-17T12:32:45Z

  }

-  _isInputFromPaste(e) {
+  _isInputFromPaste(e: DxEvent<InputEvent>): boolean | undefined {


boolean | undefined documents the tri-state instead of removing it: _isValuePasted is never
initialized. The only caller does if (isFromPaste), so returning boolean with ?? false
is both shorter and honest.

EugeniyKiyashko · 2026-08-17T12:33:11Z


 const FORCE_VALUECHANGE_EVENT_NAMESPACE = 'NumberBoxForceValueChange';

+export type NumberBoxValue = number | null | undefined;


[important] This widens rather than sharpens. _parseValue and _normalizeInputValue previously returned
number | null and cannot return undefined, so every caller now has an unreachable case to
consider.
Two knock-on effects:

_parseValue(value?: string | NumberBoxValue) - a ? on top of a union that already
contains undefined

_parsedValue?: number | null in number_box.mask.ts doesn't use the alias, so the codebase
now has two spellings for the same concept
Suggest type NumberBoxValue = number | null, and express optionality with ? / | undefined
only where it genuinely exists.

EugeniyKiyashko · 2026-08-17T12:35:52Z

+}
+
+export default class SpinButtons extends TextEditorButton<NumberBoxBase> {
+  declare instance?: dxElementWrapper | null;


This is effectively an @ts-expect-error, just an invisible one. The base declares
instance?: dxElementWrapper | Button | null, and the base render() assigns the result of
_create() (typed Button | dxElementWrapper) into it - so narrowing it in the subclass
provides no guarantee and has no runtime effect.
Better: make TextEditorButton generic over its instance type
(TextEditorButton<TComponent, TInstance>), so SpinButtons states this once and
_create()/render() stay consistent.

EugeniyKiyashko · 2026-08-17T12:36:15Z

-  _legacyRender($editor, isTouchFriendly, isVisible) {
-    $editor.toggleClass(SPIN_TOUCH_FRIENDLY_CLASS, isTouchFriendly);
-    $editor.toggleClass(SPIN_CLASS, isVisible);
+  _legacyRender(


All three parameters were made optional, even though both call sites always pass all three.
The optionality exists purely because _create() passes editor?.$element().
Consequences: $editor?.toggleClass(...) now silently no-ops instead of failing, and
toggleClass(cls, undefined) toggles the class rather than setting it.
_attachEvents already does if (!editor) return; - doing the same in _create() lets the
parameters stay required:
_legacyRender($editor: dxElementWrapper, isTouchFriendly: boolean, isVisible: boolean): void

EugeniyKiyashko · 2026-08-17T12:36:57Z

-      const spinUp = SpinButton.getInstance($spinButtons.eq(0));
-      const spinDown = SpinButton.getInstance($spinButtons.eq(1));
+      const spinUp = SpinButton.getInstance<SpinButton>($spinButtons.eq(0));
+      const spinDown = SpinButton.getInstance<SpinButton>($spinButtons.eq(1));


static getInstance<T = any>(element): T in dom_component.ts - the type argument asserts the
result isn't undefined, but getInstanceByElement can return undefined and the following
.option() would throw. Here and in the three sibling calls an @ts-expect-error was replaced by
an explicit type argument, i.e. a visible escape hatch traded for an invisible one.
Strictly speaking this is no worse than the original, but it isn't a typing improvement either.
The real fix - getInstance(): T | undefined - is probably its own PR; for this one it's enough
not to count it as resolved.

EugeniyKiyashko · 2026-08-17T12:37:04Z

-      const isVisible = this._isVisible();
-      const isTouchFriendly = this._isTouchFriendly();
-      // @ts-expect-error
+    if (shouldUpdate && instance) {


&& instance is redundant: the base update() in texteditor_button_collection/button.ts ends
with return !!this.instance, so shouldUpdate already implies it. It reads as a semantic guard
but is really a type narrowing.

EugeniyKiyashko · 2026-08-17T12:37:31Z

+  end: number;
+}
+
+export type CaretPosition = number | CaretRange | undefined;


Admitting undefined here changed getCaretWithOffset's behaviour: (range?.start ?? 0) + offset
now silently returns { start: offset, end: offset } for a missing caret, where the original
(caret.start === undefined check on a required argument) would have thrown.
That's the same pattern as the ?? 0 fallbacks in number_box.mask.ts - see the
_shouldMoveCaret comment. If _caret() gets proper getter/setter overloads, this type can go
back to number | CaretRange and the ?./?? 0 here disappears.

Rename files: remove m_ prefix

8ec293c

Raushen self-assigned this Aug 14, 2026

Copilot AI lite review requested due to automatic review settings August 14, 2026 14:25

Raushen requested a review from a team as a code owner August 14, 2026 14:25

Raushen added the 26_2 label Aug 14, 2026

Copilot started reviewing on behalf of Raushen August 14, 2026 14:25 View session

Copilot AI reviewed Aug 14, 2026

View reviewed changes

NumberBox refactoring

31d3fa9

Raushen force-pushed the NumberBox-refactoring branch from 67b2041 to 31d3fa9 Compare August 17, 2026 08:42

Copilot AI review requested due to automatic review settings August 17, 2026 08:42

Copilot AI reviewed Aug 17, 2026

EugeniyKiyashko requested changes Aug 17, 2026

View reviewed changes

Navigation Menu

Uh oh!

Numberbox: Refactor and improve typing - #34779

Numberbox: Refactor and improve typing#34779
Raushen wants to merge 2 commits into
DevExpress:mainfrom
Raushen:NumberBox-refactoring

Raushen commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

EugeniyKiyashko Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		const FORCE_VALUECHANGE_EVENT_NAMESPACE = 'NumberBoxForceValueChange';

		export type NumberBoxValue = number \| null \| undefined;

Uh oh!

Conversation

Raushen commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants