diff --git a/.changeset/i18n-global-reset-4514.md b/.changeset/i18n-global-reset-4514.md new file mode 100644 index 000000000..93f3476d1 --- /dev/null +++ b/.changeset/i18n-global-reset-4514.md @@ -0,0 +1,14 @@ +--- +--- + +Test harness only, no published behaviour change (objectui#4514). + +Mounting an `I18nProvider` in a test file left react-i18next's global +default-instance pointer on that provider's instance, so every later +provider-less render in the same file resolved through it — a failure that +surfaced hundreds of lines away, in a test nobody had touched, and passed when +run alone. `vitest.setup.base.ts` now restores the pointer after every test. + +`I18nProvider` and `useObjectTranslation()` are unchanged: the global fallback +that makes the hook provider-safe stays exactly as designed. The five source +files in this change are all `*.test.*` plus their harness. diff --git a/package.json b/package.json index 44d62640d..6bff07c96 100644 --- a/package.json +++ b/package.json @@ -110,6 +110,7 @@ "playwright": "^1.62.1", "react": "19.2.8", "react-dom": "19.2.8", + "react-i18next": "^17.0.11", "react-router-dom": "^7.18.2", "rollup-plugin-visualizer": "^7.1.1", "tailwindcss": "^4.3.3", diff --git a/packages/fields/src/__tests__/date-locale-channel.test.tsx b/packages/fields/src/__tests__/date-locale-channel.test.tsx index 7fa862542..92041acd9 100644 --- a/packages/fields/src/__tests__/date-locale-channel.test.tsx +++ b/packages/fields/src/__tests__/date-locale-channel.test.tsx @@ -291,12 +291,15 @@ describe('channel precedence is unchanged (green both sides)', () => { * The third step of the precedence — the provider-less `'en'` last resort — * is pinned in `DateCellRenderer.test.tsx`, deliberately NOT here. * - * It cannot be measured in this file: `useObjectTranslation()` outside a - * provider reports `i18n.language` from react-i18next's GLOBAL instance, and - * every `I18nProvider` mounted above leaves that global on the language it - * was given. So a provider-less render placed after these cases resolves - * `'zh'` — which is the state react-i18next is in, not the fallback under - * test. Written here it would assert a fact about test ordering. + * It USED to be unmeasurable in this file: `useObjectTranslation()` outside + * a provider reports `i18n.language` from react-i18next's GLOBAL instance, + * and every `I18nProvider` mounted above left that global on its own + * instance. So a provider-less render placed after these cases resolved + * `'zh'` — the state react-i18next was in, not the fallback under test. + * + * objectui#4514 closed that: `installI18nGlobalReset()` in + * `vitest.setup.base.ts` restores the global after every test. The split + * below is now organisation, not a workaround. * * `DateCellRenderer.test.tsx` mounts no provider at all, so its * `6 days ago` / `Overdue 6d` / `Tomorrow` cases ARE that pin, and they hold diff --git a/packages/fields/src/widgets/GridField.test.tsx b/packages/fields/src/widgets/GridField.test.tsx index c55a410b1..57d3fc90e 100644 --- a/packages/fields/src/widgets/GridField.test.tsx +++ b/packages/fields/src/widgets/GridField.test.tsx @@ -332,10 +332,16 @@ describe('GridField / LineItemsField — editable line items', () => { // The `zh` half of this — the sub-grid following the SESSION locale // (objectui#4468) — lives in `__tests__/date-locale-channel.test.tsx`, - // not here. Mounting an `I18nProvider` anywhere in THIS file changes what - // the provider-less renders further down resolve, and the file-columns - // chip test reads a translated `aria-label`; keeping the provider in its - // own file keeps that coupling out of the way. + // grouped with the rest of the locale cases. + // + // It used to be a HAZARD rather than a preference: mounting an + // `I18nProvider` anywhere in this file left react-i18next's global on + // that provider's instance, so the file-columns chip test ~200 lines + // down read a Chinese `aria-label` and went red. objectui#4514 fixed + // that in the harness — `installI18nGlobalReset()` in + // `vitest.setup.base.ts` restores the global after every test, and + // `packages/i18n/src/__tests__/global-instance-reset.test.tsx` fails if + // it stops. A provider is safe to mount here now. }); }); diff --git a/packages/i18n/src/__tests__/global-instance-reset.test.tsx b/packages/i18n/src/__tests__/global-instance-reset.test.tsx new file mode 100644 index 000000000..a98ea8d8f --- /dev/null +++ b/packages/i18n/src/__tests__/global-instance-reset.test.tsx @@ -0,0 +1,153 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4514 — mounting an `I18nProvider` must not change what a LATER + * provider-less render in the same file resolves. + * + * ── Why this file exists rather than a comment ─────────────────────────── + * The trap was "documented where tests are written" three separate times — + * `packages/fields/src/widgets/GridField.test.tsx`, + * `packages/fields/src/__tests__/date-locale-channel.test.tsx` and + * `packages/plugin-timeline/src/__tests__/timeline-scale-vocabulary-defaults.test.ts` + * each carry a paragraph telling the next author to keep providers in a + * separate file. It still recurred, because a comment cannot fail. The + * mechanism is `installI18nGlobalReset()` in `vitest.setup.base.ts`; THIS file + * is what fails if it is removed or stops working. + * + * ── The ordering is the assertion ──────────────────────────────────────── + * Every case below is written provider-LESS *after* a case that mounted a + * provider, which is exactly the arrangement that used to be unsafe. + * + * Reverse-verified by ablating the `installI18nGlobalReset()` call in + * `vitest.setup.base.ts` and re-running this file together with its `unit` + * sibling: 5 failed / 7 passed (12), against 12 passed with the call in place. + * The five are the four cases marked "THE PIN" plus the provider-safe-fallback + * case — which is the point: without the reset that fallback resolves whatever + * the previous case installed, not `'en'`. + */ + +import { describe, it, expect } from 'vitest'; +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { getI18n } from 'react-i18next'; +import { I18nProvider, useObjectTranslation } from '../index'; + +/** + * Renders what it resolved instead of assigning to a module variable — writing + * to one during render is a side effect in render, which `react-hooks/globals` + * rejects (and would be a real hazard the moment React re-rendered this twice). + */ +function Probe() { + const { t, language } = useObjectTranslation(); + return ( +
+
{language}
+
{t('common.save')}
+ {/* A key no pack defines: pins the `defaultValue` path, which is what a + provider-less host actually renders (FileField's `aria-label` etc.). */} +
+ {t('probe.no.such.key', { defaultValue: 'INLINE-DEFAULT' })} +
+
+ ); +} + +function readProbe() { + return { + language: screen.getByTestId('probe-language').textContent, + save: screen.getByTestId('probe-save').textContent, + missing: screen.getByTestId('probe-missing').textContent, + }; +} + +/** The value a provider-less render resolves in a pristine file. */ +const PRISTINE = { language: 'en', save: 'common.save', missing: 'INLINE-DEFAULT' }; + +function renderZhProvider(node: React.ReactNode) { + return render( + + {node} + , + ); +} + +describe('objectui#4514 — the react-i18next global does not leak between tests', () => { + it('baseline: with no provider anywhere, the global is unset', () => { + expect(getI18n()).toBeUndefined(); + render(); + expect(readProbe()).toEqual(PRISTINE); + }); + + it('a mounted zh provider translates inside its own subtree', () => { + renderZhProvider(); + expect(readProbe()).toEqual({ language: 'zh', save: '保存', missing: 'INLINE-DEFAULT' }); + // ...and it really did install itself as the global while mounted. This is + // the deliberate design (direction 2 of the card, NOT taken): the global + // fallback is what makes `useObjectTranslation()` provider-safe. + expect(getI18n()).toBeDefined(); + expect(getI18n().language).toBe('zh'); + }); + + it('THE PIN: a provider-less render after that zh case still resolves pristine', () => { + // Before objectui#4514 this resolved `zh` / `保存` — the state react-i18next + // was left in by the case above, ~1 screen up. That is the whole defect. + expect(getI18n()).toBeUndefined(); + render(); + expect(readProbe()).toEqual(PRISTINE); + }); + + it('THE PIN: more than the language is restored', () => { + render( + + + , + ); + const mounted = getI18n(); + // While mounted, the provider's own resources and RTL direction are on the + // global — this is the part a `changeLanguage('en')` reset would NOT undo. + expect(mounted.t('probe.custom')).toBe('FROM-THE-PROVIDER'); + expect(mounted.dir()).toBe('rtl'); + }); + + it('THE PIN: the provider above left no resources, no direction, no instance', () => { + // A language-only reset would leave `probe.custom` reachable here (measured: + // it resolves through the instance's `en` resources instead of vanishing). + // Restoring the POINTER is what makes the leak total rather than partial. + expect(getI18n()).toBeUndefined(); + render(); + expect(readProbe()).toEqual(PRISTINE); + }); + + it('the provider-safe fallback itself still works (must-not-break)', () => { + // `useObjectTranslation()` outside a provider must not throw, must report a + // language, and must serve `defaultValue` — that is the deliberate design at + // packages/i18n/src/provider.tsx (`context?.language || i18n.language || 'en'`). + // The reset must not destroy it, only make it deterministic. + expect(() => render()).not.toThrow(); + expect(readProbe().language).toBe('en'); + expect(readProbe().missing).toBe('INLINE-DEFAULT'); + }); + + it('a provider still works when mounted AFTER all of the above', () => { + // The reset restores a pointer; it must not leave react-i18next in a state + // where a later provider cannot install itself. + renderZhProvider(); + expect(readProbe()).toEqual({ language: 'zh', save: '保存', missing: 'INLINE-DEFAULT' }); + }); +}); diff --git a/packages/i18n/src/__tests__/global-instance-reset.unit.test.ts b/packages/i18n/src/__tests__/global-instance-reset.unit.test.ts new file mode 100644 index 000000000..2cbee8657 --- /dev/null +++ b/packages/i18n/src/__tests__/global-instance-reset.unit.test.ts @@ -0,0 +1,58 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4514, the `unit` project's half. + * + * The sibling `global-instance-reset.test.tsx` pins the DOM projects. This one + * exists because the `unit` project is where the same leak is WORSE, and it is + * easy to assume node-env tests are out of reach of a React-flavoured bug: + * + * - `createI18n()` installs react-i18next's global from plain node code — no + * React, no render, no provider needed. `instance.use(initReactI18next)` + * is what does it, and `packages/i18n/src/__tests__/i18n.test.ts` calls + * `createI18n()` a dozen times. + * - the `unit` project runs `isolate: false` (vitest.config.mts), so its + * module graph — react-i18next's module-level pointer included — is shared + * across FILES in a worker. Uncontained, an instance installed by one file + * is still the global for the next file that worker picks up. + * + * A cross-file assertion would depend on worker assignment and file ordering, + * so this pins the per-test guarantee that makes the cross-file one hold. + */ + +import { describe, it, expect } from 'vitest'; +import { getI18n } from 'react-i18next'; +import { createI18n } from '../i18n'; + +describe('objectui#4514 — createI18n does not leave a global behind (unit project)', () => { + it('baseline: no global before anything in this file runs', () => { + expect(getI18n()).toBeUndefined(); + }); + + it('createI18n installs itself as the react-i18next global', () => { + const instance = createI18n({ defaultLanguage: 'zh', detectBrowserLanguage: false }); + // Not incidental — this is `initReactI18next`'s documented job, and the + // reason `useObjectTranslation()` is provider-safe. + expect(getI18n()).toBe(instance); + expect(getI18n().language).toBe('zh'); + }); + + it('THE PIN: the next test sees no global at all', () => { + expect(getI18n()).toBeUndefined(); + }); + + it('THE PIN: holds for a second instance too', () => { + createI18n({ defaultLanguage: 'ja', detectBrowserLanguage: false }); + expect(getI18n().language).toBe('ja'); + }); + + it('THE PIN: and again after that one', () => { + expect(getI18n()).toBeUndefined(); + }); +}); diff --git a/packages/plugin-timeline/src/__tests__/timeline-scale-vocabulary-defaults.test.ts b/packages/plugin-timeline/src/__tests__/timeline-scale-vocabulary-defaults.test.ts index abb537830..c6ee57439 100644 --- a/packages/plugin-timeline/src/__tests__/timeline-scale-vocabulary-defaults.test.ts +++ b/packages/plugin-timeline/src/__tests__/timeline-scale-vocabulary-defaults.test.ts @@ -12,13 +12,16 @@ * * ── Why this file mounts nothing ───────────────────────────────────────── * There is no React here at all — no `render`, no provider, not even an import - * of one. That is deliberate (objectui#4514): the sibling + * of one. That began as a workaround: the sibling * `timeline-scale-vocabulary.test.tsx` mounts an `I18nProvider` in every case, - * and every mounted provider leaves react-i18next's GLOBAL instance on the - * language it was given, so a provider-less assertion sharing that file would - * resolve `'zh'` and pin test ordering instead of the fallback. Keeping the - * pure-function cases in their own file makes the separation structural rather - * than a comment someone has to remember. + * and a mounted provider used to leave react-i18next's GLOBAL instance on its + * own instance, so a provider-less assertion sharing that file resolved `'zh'` + * and pinned test ordering instead of the fallback. + * + * objectui#4514 fixed the harness — `installI18nGlobalReset()` in + * `vitest.setup.base.ts` restores the global after every test — so the split is + * no longer load-bearing. It stays because a pure-function file that imports no + * React is worth having on its own merits. * * ── What the seam is ───────────────────────────────────────────────────── * `generateTimeScaleHeaders` is a pure exported function, so it cannot host a diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index dc8e5f92a..deb53ec2f 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -102,6 +102,9 @@ importers: react-dom: specifier: 19.2.8 version: 19.2.8(react@19.2.8) + react-i18next: + specifier: ^17.0.11 + version: 17.0.11(i18next@26.3.6(typescript@6.0.3))(react-dom@19.2.8(react@19.2.8))(react@19.2.8)(typescript@6.0.3) react-router-dom: specifier: ^7.18.2 version: 7.18.2(react-dom@19.2.8(react@19.2.8))(react@19.2.8) diff --git a/vitest.setup.base.ts b/vitest.setup.base.ts index 041cdbe1d..2013d3832 100644 --- a/vitest.setup.base.ts +++ b/vitest.setup.base.ts @@ -8,6 +8,28 @@ */ import { vi } from 'vitest'; +import { installI18nGlobalReset } from './vitest.setup.i18n-global'; + +// objectui#4514 — put react-i18next's GLOBAL default-instance pointer back +// after every test, so a provider-less render resolves the same way whether it +// sits above or below an `I18nProvider` in the same file. +// +// It lives HERE, in the one file every project's setup leads back to (`unit` +// directly; `dom` via vitest.setup.dom-light.tsx; `dom-heavy` and apps/console +// via vitest.setup.dom.tsx), rather than in the two DOM setups, for two +// measured reasons: +// +// 1. Both DOM setups already import this file, so one call covers them; two +// calls would be two places for a third DOM project to forget. +// 2. The `unit` project needs it MORE, not less. It runs `isolate: false`, so +// its module graph — including react-i18next's module-level pointer — is +// shared across FILES in a worker. `packages/i18n/src/__tests__/i18n.test.ts` +// alone calls `createI18n()` a dozen times, and each call installs a new +// global. There the leak outlives the file that caused it. +// +// Cost of the import here is bounded by the same `isolate: false`: the unit +// project pays react-i18next once per worker, not once per file. +installI18nGlobalReset(); // Polyfill ResizeObserver (Radix UI / Shadcn components reference it even in // node-env unit tests that import components transitively). diff --git a/vitest.setup.i18n-global.ts b/vitest.setup.i18n-global.ts new file mode 100644 index 000000000..f92f6b4c3 --- /dev/null +++ b/vitest.setup.i18n-global.ts @@ -0,0 +1,84 @@ +/** + * ObjectUI — restore react-i18next's GLOBAL default instance between tests. + * + * objectui#4514. Mounting an `I18nProvider` in a test file silently changed + * what every LATER provider-less render in that file resolved. This file is the + * mechanism that makes that impossible; `installI18nGlobalReset()` is called + * from `vitest.setup.base.ts`, which every project's setup leads back to. + * + * ## What actually leaks — measured, and it is NOT "the language" + * + * `createI18n()` (packages/i18n/src/i18n.ts) builds a FRESH instance and calls + * `instance.use(initReactI18next).init(...)`. react-i18next's `initReactI18next` + * is a `3rdParty` module whose `init` runs `setI18n(instance)` — it overwrites + * react-i18next's module-level default-instance POINTER. So every mounted + * provider does not mutate one shared instance's language; it REPLACES the + * global with its own instance, carrying that instance's language, resources, + * namespaces, `dir()` and missing-key handler. + * + * Measured on this tree, in one file, in order: + * + * before any provider getI18n() === undefined t('common.save') -> 'common.save' + * mount zh provider pointer swapped: true t('common.save') -> '保存' + * provider-less render language 'zh' t('common.save') -> '保存' <- the bug + * changeLanguage('en') language 'en' t('common.save') -> 'Save' + * + * That fourth line is why a language-only reset is not the fix. It does not + * restore the pristine state — it leaves the test's instance installed as the + * global, so a provider-less render that used to return the raw key now returns + * an en pack value. Measured with a provider carrying custom resources: after + * `changeLanguage('en')` the provider's own `probe.custom` key was STILL + * reachable through the global. Language is one field of the leak, not the leak. + * + * ## What this does instead + * + * Snapshot the pointer before the test file runs, and put that exact pointer + * back after every test. In this repo the snapshot is `undefined` (nothing + * installs a global at setup time), so a provider-less render resolves the same + * way on line 1 and line 600 of a file: through no global at all, which is + * `useObjectTranslation()`'s `|| 'en'` last resort plus i18next's key/ + * defaultValue behaviour. The provider-safe fallback is untouched — it is + * exactly the behaviour the FIRST test in every file already got. + * + * `I18nProvider`'s runtime behaviour is deliberately NOT changed (that was the + * card's direction 2, and the global fallback is what makes + * `useObjectTranslation()` provider-safe at all). This is harness state only. + * + * ## Why a function, and not top-level code + * + * Vitest executes setup FILES once per test file, but an `import` inside one is + * module-cached — under `isolate: false` (the `unit` project) a cached module's + * top-level `afterEach` would register once per worker and cover only the first + * file. Same reason `vitest.setup.dom-light.tsx` registers RTL's `cleanup()` + * itself instead of relying on RTL's import-time registration. Callers invoke + * this function so both the snapshot and the hook are per test file. + */ + +import { afterEach } from 'vitest'; +import { getI18n, setI18n } from 'react-i18next'; + +/** + * react-i18next types both accessors as total (`getI18n(): i18n`, + * `setI18n(instance: i18n)`), but the runtime is + * `let i18nInstance; setI18n = (i) => { i18nInstance = i; }` — a plain + * assignment over an initially `undefined` binding. `undefined` is therefore + * both a value `getI18n()` really returns (measured above) and one `setI18n` + * really accepts; the casts below are about that gap and nothing else. + * + * The instance type is derived from `getI18n` rather than imported from + * `i18next`, so this file needs ONE root devDependency instead of two and the + * type tracks whatever react-i18next itself declares. + */ +type MaybeInstance = ReturnType | undefined; + +export function installI18nGlobalReset(): void { + // Runs while the setup file executes — i.e. before the test file's own module + // scope, so this is what the file INHERITED, not anything it installed. + const pristine = (getI18n as () => MaybeInstance)(); + + afterEach(() => { + if ((getI18n as () => MaybeInstance)() !== pristine) { + (setI18n as (instance: MaybeInstance) => void)(pristine); + } + }); +}