Skip to content

fix: prototype-safe locale reads and loader error handling - #19

Draft
jarda-svoboda wants to merge 5 commits into
masterfrom
fix/locale-key-safety
Draft

fix: prototype-safe locale reads and loader error handling#19
jarda-svoboda wants to merge 5 commits into
masterfrom
fix/locale-key-safety

Conversation

@jarda-svoboda

@jarda-svoboda jarda-svoboda commented Jul 25, 2026

Copy link
Copy Markdown
Member

Correctness fixes split out of #18 so they can land independently of the performance work. 5 commits, each independently green.

1. fix: guard accumulator reads indexed by user-supplied locales

loadedKeys and the reduce accumulators in getTranslationProps and addTranslations were read via bare bracket access keyed by a user-supplied locale, so a locale named like an Object.prototype member (__proto__) resolved the inherited member instead of an own entry.

  • loadedKeys (functional)this.loadedKeys[locale] = … on a plain object goes through the __proto__ setter, so no own key is ever recorded: the load-once cache silently never fills and the loader refetches on every route change. Fixed with a null-prototype cache.
  • The keys reduce (functional)[...(acc[locale] || [])] spreads Object.prototype into an array, throwing TypeError: … is not iterable and aborting the load.
  • Object.keys(translations[$locale]) (functional) — a null payload for a locale crashed addTranslations, although every other step tolerates it. Now fails soft.

serialize is left exactly as it is on master. A rewrite to a Map accumulator was tried and reverted: it fixed nothing (spreading Object.prototype yields {} either way) and introduced a defect of its own — Map keys are SameValueZero, so a loader with a missing locale and one whose locale is the literal string 'undefined' collided on the object rebuild and one payload was dropped.

Why the suite never caught the real bugs: ts-jest inherited the root target es2017, where tsc downlevels object spread to an Object.assign helper whose [[Set]] semantics silently drop own __proto__ keys. esbuild emits a defineProperty-based helper and keeps them, so the shipped bundle did have the bug. The commit raises the target for the suite only (tests/tsconfig.json); the published target is untouched. The two changes are inseparable: the regression test needs the new target to be meaningful, and the old source crashes under it.

2. fix(loaders): report a failed load instead of dropping it

Promises that nobody could reach:

  • the loader's own subscriber promisesubscribe discards what loader returns, so a throw in its synchronous prologue rejected a promise with no handler. Everything that can throw now runs inside the tracked promise, and a prologue failure is filed under the sanitized locale, so loading.toPromise — which matches on the sanitized form — cannot filter the rejection out and report success for e.g. setLocale('EN');
  • the aggregate toPromise() returns to fire-and-forget setLocale/setRoute/loadTranslations callers;
  • the config loadloadConfig reports and marks its own failure, so the public fire-and-forget call is covered as well as the constructor's;
  • loadTranslations' own locale lookup, which resolves before any promise exists, so a throw there reached the caller synchronously — before they could attach the .catch the docs describe.

Around that:

  • a consumer's logger is arbitrary code, so loggerFactory contains its throws — including the method lookup, since the logger itself can be null or expose a level through a throwing accessor;
  • the promise purge waited on Promise.all, which settles at the first failure, and then cleared promises still in flight; it now waits for every tracked promise to settle and removes only those;
  • loading.toPromise() no longer assumes a config has loaded — new i18n().setRoute('/') threw a TypeError.

Failures go through one logError(message, error) helper and are not deduplicated: suppressing repeats of a shared error object (a module-level constant, a cached rejection) silenced it for the whole process — a second instance reported nothing at all. One broken load can therefore produce two reports (loader + config); that is noise rather than a defect, and collapsing it properly needs one owning reporting layer — see follow-ups.

Two changes were tried here and reverted, both because they cost more than they fixed:

  • making isLoading a count rather than a flag. It made $t withhold delivered translations, could be left permanently above zero by a throwing $loading subscriber or a loader that never settles, and changed the public meaning of $loading. Its only benefit was diagnostic accuracy of loading.get() during overlapping loads;
  • removing isLoading from the translation derived. That was only needed to undo the counter's damage; with the flag restored, the derived matches master again.

3. fix(routes): match a route pattern without mutating it

RegExp.prototype.test advances lastIndex on a g/y pattern, so routes: [/\/shop/g] matched only every other navigation and the loader silently skipped every second load. A stateful RegExp is now matched through a throwaway copy:

  • the consumer's pattern object is never written to;
  • flag semantics are preserved — y still anchors at the start;
  • a frozen pattern works, including a frozen g/y one, where any write to lastIndex throws and the surrounding catch would turn that into a permanent silent non-match;
  • duck-typed matchers keep working. The copy is gated on input instanceof RegExp: source/flags on anything else are not a pattern, and new RegExp(undefined) is /(?:)/, which matches every route. Anything else object-shaped is asked for test as before, with the route passed through unchanged, so a custom matcher can still tell an unset route from the string 'undefined'.

4. test: type-check the repo and cover both published bundles

tsup does not type-check: verified that a type error outside the entry file built clean, so a broken root config first surfaced at npm publish. pretest now runs tsc --noEmit, and because the root config compiles src only, tsconfig.tools.json covers the repo's own jest.config.js and tsup.config.js. Both gates verified to fail on an injected error.

