Skip to content

fix: Node.js v25 / Windows compatibility and locale-aware number formatting#407

Merged
fank merged 3 commits into
OCAP2:mainfrom
smitt14ua:nodejs-25
May 1, 2026
Merged

fix: Node.js v25 / Windows compatibility and locale-aware number formatting#407
fank merged 3 commits into
OCAP2:mainfrom
smitt14ua:nodejs-25

Conversation

@smitt14ua
Copy link
Copy Markdown
Contributor

@smitt14ua smitt14ua commented Apr 30, 2026

Summary

Fixes three test failures that appear when running npm test locally on Windows with Node.js v25 (CI uses Ubuntu + Node.js v24 where none of these manifest):

  • localStorage.getItem is not a function — Node.js v22.12+ exposes a native localStorage global (SQLite-backed in v25).
    Without --localstorage-file the object exists but its methods are not functions, silently overriding jsdom's storage mock.
    testSetup.ts now detects this and replaces the broken global with a plain in-memory Storage implementation.
  • TypeError: … Received 'file:///@solid-refresh'vite-plugin-solid registers /@solid-refresh as a virtual module ID; vitest 4 converts it to file:///@solid-refresh. On Windows, fileURLToPath rejects that URL because the path has no drive
    letter. A new enforce: "pre" plugin in vitest.config.ts intercepts the ID and returns the real file path before solidPlugin can register the virtual one. Also removes the deprecated poolOptions block (removed in vitest 4).
  • Wrong thousands separator in testsMapDetail.tsx called toLocaleString() without a locale, so on a non-English Windows machine 30720 rendered as "30 720" instead of "30,720". Now passes locale() from useI18n() explicitly.

How to test

  cd ui && npm test

Optionally verify on Windows with Node.js v25 — previously produced 40 failed test files and 9 failed tests.

Checklist

  • go test ./... passes
  • cd ui && npm test passes (if frontend changed)
  • No breaking changes (or documented below)

- testSetup.ts: polyfill localStorage when Node.js v22.12+ native global
  exposes it without a backing file (getItem/setItem not functions)
- vitest.config.ts: add enforce:pre plugin to redirect /@solid-refresh to
  the real file path, avoiding fileURLToPath failure on Windows where the
  virtual module URL has no drive letter; remove deprecated poolOptions
- MapDetail.tsx: pass i18n locale to toLocaleString() so thousands separator
  follows the active locale instead of the OS locale

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces localization for world size formatting in the Map Detail view and addresses environment-specific issues in the test suite. Key changes include a localStorage polyfill for Node.js environments where the native global is incomplete and a Vitest configuration fix for Windows-specific path resolution of @solid-refresh. Feedback was provided to improve the localStorage polyfill by using a null-prototype object for the internal store to prevent collisions with inherited properties.

Comment thread ui/src/testSetup.ts Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Copy link
Copy Markdown
Member

@fank fank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the diff and ran npm test locally — 1498/1498 pass. All three fixes look correct:

  • MapDetail.tsx: useI18n() returns locale as an Accessor signal (ui/src/i18n/i18n.ts:34), so locale() is the right call.
  • testSetup.ts: the getItem !== "function" probe correctly identifies the broken Node 25 native localStorage and swaps in an in-memory Storage.
  • vitest.config.ts: enforce: "pre" ensures the redirect runs before vite-plugin-solid's resolveId claims the virtual ID. require.resolve("solid-refresh/dist/solid-refresh.mjs") works because solid-refresh exports ./dist/*. Minor nit — require.resolve("solid-refresh") would resolve via the import condition and be a bit less fragile to file renames, but the explicit path is fine.

Same Windows locale bug elsewhere

The thousands-separator fix in MapDetail.tsx patches one site, but the identical pattern exists in ui/src/pages/recording-selector/DetailSidebar.tsx at lines 128, 132, 136, 140, 168, 176 — six bare toLocaleString() calls (units, alive, dead, kills, playerKills). On a German Windows host these will render "30 720" and would break the same kind of test assertion this PR is fixing. Worth including here for consistency, or filing a follow-up.

@smitt14ua
Copy link
Copy Markdown
Contributor Author

I think I'll make same changes for these components as well, so I'm moving this PR into draft

@smitt14ua smitt14ua marked this pull request as draft April 30, 2026 12:44
@smitt14ua smitt14ua changed the title fix: Node.js v25 / Windows compatibility in ui tests fix: Node.js v25 / Windows compatibility and locale-aware number formatting Apr 30, 2026
Pass locale() from useI18n() to all toLocaleString() calls (units, alive,
dead, kills, playerKills) so number formatting follows the active locale
instead of the OS locale.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@smitt14ua smitt14ua marked this pull request as ready for review April 30, 2026 13:00
@smitt14ua smitt14ua requested a review from fank April 30, 2026 13:00
@fank fank merged commit 5cd3fe8 into OCAP2:main May 1, 2026
6 checks passed
smitt14ua added a commit to smitt14ua/OCAP2-web that referenced this pull request May 1, 2026
…atting (OCAP2#407)

* fix: Node.js v25 / Windows compatibility in ui tests

- testSetup.ts: polyfill localStorage when Node.js v22.12+ native global
  exposes it without a backing file (getItem/setItem not functions)
- vitest.config.ts: add enforce:pre plugin to redirect /@solid-refresh to
  the real file path, avoiding fileURLToPath failure on Windows where the
  virtual module URL has no drive letter; remove deprecated poolOptions
- MapDetail.tsx: pass i18n locale to toLocaleString() so thousands separator
  follows the active locale instead of the OS locale

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

* test: apply suggestion from @gemini-code-assist[bot]

Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>

* fix: use i18n locale for toLocaleString in DetailSidebar

Pass locale() from useI18n() to all toLocaleString() calls (units, alive,
dead, kills, playerKills) so number formatting follows the active locale
instead of the OS locale.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@smitt14ua smitt14ua deleted the nodejs-25 branch May 1, 2026 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants