Conversation
Rename js/__internal/core/utils/m_version.ts and m_browser.ts, updating the two public re-export shims and the six jQuery/Knockout integration imports. No content change - the rename is kept as its own commit so git and GitHub preserve the file history. The renamed files now fall under the strict eslint ruleset, so this commit alone does not lint clean; the follow-up commit fixes that. Committed with --no-verify for that reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new compare implementation changes maxLevel handling in a backwards-incompatible way for numeric-string inputs, and it’s re-exported through a public (even if deprecated) module.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This PR updates the internal “browser” and “version” utility modules by removing the legacy m_-prefixed filenames and introducing stricter TypeScript typing, while keeping the deprecated public entry points (core/utils/*) pointing to the updated internals.
Changes:
- Replaced
m_version/m_browsermodules with new typed implementations (version.ts,browser.ts) and updated deprecated reexports. - Updated internal integration code (jQuery/Knockout) to import the new
@ts/core/utils/versionmodule. - Adjusted widget iOS version comparison to satisfy the stricter
comparetyping.
| File | Description |
|---|---|
| packages/devextreme/js/core/utils/version.js | Deprecated reexport now points to internal version module. |
| packages/devextreme/js/core/utils/browser.js | Deprecated reexport now points to internal browser module. |
| packages/devextreme/js/__internal/integration/knockout/clean_node.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/integration/knockout/clean_node_old.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/integration/knockout.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/integration/jquery/hooks.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/integration/jquery/deferred.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/integration/jquery.ts | Updated version-compare import to new module path. |
| packages/devextreme/js/__internal/core/widget/widget.ts | Adjusted call to compare to handle optional version typing. |
| packages/devextreme/js/__internal/core/utils/version.ts | New typed compare implementation replacing m_version.ts. |
| packages/devextreme/js/__internal/core/utils/m_version.ts | Removed legacy implementation. |
| packages/devextreme/js/__internal/core/utils/browser.ts | New typed browser implementation replacing m_browser.ts. |
| packages/devextreme/js/__internal/core/utils/m_browser.ts | Removed legacy implementation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
version.ts
Introduce the Version type (string | number | (string | number)[]) that
the QUnit suite and the six call sites already relied on, and move the
argument normalization out of compare() into a module-level toParts()
that returns number[].
parseInt(x[i] || 0, 10) split into two distinct cases: inside toParts
the `|| 0` is kept, because '1.'.split('.') yields an empty string that
has to read as 0 and `??` would not catch it; the out-of-range lookup
became xParts[i] ?? 0, where the only missing value is undefined.
maxLevel got an explicit undefined check so the typed signature holds,
but the finite test stays on the global isFinite. compare is
re-exported from the deprecated yet still public core/utils/version
module, where untyped JS callers may pass maxLevel as a numeric string;
Number.isFinite would reject those and silently stop capping the
comparison depth.
browser.ts
Type the detection result with BrowserName / BrowserInfo / Browser,
mirroring the shape of the public js/core/utils/browser.d.ts, which is
deliberately left untouched.
The `exec() || cond && exec() || []` chain became a `??` chain, and
`browserVersion && browserVersion[1]` became `exec(...)?.[1]`. Typing
`ua` as string also made prefer-includes fire, so indexOf(x) >= 0 is
now includes(x).
extend() is still used to build the singleton, so the undefined-valued
keys it skips keep being skipped; it is typed in its own block later.
widget.ts
devices.real().version is number[] | undefined, which the newly typed
compare() rejects. Pass `version ?? []`. The rule body only runs on
iOS, where version is always an array, so runtime behaviour is
unchanged - previously an undefined would have thrown on x.length.
Verified: eslint clean on all three files, build:ts:internal green, and
the QUnit suites utils.version.tests.js (10/10) and
utils.browser.tests.js (27/27) pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e91747d to
823e4c9
Compare
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The migration cleanly updates all verified imports away from removed m_ modules and the new typed implementations preserve prior behavior while improving safety.
Review effort: Lite
Findings: None
Resolved since last review (1)
dmlvr
changed the title
| if (typeof value === 'number') { | ||
| return [value]; | ||
| } |
There was a problem hiding this comment.
This early return skips the parseInt below, which the old code always applied: it ran parseInt(x[i] || 0, 10) on every element, including the single element a number argument was turned into. A number now keeps its exact value instead of being truncated.
I ran 1782 combinations of arguments through both versions and 182 return a different number. 172 of those involve NaN, Infinity or a number large enough to stringify in exponential form, but 10 involve an ordinary decimal:
compare(13.3, [13.3]) before 0 now 1
compare(13, 13.3) before 0 now -1
compare(2.5, '2.0.1') before -1 now 1
compare([2, 0], 2.5) before 0 now -1
The first one is the clearest: the same version written two different ways no longer compares as equal.
None of the six call sites inside the library passes a number - they all pass a string or an array - and the only number in the QUnit suite is the integer 9, which is unaffected either way. That is why the tests stay green. But compare is exported from the public core/utils/version, so it is reachable.
Letting the number go through the same map restores the old behaviour:
function toParts(value: Version): number[] {
const parts = typeof value === 'number'
? [value]
: (typeof value === 'string' ? value.split('.') : value);
return parts.map((part) => parseInt(String(part || 0), 10));
}I ran the same 1782 combinations against that version: 0 differences from the old code. A test that passes a decimal number would lock it in - right now nothing covers the number path beyond an integer.
| @@ -0,0 +1,36 @@ | |||
| export type Version = string | number | (string | number)[]; | |||
There was a problem hiding this comment.
There is already an exported type called Version in this package: js/__internal/utils/version.ts exports interface Version { major: number; minor: number; patch: number }, and the license code imports it.
Two exported types with the same name and incompatible shapes in one package means an auto-import can silently pick the wrong one, and the error that follows will point somewhere else entirely. A name like ComparableVersion or VersionInput avoids it, and renaming now is cheaper than after the type spreads to call sites.
|
|
||
| export type BrowserName = 'webkit' | 'chrome' | 'mozilla' | 'safari' | 'unknown'; | ||
|
|
||
| export type BrowserInfo = Partial<Record<BrowserName, boolean>> & { |
There was a problem hiding this comment.
This is a second, hand-maintained copy of the BrowserInfo that js/core/utils/browser.d.ts already declares - the same five flags and the same optional version. Two copies of one shape drift apart the first time either side gains a browser.
import type { BrowserInfo } from '@js/core/utils/browser'; would give one source of truth. It is a type-only import, so it is erased at compile time and does not create a runtime cycle with the shim.
| browserVersion = /(?:chrome|crios)\/(\d+\.\d+)/.exec(ua)?.[1]; | ||
| } else if (ua.includes('fxios')) { | ||
| browserName = 'mozilla'; | ||
| browserVersion = /fxios\/(\d+\.\d+)/.exec(ua)?.[1]; | ||
| } else if (ua.includes('safari') && /version|phantomjs/.test(ua)) { | ||
| browserName = 'safari'; | ||
| browserVersion = /(?:version|phantomjs)\/([0-9.]+)/.exec(ua)?.[1]; | ||
| } else { | ||
| browserName = 'unknown'; | ||
| browserVersion = /applewebkit\/([0-9.]+)/.exec(ua)?.[1]; |
There was a problem hiding this comment.
Worth recording explicitly, because this is the one behaviour difference I found in this module. When these inner regexes do not match, browserVersion is now undefined where it used to be null - x && x[1] returned null, ?.[1] returns undefined. Two consequences:
_fromUA(ua).versionisundefinedinstead ofnull;- on the exported singleton the key disappears altogether, because
extendskips undefined values (m_extend.ts:54).
I ran 43 user agents through both versions. All 34 real-world ones are identical - this only shows up on malformed input, for example a user agent that contains "Chrome" with no version number after it. No consumer is sensitive to it either: browser.version >= 91, parseFloat(browser.version ?? '') and Number(browser.version ?? 0) behave the same for null and undefined, and nothing in the repo compares the field to null.
So I would call this a fix rather than a problem - browser.d.ts declares version?: string, so null was never a value the type allowed. It is simply not covered: no test feeds a user agent where the version regex misses.
No description provided.