tests/specs/dist.spec.ts asserts prototype safety of what actually ships (esbuild's spread helper, emitted once per format), covering both published entries — the CJS one in a real Node process, since jest's ESM runtime cannot require() it. The probe reports its own outcome and its reason: only ERR_REQUIRE_ESM is tolerated as a skip, so a missing, malformed or otherwise broken bundle fails. Verified all three ways. Its path resolves relative to the spec, and the child is bounded by a timeout since it runs at module evaluation where jest's per-test timeout does not apply.

5. ci: test on the current LTS Node as well as the minimum

The matrix ran only on Node 18, EOL since April 2025. Node 24 is the current LTS and the only leg where require() of an ESM dependency works, so it is what actually executes the CJS assertions — confirmed in CI, including on Windows, which is also the only place the probe's path handling is exercised.

Testing

  • npm test — 58 tests; every commit verified green independently (41 / 52 / 58 / 58 / 58)
  • npm run test:dist — 3 tests against the built ESM and CJS bundles
  • npm run build, npm run typecheck and npm run lint clean
  • ✅ Fixes verified red→green by reverting to master's actual implementation, not to an invented broken one
  • ✅ Tests wait on observable state (a helper that polls until the condition holds, with a deadline under jest's timeout) rather than fixed delays, so the 3-OS × 2-Node matrix cannot fail them on scheduling alone

Known limitations

  • One broken load can be reported twice (loader + config). Fixing it properly means one layer owning failure reporting, which belongs with the store-layer work planned for the next major.
  • logError hands the error to the configured logger, which stringifies it like any other value, so a structured transport receives a message rather than the Error. Unchanged from master; changing it means changing loggerFactory's formatting contract.

@jarda-svoboda
jarda-svoboda force-pushed the fix/locale-key-safety branch 8 times, most recently from 680c19a to 83b4d74 Compare July 26, 2026 22:30
@jarda-svoboda
jarda-svoboda marked this pull request as draft July 26, 2026 23:31
@jarda-svoboda
jarda-svoboda force-pushed the fix/locale-key-safety branch 3 times, most recently from f236673 to 0ee6479 Compare July 27, 2026 00:48
claude added 5 commits July 27, 2026 01:19
`loadedKeys` and the reduce accumulators in `getTranslationProps` and
`addTranslations` were read via bare bracket access keyed by a user-supplied
locale, so a locale named like an `Object.prototype` member resolved the
inherited member instead of an own entry.

Two of those sites were broken rather than merely fragile:

- `this.loadedKeys[locale] = …` on a plain object routes '__proto__' through
  the prototype setter, so no own key is recorded: the load-once cache never
  fills and the loader refetches on every route change. It now has a null
  prototype.
- `[...(acc[locale] || [])]` spreads `Object.prototype` into an array and
  throws, aborting the load.

`Object.keys(translations[$locale])` also crashed on a null payload for a
locale, which every other step tolerates; it now fails soft.

The suite could not see any of this: ts-jest inherited the root `es2017`
target, where tsc downlevels object spread to an `Object.assign` helper whose
[[Set]] semantics drop own '__proto__' keys, so such a locale never reached the
accumulators under test. esbuild keeps them, so the shipped bundle did have the
bug. The suite alone is moved to a newer target via `tests/tsconfig.json`; the
published build target is unchanged.
Promises that nobody could reach:

- the loader's own subscriber promise — `subscribe` discards what `loader`
  returns, so a throw in its synchronous prologue rejected a promise with no
  handler. Everything that can throw now runs inside the tracked promise, and a
  prologue failure is filed under the sanitized locale, so `loading.toPromise`
  — which matches on the sanitized form — cannot filter the rejection out and
  report success for e.g. `setLocale('EN')`;
- the aggregate `toPromise()` returns to fire-and-forget callers;
- the config load — `loadConfig` reports and marks its own failure, so the
  public fire-and-forget call is covered as well as the constructor's;
- `loadTranslations`' own locale lookup, which resolves before any promise
  exists, so a throw there reached the caller synchronously — before they could
  attach the `.catch` the docs describe.

A consumer's logger is arbitrary code, so `loggerFactory` contains its throws,
including the method lookup: the logger itself can be null or expose a level
through a throwing accessor.

The purge waited on `Promise.all`, which settles at the first failure, and then
cleared promises still in flight; it now waits for every tracked promise to
settle and removes only those. `loading.toPromise()` also no longer assumes a
config has loaded — `new i18n().setRoute('/')` threw a TypeError.

Failures are reported through one `logError(message, error)` helper and are
deliberately not deduplicated: suppressing repeats of a shared error object
silences it for the whole process.
`RegExp.prototype.test` advances `lastIndex` on a `g`/`y` pattern, so a shared
route object matched only every other navigation and the loader silently
skipped every second load. Matching through `String.prototype.search` fixes
that at the single choke point that owns route matching, leaves the consumer's
pattern object untouched, and keeps a frozen pattern working — writing
`lastIndex` to reset it would throw there, and the surrounding catch would turn
that into a permanent silent non-match.
`tsup` does not type-check, so a type error outside the entry file built clean
and only surfaced at publish time; `pretest` now runs `tsc --noEmit` against the
published `tsconfig.json`. That config compiles `src` only — `allowJs` is off —
so `tsconfig.tools.json` covers the repo's own `jest.config.js` and
`tsup.config.js`, which nothing checked before.

The bundle checks assert prototype safety of what actually ships (esbuild's own
spread helper, emitted once per format), so they cover the CJS entry as well as
the ESM one. They need a fresh build, so they live in their own spec behind
`npm run test:dist` — a bare `jest` skips them rather than resolving a stale or
missing `dist/`. The CJS entry is loaded in a real Node process, since jest's
ESM runtime cannot `require()` it.
The matrix ran only on Node 18, which reached end of life in April 2025, so
nothing verified the package on a Node anyone should be running. Node 24 is the
current LTS and is also the only leg where `require()` of an ESM dependency
works, so it is what actually executes the published CJS entry's assertions —
on 18 that spec can only report itself skipped.
@jarda-svoboda
jarda-svoboda force-pushed the fix/locale-key-safety branch from 0ee6479 to de73253 Compare July 27, 2026 01:22
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