diff --git a/.changeset/one-number-display-home-4576.md b/.changeset/one-number-display-home-4576.md new file mode 100644 index 000000000..ecb9e4bc8 --- /dev/null +++ b/.changeset/one-number-display-home-4576.md @@ -0,0 +1,16 @@ +--- +'@object-ui/core': minor +'@object-ui/i18n': minor +--- + +One home for the number-display policy — and a percent stops meaning two different things between a list cell and a dashboard measure + +`formatDisplayNumber`, `shouldGroupDisplayNumber` and `DisplayNumberFormatOptions` move from `@object-ui/i18n` into `@object-ui/core`. `@object-ui/i18n` re-exports all three under the same names, so every existing import path keeps working unchanged and both spellings resolve to the same function object; nothing published was removed. + +The move is what fixes the bug. `@object-ui/core`'s `formatMeasure` needed exactly this policy and could not import it — `core` is the React-free engine and is a runtime dependency of React-free consumers (the `object-ui` VS Code extension, `@object-ui/data-objectstack`), while `i18n` depends on `i18next`/`react-i18next` and peer-depends on React. So `formatMeasure` carried a parallel `Intl` implementation, recorded at both ends as deliberate duplication, and the two drifted in the one place a hand-built string and `Intl` disagree. A German session read `1.234,5 %` from a list cell and `1.234,5%` from a dashboard measure showing the same number. The function is pure, so the boundary was never a property of the code — only of where the code sat; moving it down removes the obstacle instead of working around it. `core` imports nothing from `i18n`, so the new edge adds no cycle. + +**Behaviour change — a measure's percent sign now follows the locale.** `formatMeasure` appended a literal `%` in every locale; it now renders the locale's own percent convention, the same one the list-cell `formatPercent` has used since the fix to its own machine-locale defect. Measured to change output in de, fr, es, ru, sv, cs, fi (a no-break space appears before the sign), tr (the sign moves to the FRONT: `%1.234,5`) and ar (its own percent sign plus U+061C). English, Japanese and Chinese are byte-identical — their convention is a bare trailing sign — which is why this was invisible in an English session. + +**No numeral moves, in any locale, at any magnitude.** The obvious route to the locale's convention is `Intl`'s `style: 'percent'`, but that style expects a fraction, so a value already in percentage points would have to be divided by 100 for `Intl` to multiply it straight back — and that round trip is lossy. Measured, it moves 27,581 of 1,200,013 ordinary-magnitude en-US forms at rounding ties (`0.175` at two decimals becomes `0.17%` instead of `0.18%`), plus `MAX_SAFE_INTEGER` and everything from 1e23 up, where `100,000,000,000,000,000,000,000%` becomes `99,999,999,999,999,990,000,000%`. The percentage points are formatted directly instead, through a new `style: 'percentPoints'` on `DisplayNumberFormatOptions`; that route was measured to produce a byte-identical percent affix to `style: 'percent'` across all 171 locale tags tested while moving none of those 1,200,013 forms. Callers holding a fraction keep using `style: 'percent'`, whose behaviour is unchanged — naming the two cases apart is what stops the next caller from reaching for the lossy one. + +`@object-ui/i18n`'s entry declaration is byte-identical, but the declaration it points at now lives in `@object-ui/core` and the package gains that dependency, so it takes the same minor bump rather than a patch. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.measureLocale.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.measureLocale.test.tsx index 6be71c81b..100ec0282 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.measureLocale.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/DatasetDefaultInspector.measureLocale.test.tsx @@ -127,7 +127,15 @@ describe('dataset inspector format sample follows the display locale (objectui#4 ); // The decimal COMMA is the point: `12.3%` and `12,3%` read as different // numbers to the two audiences, not as the same one restyled. - expect(sampleText()).toBe('12,3%'); + // + // PIN MOVED (objectui#4576). The German expectation gained the no-break + // space before the sign. `formatMeasure` appended a literal '%' in every + // locale; it now renders the locale's own percent convention, the one a + // list cell has used since #4553. The en sample in the case below is + // UNMOVED, which is what says this was a convention change and not a + // numeral one. Same class as the two moves in + // `packages/core/src/utils/__tests__/dataset-format.locale.test.ts`. + expect(sampleText()).toBe('12,3\u00a0%'); }); it('keeps the en percent sample byte-identical (must-not-change)', () => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 4938d6dbc..91506411d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -59,6 +59,12 @@ export * from './utils/dashboard-filters.js'; export * from './utils/merge-filters.js'; export * from './utils/compare-to.js'; export * from './utils/chart-series.js'; +// The ONE number-display formatter (objectui#4033) — grouping policy, display +// locale and the percent convention. It lived in `@object-ui/i18n` until +// objectui#4576; it is pure, and living above `core` was what kept +// `dataset-format` below from reaching it (so the two drifted). `@object-ui/i18n` +// re-exports these names unchanged, so both import paths name the same symbol. +export * from './utils/number-display.js'; export * from './utils/dataset-format.js'; // Pivot lookup-key encoders, shared by every cross-tab renderer so the // dashboard widget and the report renderer key their buckets identically diff --git a/packages/core/src/utils/__tests__/dataset-format.locale.test.ts b/packages/core/src/utils/__tests__/dataset-format.locale.test.ts index f877ae19b..d242a813c 100644 --- a/packages/core/src/utils/__tests__/dataset-format.locale.test.ts +++ b/packages/core/src/utils/__tests__/dataset-format.locale.test.ts @@ -67,12 +67,22 @@ describe('formatMeasure follows the display locale (objectui#4566)', () => { it('formats a percent measure in the threaded locale', () => { // The decimal COMMA is the whole point: `60.8%` and `60,8%` read as // different numbers to the two audiences, not as the same one restyled. - expect(formatMeasure(0.608_333_333, '0.0%', undefined, undefined, 'de-DE')).toBe('60,8%'); + // + // PIN MOVED (objectui#4576). The German expectation gained the no-break + // space before the sign. That is not this case's subject drifting — it is + // the divergence #4566 recorded and declined to fix here, now closed: a + // German list cell has written `60,8${NBSP}%` since #4553, and the measure + // appended a literal '%'. The en-US line beside it is UNMOVED, which is + // what says this was a convention change and not a numeral one. + expect(formatMeasure(0.608_333_333, '0.0%', undefined, undefined, 'de-DE')).toBe(`60,8${NBSP}%`); expect(formatMeasure(0.608_333_333, '0.0%', undefined, undefined, 'en-US')).toBe('60.8%'); }); it('honours a declared percentScale in the threaded locale', () => { - expect(formatMeasure(1, '0.0%', undefined, 'fraction', 'de-DE')).toBe('100,0%'); + // PIN MOVED (objectui#4576) — same reason as the case above; the declared + // `percentScale` behaviour it actually pins is untouched, and en-US is + // byte-identical. + expect(formatMeasure(1, '0.0%', undefined, 'fraction', 'de-DE')).toBe(`100,0${NBSP}%`); expect(formatMeasure(1, '0.0%', undefined, 'fraction', 'en-US')).toBe('100.0%'); }); diff --git a/packages/core/src/utils/__tests__/dataset-format.percent-convention.test.ts b/packages/core/src/utils/__tests__/dataset-format.percent-convention.test.ts new file mode 100644 index 000000000..a9d795723 --- /dev/null +++ b/packages/core/src/utils/__tests__/dataset-format.percent-convention.test.ts @@ -0,0 +1,192 @@ +/** + * 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#4576 — a percent renders as `1.234,5 %` in a list cell and + * `1.234,5%` as a dashboard measure. Two conventions for one number. + * + * `formatPercent` (`@object-ui/fields`) has gone through `Intl` since #4553, so + * it gets the locale's percent CONVENTION — German writes a no-break space + * before the sign. `formatMeasure` appended a LITERAL '%' to a decimal body, so + * it got no space in any locale. The SCALING already agreed (both go through + * `percentDisplayValue`); the CONVENTION did not, which is exactly what + * `percentDisplayValue`'s doc comment promises can never happen. + * + * ── PREDICTIONS, written before the run ── + * Reverse verification takes the fix out (`git diff` to a patch + + * `git checkout --`, never `git stash`) and keeps these expectations. + * + * RED before the fix (the convention move): + * - every de-DE / fr-FR / ru-RU / es-ES case — they gain U+00A0 before '%' + * - the tr-TR case — the sign moves to the FRONT of the number + * - the ar-EG case — its own percent sign (U+066A) plus U+061C, not ASCII '%' + * - the cross-surface agreement cases, which is the card's actual complaint + * + * GREEN on BOTH sides (must-not-change pins, each labelled in place): + * - every en-US case, ordinary magnitude AND extreme — this is the discriminator + * - ja-JP / zh-CN, whose percent convention has no space either + * - the rounding-tie cases, which pin the route NOT taken (see below) + * + * Every case pins at least one locale whose convention has a space AND en-US, + * because a lone de assertion is not falsifiable on a German runner. Runner + * measured: node v22.22.2, ICU 78.2, machine locale en-US. + * + * ⚠️ The en cases are not decoration. The obvious way to adopt the locale's + * convention is `Intl`'s `style: 'percent'` — the way `formatPercent` does it — + * but that style wants a FRACTION, so a value already in percentage points must + * be divided by 100 for `Intl` to multiply it straight back, and that round trip + * is lossy at rounding TIES. Measured on this runner: 27,581 of 1,200,013 + * ordinary-magnitude en-US forms move that way (`0.175` at 2 decimals goes from + * `0.18%` to `0.17%`), plus `MAX_SAFE_INTEGER` and everything from 1e23 up. The + * implementation formats the percentage points DIRECTLY with the percent unit + * instead, which was measured byte-identical on the affix across all 171 locale + * tags tested and moves none of those 1,200,013 forms. The tie and + * extreme-magnitude cases below are what keep that route honest: they go red if + * anyone "simplifies" this into the divide-by-100 form. + */ + +import { describe, it, expect } from 'vitest'; +import { formatMeasure } from '../dataset-format'; + +// The locale's own percent affix, taken from `Intl` rather than hardcoded — +// this is the SAME source `formatPercent` renders through, so comparing against +// it is a structural pin on "both surfaces use the locale's convention" rather +// than a restatement of today's CLDR data. +function percentAffix(locale: string): { prefix: string; suffix: string } { + let prefix = ''; + let suffix = ''; + let seenNumber = false; + for (const part of new Intl.NumberFormat(locale, { style: 'percent' }).formatToParts(1234.5)) { + if (part.type === 'integer' || part.type === 'group' || part.type === 'decimal' || part.type === 'fraction') { + seenNumber = true; + continue; + } + if (part.type === 'minusSign' || part.type === 'plusSign') continue; + if (seenNumber) suffix += part.value; + else prefix += part.value; + } + return { prefix, suffix }; +} + +describe('formatMeasure percent — the locale\'s convention, not a literal sign (#4576)', () => { + it('German puts a no-break space before the sign, as the list cell already did', () => { + // U+00A0 written as an escape, never as a raw byte. + expect(formatMeasure(0.608_333_333, '0.0%', undefined, undefined, 'de-DE')).toBe('60,8\u00a0%'); + expect(formatMeasure(1, '0.0%', undefined, 'fraction', 'de-DE')).toBe('100,0\u00a0%'); + expect(formatMeasure(80, '0.0%', undefined, 'whole', 'de-DE')).toBe('80,0\u00a0%'); + // en-US pinned alongside so the case is falsifiable on a German runner. + expect(formatMeasure(0.608_333_333, '0.0%', undefined, undefined, 'en-US')).toBe('60.8%'); + }); + + it('French and Russian do the same; Spanish too', () => { + // fr-FR groups with U+202F (narrow no-break space) and spaces the sign with U+00A0. + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'fr-FR')).toBe('1\u202f234,5\u00a0%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'ru-RU')).toBe('1\u00a0234,5\u00a0%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'es-ES')).toBe('1234,5\u00a0%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'en-US')).toBe('1,234.5%'); + }); + + it('Turkish puts the sign in FRONT — a convention a literal suffix can never reach', () => { + // The strongest case in the file: no amount of "append '%'" produces this, + // so it cannot pass by accident. + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'tr-TR')).toBe('%1.234,5'); + }); + + it('Arabic uses its OWN percent sign, not ASCII "%"', () => { + const out = formatMeasure(50, '0%', undefined, 'whole', 'ar-EG'); + // U+066A ARABIC PERCENT SIGN, then U+061C ARABIC LETTER MARK. + expect(out).toContain('\u066a'); + expect(out).not.toContain('%'); + }); + + it('agrees with the locale\'s percent affix — the same one the list cell renders through', () => { + for (const locale of ['de-DE', 'fr-FR', 'tr-TR', 'ar-EG', 'en-US', 'ja-JP', 'zh-CN']) { + const { prefix, suffix } = percentAffix(locale); + const out = formatMeasure(1234.5, '0.0%', undefined, 'whole', locale); + expect(out.startsWith(prefix), `${locale} prefix`).toBe(true); + expect(out.endsWith(suffix), `${locale} suffix`).toBe(true); + } + }); + + // ── must-not-change pins ──────────────────────────────────────────────── + // GREEN on both sides of the fix. They are the discriminator: this card + // changes a rendering CONVENTION, and English has no space in its convention, + // so English must not move at all. + + it('MUST NOT CHANGE: ordinary-magnitude en-US is byte-identical', () => { + expect(formatMeasure(50, '0%')).toBe('50%'); + expect(formatMeasure(0.75, '0%')).toBe('75%'); + expect(formatMeasure(0.608_333_333, '0.0%')).toBe('60.8%'); + expect(formatMeasure(0, '0%')).toBe('0%'); + expect(formatMeasure(1, '0%')).toBe('1%'); + expect(formatMeasure(1, '0.0%', undefined, 'fraction')).toBe('100.0%'); + expect(formatMeasure(0.6667, '0.0%', undefined, 'fraction')).toBe('66.7%'); + expect(formatMeasure(0, '0.0%', undefined, 'fraction')).toBe('0.0%'); + expect(formatMeasure(1, '0.0%', undefined, 'whole')).toBe('1.0%'); + expect(formatMeasure(0.5, '0.0%', undefined, 'whole')).toBe('0.5%'); + expect(formatMeasure(80, '0.0%', undefined, 'whole')).toBe('80.0%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'en-US')).toBe('1,234.5%'); + }); + + it('MUST NOT CHANGE: ja-JP and zh-CN have no space in their convention either', () => { + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'ja-JP')).toBe('1,234.5%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'zh-CN')).toBe('1,234.5%'); + }); + + it('MUST NOT CHANGE: rounding ties keep the AUTHORED decimal (the /100 route is refused)', () => { + // 0.175 percentage points at 2 decimals. Formatted directly this is + // `0.18%`. Divided by 100 for `Intl`'s `style: 'percent'` to multiply back, + // the double lands below the tie and it becomes `0.17%`. This case is the + // pin on which route is taken — it goes red the moment someone rewrites + // this path as `style: 'percent'` on `display / 100`. + expect(formatMeasure(0.175, '0.00%', undefined, 'whole')).toBe('0.18%'); + expect(formatMeasure(0.305, '0.00%', undefined, 'whole')).toBe('0.31%'); + expect(formatMeasure(0.35, '0.0%', undefined, 'whole')).toBe('0.4%'); + }); + + it('a tie keeps its digits AND gains the German space \u2014 both halves at once', () => { + // Deliberately NOT filed under "MUST NOT CHANGE": the German form moves + // (it gains U+00A0), so this case is RED before the fix. Its job is to show + // the two properties are independent \u2014 adopting the convention did not cost + // the digits, which is the whole argument for the route taken. + expect(formatMeasure(0.175, '0.00%', undefined, 'whole', 'de-DE')).toBe('0,18\u00a0%'); + expect(formatMeasure(0.175, '0.00%', undefined, 'whole', 'en-US')).toBe('0.18%'); + }); + + it('MUST NOT CHANGE: extreme magnitudes keep every digit', () => { + // The `/100` round trip corrupts both of these (measured: + // `9,007,199,254,740,990%` and `99,999,999,999,999,990,000,000%`). + // #4577 measured 24 of its 32,760 combinations moving that way and declined + // the swap for it; this implementation does not move them at all. + expect(formatMeasure(Number.MAX_SAFE_INTEGER, '0%', undefined, 'whole')).toBe( + '9,007,199,254,740,991%', + ); + expect(formatMeasure(1e21, '0%', undefined, 'fraction')).toBe( + '100,000,000,000,000,000,000,000%', + ); + }); + + it('MUST NOT CHANGE: non-percent formats gain no sign, and the legacy "$" literal still leads', () => { + expect(formatMeasure(1234.5, '0,0')).toBe('1,235'); + expect(formatMeasure(1234.5, '$0.0')).toBe('$1,234.5'); + expect(formatMeasure(1234.5, '$0.0%', undefined, 'whole')).toBe('$1,234.5%'); + }); + + it('the legacy "$" literal still leads a German percent too \u2014 it sits OUTSIDE the affix', () => { + // RED before the fix (the German form gains U+00A0). Pinned separately from + // the must-not-change case above so that label stays honest. + expect(formatMeasure(1234.5, '$0.0%', undefined, 'whole', 'de-DE')).toBe('$1.234,5\u00a0%'); + }); + + it('MUST NOT CHANGE: a malformed locale tag degrades instead of throwing', () => { + // `en_US` (underscore) is the likeliest tenant-config typo and throws from a + // bare `Intl.NumberFormat`. The retry lives in `formatDisplayNumber`. + expect(() => formatMeasure(50, '0%', undefined, 'whole', 'en_US')).not.toThrow(); + expect(formatMeasure(50, '0%', undefined, 'whole', 'en_US')).toMatch(/50.?%|%.?50/u); + }); +}); diff --git a/packages/core/src/utils/__tests__/number-display.percent-points.test.ts b/packages/core/src/utils/__tests__/number-display.percent-points.test.ts new file mode 100644 index 000000000..ee9c806a9 --- /dev/null +++ b/packages/core/src/utils/__tests__/number-display.percent-points.test.ts @@ -0,0 +1,159 @@ +/** + * 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#4576 — `style: 'percentPoints'`, the option that lets a caller + * holding percentage POINTS render the locale's percent convention without + * `Intl`'s x100 re-scaling. + * + * The distinction is the whole of this card. `Intl`'s `style: 'percent'` wants a + * FRACTION, so a caller with percentage points has to divide by 100 for `Intl` + * to multiply straight back — and that round trip is lossy at rounding ties. + * Both spellings exist here so the choice is made once, by name, at the call + * site, instead of being re-derived (wrongly) at each one. + * + * ── PREDICTIONS, written before the run ── + * The whole file is RED before the fix: `style: 'percentPoints'` does not exist + * on `DisplayNumberFormatOptions`, so it fails to compile under `tsc` and every + * case falls through to plain decimal formatting under vitest (`80` instead of + * `80%`). The two `style: 'percent'` cases are the exception — they are GREEN on + * both sides, and are here to pin the CONTRAST rather than the new behaviour. + * + * Every no-break space is written as a `\u00a0` escape, never as a raw byte. + * Runner measured: node v22.22.2, ICU 78.2, machine locale en-US. + */ + +import { describe, it, expect } from 'vitest'; +import { formatDisplayNumber } from '../number-display'; + +describe('formatDisplayNumber — style: percentPoints (#4576)', () => { + it('does NOT re-scale: 80 points renders as 80%, where style:percent would say 8,000%', () => { + expect(formatDisplayNumber(80, { locale: 'en-US', style: 'percentPoints' })).toBe('80%'); + // GREEN on both sides — the contrast, not the new behaviour. `percent` + // treats the same number as a fraction, which is what makes a caller + // holding points need the other spelling. + expect(formatDisplayNumber(80, { locale: 'en-US', style: 'percent' })).toBe('8,000%'); + }); + + it('carries the locale percent CONVENTION, including the no-break space and the prefix position', () => { + expect( + formatDisplayNumber(1234.5, { + locale: 'de-DE', + style: 'percentPoints', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }), + ).toBe('1.234,5\u00a0%'); + // Turkish moves the sign to the FRONT — unreachable by appending a literal. + expect( + formatDisplayNumber(1234.5, { + locale: 'tr-TR', + style: 'percentPoints', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }), + ).toBe('%1.234,5'); + expect( + formatDisplayNumber(1234.5, { + locale: 'en-US', + style: 'percentPoints', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }), + ).toBe('1,234.5%'); + }); + + it('produces the SAME percent affix as style:percent, across every locale the console ships', () => { + // The structural claim behind the implementation choice: `style: 'unit'` / + // `unit: 'percent'` was measured to give a byte-identical affix to + // `style: 'percent'` across all 171 locale tags tested. These are the ten + // the repo ships locale packs for, plus four whose convention differs most. + const tags = ['en', 'zh', 'ja', 'ko', 'de', 'fr', 'es', 'pt', 'ru', 'ar', + 'tr-TR', 'bn-IN', 'sv-SE', 'cs-CZ']; + for (const locale of tags) { + const points = formatDisplayNumber(1234.5, { + locale, + style: 'percentPoints', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }); + // The same magnitude expressed as a fraction, through `Intl`'s own percent. + const fraction = new Intl.NumberFormat(locale, { + style: 'percent', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }).format(12.345); + expect(points, `affix parity for ${locale}`).toBe(fraction); + } + }); + + it('keeps the AUTHORED decimal at rounding ties, where the /100 round trip does not', () => { + // Measured: 27,581 of 1,200,013 ordinary-magnitude en-US forms differ + // between the two routes. These are three of them. + for (const [value, digits, expected] of [ + [0.175, 2, '0.18%'], + [0.305, 2, '0.31%'], + [0.35, 1, '0.4%'], + ] as const) { + expect( + formatDisplayNumber(value, { + locale: 'en-US', + style: 'percentPoints', + minimumFractionDigits: digits, + maximumFractionDigits: digits, + }), + ).toBe(expected); + // and the route this option exists to avoid, shown failing to agree + expect( + new Intl.NumberFormat('en-US', { + style: 'percent', + minimumFractionDigits: digits, + maximumFractionDigits: digits, + }).format(value / 100), + ).not.toBe(expected); + } + }); + + it('keeps every digit at the top of the double range', () => { + expect( + formatDisplayNumber(Number.MAX_SAFE_INTEGER, { locale: 'en-US', style: 'percentPoints' }), + ).toBe('9,007,199,254,740,991%'); + expect(formatDisplayNumber(1e23, { locale: 'en-US', style: 'percentPoints' })).toBe( + '100,000,000,000,000,000,000,000%', + ); + }); + + it('still survives a malformed locale tag by retrying without it', () => { + // `en_US` (underscore) throws from a bare `Intl.NumberFormat`; the retry is + // the one in `formatDisplayNumber`, shared with every other style. + expect(() => formatDisplayNumber(50, { locale: 'en_US', style: 'percentPoints' })).not.toThrow(); + expect(formatDisplayNumber(50, { locale: 'en_US', style: 'percentPoints' })).toBe('50%'); + }); + + it('a declared currency still wins — money is never a percentage', () => { + // Nothing in the repo passes both; the pin records which one governs so a + // future caller cannot discover it by accident. + expect( + formatDisplayNumber(1234.5, { + locale: 'en-US', + style: 'percentPoints', + currency: 'USD', + minimumFractionDigits: 2, + maximumFractionDigits: 2, + }), + ).toBe('$1,234.50'); + }); + + it('the grouping policy applies to percentages as it does to everything else', () => { + // scale 0 with no currency is an ordinal → ungrouped, in this style too. + expect( + formatDisplayNumber(1234, { locale: 'en-US', style: 'percentPoints', scale: 0 }), + ).toBe('1234%'); + expect(formatDisplayNumber(1234, { locale: 'en-US', style: 'percentPoints' })).toBe('1,234%'); + }); +}); diff --git a/packages/core/src/utils/dataset-format.ts b/packages/core/src/utils/dataset-format.ts index c53ff167b..19f6f2677 100644 --- a/packages/core/src/utils/dataset-format.ts +++ b/packages/core/src/utils/dataset-format.ts @@ -20,6 +20,8 @@ import type { AnalyticsResult } from '@objectstack/spec/contracts'; import type { PercentScale } from '@objectstack/spec/data'; +import { formatDisplayNumber, type DisplayNumberFormatOptions } from './number-display.js'; + /** * Column metadata the analytics server returns alongside the rows — the spec's * `AnalyticsResult.fields[]` element BY REFERENCE, never a local restatement of @@ -74,6 +76,16 @@ export type { PercentScale } from '@objectstack/spec/data'; * percent cell renderer (`formatPercent` in `@object-ui/fields`) and the dataset * measure formatter ({@link formatMeasure}) so a percent renders identically as * a row value and as an aggregated metric — the two surfaces can never drift. + * + * ⚠️ That last sentence was briefly FALSE, and objectui#4576 is what made it + * true again. Sharing the SCALING was never enough on its own: between #4553 + * and #4576 the cell renderer got the locale's percent CONVENTION from `Intl` + * while {@link formatMeasure} appended a literal '%', so a German session read + * `1.234,5 %` in a list cell and `1.234,5%` in a dashboard measure — the same + * number, scaled identically, rendered under two conventions. Both ends now go + * through the locale's own percent affix. If a third surface ever needs percent + * display, it takes BOTH halves from here — the scaling AND the convention — + * or this promise breaks again in the same place. */ export function percentDisplayValue(value: number): number { return value > -1 && value < 1 ? value * 100 : value; @@ -82,35 +94,40 @@ export function percentDisplayValue(value: number): number { /** * Format one number in the DISPLAY locale, surviving a malformed locale tag. * - * ⚠️ SURVIVING DUPLICATION, recorded deliberately (objectui#4566). This mirrors - * the `try`/`catch` retry inside `formatDisplayNumber` - * (`@object-ui/i18n`'s `utils/number-display.ts`) — the ONE number-display - * formatter — and the mirror exists only because of a PACKAGE BOUNDARY, not - * because the behaviour differs. See {@link formatMeasure}'s note for the - * measurement that chose this over routing through it. Keep the two in step: - * a change to the retry policy there belongs here too. + * THE DUPLICATE IS GONE (objectui#4576). This used to be a hand-mirrored copy + * of the `try`/`catch` retry inside `formatDisplayNumber` — the ONE + * number-display formatter — kept in step by a comment at each end because a + * PACKAGE BOUNDARY stood between them: `formatDisplayNumber` lived in + * `@object-ui/i18n`, which depends on `i18next`/`react-i18next` and + * peer-depends on React, and this package is the React-free engine. #4566 + * measured the option mapping LOSSLESS across 32,760 combinations and recorded + * the duplication rather than crossing that boundary; #4576 removed the + * boundary instead, by moving the pure function DOWN into this package + * (`./number-display.ts`), where `@object-ui/i18n` now re-exports it. * - * The retry is load-bearing rather than defensive. Before #4566 these sites - * passed a literal `undefined`, which never throws; a THREADED tag can be - * malformed, and a bare `Intl.NumberFormat('en_US', …)` throws `RangeError` - * (measured — underscore instead of hyphen is the likeliest tenant-config - * typo). Un-caught that would take the whole widget down, so a bad tag falls - * back to the runtime default exactly as `formatDisplayNumber` does. A bad + * So this is a thin ADAPTER, not a second implementation: it converts the + * `Intl`-shaped option bag these call sites already speak into + * {@link DisplayNumberFormatOptions}, and every policy decision — grouping, + * the malformed-locale retry, the percent convention — is made in the one + * home. The retry is load-bearing rather than defensive: before #4566 these + * sites passed a literal `undefined`, which never throws, but a THREADED tag + * can be malformed and a bare `Intl.NumberFormat('en_US', …)` throws + * `RangeError` (measured — underscore instead of hyphen is the likeliest + * tenant-config typo), which would take the whole widget down. A bad * `currency` still throws out of both attempts, which is what lets * {@link formatMeasure}'s own `catch` fall through to plain-number formatting. + * + * ⚠️ `scale` is deliberately never passed. A measure carries no field `scale` + * (its decimals come from a numeral format PATTERN), so the grouping policy + * keyed on it must not fire here — which is exactly why #4566 measured the two + * implementations byte-identical across all 32,760 combinations. */ function formatNumberInLocale( value: number, locale: string | undefined, - options: Intl.NumberFormatOptions, + options: Omit, ): string { - try { - return new Intl.NumberFormat(locale, options).format(value); - } catch { - // Retry WITHOUT the locale, keeping every other option: rescues a malformed - // tag while still surfacing a genuinely bad `currency` to the caller. - return new Intl.NumberFormat(undefined, options).format(value); - } + return formatDisplayNumber(value, { ...options, locale }); } /** @@ -133,32 +150,25 @@ function formatNumberInLocale( * `1,234.5` beside a grid cell rendering the same number as `1.234,5`, and * inverted separators read as a different number, not an unstyled one. * - * ── Why this still formats itself instead of calling `formatDisplayNumber` ── - * The one-resolver rule says this should route through `formatDisplayNumber` - * (`@object-ui/i18n`) and retire the parallel implementation. That was measured - * across 32,760 value × format × currency × percentScale × locale combinations - * and the option mapping is LOSSLESS — routing through it changes literally no - * byte of output, because the policy layer `formatDisplayNumber` adds over - * plain `Intl` is grouping suppression keyed on a field's declared `scale`, and - * a measure has no `scale` to feed it (decimals here come from a numeral format - * PATTERN, so grouping stays on and the two agree everywhere). + * ── This routes through `formatDisplayNumber` (objectui#4576) ── + * It did not always. #4566 measured the option mapping across 32,760 + * value × format × currency × percentScale × locale combinations and found it + * LOSSLESS — routing changes no byte of output, because the policy layer + * `formatDisplayNumber` adds over plain `Intl` is grouping suppression keyed on + * a field's declared `scale`, and a measure has no `scale` to feed it (decimals + * here come from a numeral format PATTERN, so grouping stays on and the two + * agree everywhere). What blocked it was the PACKAGE BOUNDARY: the formatter + * lived in `@object-ui/i18n`, which depends on `i18next`/`react-i18next` and + * peer-depends on React, while this package is the React-free engine (AGENTS.md + * §3: "No UI-lib deps. Logic only.") and a runtime dependency of React-FREE + * consumers — the `object-ui` VS Code extension and `@object-ui/data-objectstack`. * - * What blocks the routing is the PACKAGE BOUNDARY, not the behaviour. - * `@object-ui/core` is the React-free engine (see this module's header, and the - * topology table in AGENTS.md §3: "No UI-lib deps. Logic only."), while - * `@object-ui/i18n` depends on `i18next`/`react-i18next` and peer-depends on - * React, and publishes no pure-utility subpath to import in isolation — its - * `exports` map is `.` and `./locales/*`. A `core` → `i18n` edge would put - * React into the dependency closure of every React-FREE consumer of this - * package: the `object-ui` VS Code extension and `@object-ui/data-objectstack` - * both take `@object-ui/core` as a runtime dependency and declare no React. - * - * So the duplication survives on purpose and is recorded at BOTH ends (see - * `formatNumberInLocale` above and the note in `number-display.ts`). Retiring - * it for real means moving `formatDisplayNumber` DOWN into this package and - * re-exporting it from `@object-ui/i18n` — the right direction, since `core` is - * the lower layer — but that relocates a published export across a package - * boundary and is deliberately left to its own card. + * #4576 removed the boundary rather than working around it: `formatDisplayNumber` + * is pure, so it moved DOWN into this package (`./number-display.ts`) and + * `@object-ui/i18n` re-exports it. The duplicate implementation this file used + * to carry is gone, and with it the drift that duplicate produced — see the + * percent note in the body, and {@link percentDisplayValue}'s promise, which + * is true again. */ export function formatMeasure( v: unknown, @@ -174,8 +184,10 @@ export function formatMeasure( if (currency) { try { + // No `style: 'currency'` here: `formatDisplayNumber` derives that from the + // presence of `currency` itself, which is also what keeps its "money + // always groups" policy in one place instead of two. return formatNumberInLocale(v, locale, { - style: 'currency', currency, minimumFractionDigits: decimals ?? 0, maximumFractionDigits: decimals ?? 2, @@ -217,22 +229,35 @@ export function formatMeasure( const display = isPercent ? (percentScale ? (percentScale === 'fraction' ? v * 100 : v) : percentDisplayValue(v)) : v; + // The percent sign is the LOCALE's, not a literal '%' (objectui#4576). + // + // Until #4576 this appended a hard-coded '%' to a decimal-formatted body, so + // a German session read `1.234,5%` from a dashboard measure beside + // `1.234,5 %` (no-break space — the German percent convention) from a list + // cell showing the SAME number, because `formatPercent` had gone through + // `Intl` since #4553. That contradicted {@link percentDisplayValue}'s own + // promise that the two surfaces "can never drift": the SCALING had stopped + // drifting, the CONVENTION had started. Measured to differ in de, fr, es, ru, + // sv, cs, fi (no-break space), tr (the sign moves to the FRONT: `%1.234,5`) + // and ar (its own sign plus U+061C); en, ja, zh were already identical. + // + // `style: 'percentPoints'` is what closes it, and the choice of route is + // measured, not incidental. `display` is already in percentage POINTS, while + // `Intl`'s `style: 'percent'` wants a fraction — so routing through that (the + // way `formatPercent` does) would mean dividing by 100 for `Intl` to multiply + // straight back, and that round trip is lossy at rounding TIES: 27,581 of + // 1,200,013 ordinary-magnitude en-US forms move (`0.175` at 2 decimals goes + // from `0.18%` to `0.17%`), plus `MAX_SAFE_INTEGER` and everything from 1e23 + // up. Formatting the points directly with the percent UNIT was measured to + // produce a byte-identical percent affix to `style: 'percent'` across all 171 + // locale tags tested, while moving ZERO of those 1,200,013 en forms — the + // same convention by a route that does not touch the value. const body = formatNumberInLocale(display, locale, { + style: isPercent ? 'percentPoints' : 'decimal', minimumFractionDigits: decimals ?? 0, maximumFractionDigits: decimals ?? 0, }); - // The '%' stays a LITERAL suffix rather than `Intl`'s `style: 'percent'`, - // and that is a measured choice, not an oversight (objectui#4566). The - // percent STYLE would re-scale by 100, and the round trip loses precision at - // the top of the range: en `100,000,000,000,000,000,000,000%` becomes - // `99,999,999,999,999,990,000,000%`. English output moving is the signal that - // a mapping is wrong on this card — unlike `formatPercent`'s (objectui#4553), - // where en had never grouped and moving it WAS the fix. The consequence is - // that a German session reads `1.234,5%` here and `1.234,5 %` (no-break space, - // the German percent convention) from a list cell; that divergence is real, - // is narrower than the one this card closes, and is filed separately rather - // than smuggled in behind a locale fix. - return `${legacyDollar}${body}${isPercent ? '%' : ''}`; + return `${legacyDollar}${body}`; } /** diff --git a/packages/core/src/utils/number-display.ts b/packages/core/src/utils/number-display.ts new file mode 100644 index 000000000..0921cf2d7 --- /dev/null +++ b/packages/core/src/utils/number-display.ts @@ -0,0 +1,205 @@ +/** + * 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. + */ + +/** + * `formatDisplayNumber` — the ONE number-display formatter behind every field + * cell, field widget, metric renderer and dataset measure in the console. + * + * It exists because the same `new Intl.NumberFormat('en-US', …)` construction + * had been copied into the number cell renderer, the currency cell renderer, + * the currency widget, the compact `formatNumber` helper and the dashboard + * metric widget. Two defects therefore had five homes each, and fixing "the" + * renderer never changed the answer (objectui#4033, source thread + * objectstack#5067): + * + * 1. the locale was hardcoded to `en-US`, so a `zh-CN` / `de-DE` console still + * grouped and pointed decimals the US way; and + * 2. `useGrouping` was never set, so a four-digit YEAR stored as + * `Field.number({ scale: 0 })` rendered as `2,026` — in every locale, with + * no field property able to turn it off. + * + * Both policies now live here and nowhere else. Call sites bring the value and + * the display width; they do not bring a locale default and they do not decide + * grouping. + * + * ── Why this module lives in `@object-ui/core` (objectui#4576) ── + * It used to live in `@object-ui/i18n`, and that home cost the repo a real + * defect. `@object-ui/core`'s `utils/dataset-format.ts` could not import it — + * `core` is the React-free engine (AGENTS.md §3: "No UI-lib deps. Logic only.") + * and is a runtime dependency of React-FREE consumers (the `object-ui` VS Code + * extension, `@object-ui/data-objectstack`), while `i18n` depends on + * `i18next`/`react-i18next` and peer-depends on React. So `formatMeasure` kept + * a parallel `Intl` implementation, the two drifted, and a German session read + * `1.234,5 %` from a list cell beside `1.234,5%` from a dashboard measure + * (objectui#4576). #4577 measured the option mapping LOSSLESS across 32,760 + * combinations and recorded the duplication at both ends rather than routing + * through a boundary it could not cross. + * + * The function is pure — no React, no i18next, no DOM — so the boundary was + * never a property of the CODE, only of where the code sat. Moving it DOWN to + * the lower layer removes the obstacle instead of working around it: + * `@object-ui/i18n` now RE-EXPORTS these three names, so every existing import + * path keeps compiling and `import { formatDisplayNumber } from '@object-ui/i18n'` + * and `from '@object-ui/core'` are the SAME symbol (pinned in + * `packages/i18n/src/__tests__/number-display.reexport-identity.test.ts`). + * `core` imports `i18n` nowhere, so the new edge introduces no cycle. + */ + +export interface DisplayNumberFormatOptions { + /** + * BCP-47 tag for the ACTIVE display locale — in React, whatever + * `useDisplayLocale()` returns. + * + * `undefined` means "follow the runtime default", which is the honest answer + * for a non-React caller that has no locale in hand. It never means `en-US`: + * assuming US conventions for the whole world is the defect this module was + * created to remove. + */ + locale?: string; + + /** + * ISO 4217 code. When present the value is formatted as money — which also + * means grouping is kept, because a grouped amount is what every currency + * convention expects and an ordinal amount of money is not a thing. + * + * Money wins over {@link DisplayNumberFormatOptions.style}: a call that sets + * both gets a currency rendering. Nothing in the repo does that, and the + * combination has no meaning to express. + */ + currency?: string; + + /** + * The FIELD's declared `scale` — the `s` of a `decimal(p, s)` column — and + * nothing else. This is a POLICY input, not a display width: pass it only + * when a field declaration actually said so. + * + * `scale: 0` with no currency declares a discrete integer (a year, a fiscal + * period, an ordinal), and those are not grouped. Leave `scale` undefined and + * grouping is kept — which is correct for the two cases that look similar but + * are not: + * + * - an UNDECLARED scale (`scale` is optional in the spec, so absent means + * "decimals unknown", not "integer"); and + * - a caller whose zero-decimal display comes from something other than a + * field declaration — e.g. the dashboard `MetricWidget`, whose decimals + * come from a numeral.js format pattern and whose large KPI aggregates + * are *documented* to want separators ("`1,930,000` not `1930000`"). + * + * ⚠️ INTERIM DEFAULT (objectui#4033, PM ruling 2026-08-11). Suppressing + * grouping for every scale-0 number is a transitional policy with a known, + * accepted cost: a large scale-0 COUNT loses its separators too. It is the + * better trade only until the spec gains an authorable presentation hint + * (`useGrouping` / `displayFormat` — being specified separately, contract-first, + * in the objectstack repo). When that hint lands it OVERRIDES this default, + * and this heuristic should be reduced to the fallback for fields that + * declare nothing. + */ + scale?: number; + + /** + * How the number relates to a percentage — the ONE place that distinction is + * expressed, because getting it wrong is the whole of objectui#4576. + * + * - `'decimal'` (default) — a plain number, no percent sign. + * - `'percent'` — the value is a FRACTION. `Intl` multiplies it by 100 and + * appends the locale's percent convention, so `0.8` renders as `80%`. + * - `'percentPoints'` — the value is ALREADY in percentage points, so `80` + * renders as `80%`. Same locale convention, NO re-scaling. + * + * The third one exists because "divide by 100 so `Intl` can multiply it back" + * is not free, and the round trip is lossy at rounding TIES: measured on this + * repo's runner, `80.175` percentage points formatted to 2 decimals renders + * `80.18%` when formatted directly and `80.17%` after the divide-and-remultiply + * — 27,581 of 1,200,013 ordinary-magnitude en-US forms move, plus every + * value at the top of the double range (`MAX_SAFE_INTEGER`, `1e23`). A caller + * holding percentage points must therefore NOT reach for `'percent'`; that is + * exactly the trap this option removes. + * + * `'percentPoints'` is implemented with `Intl`'s `style: 'unit'` / + * `unit: 'percent'`, which was measured to produce a BYTE-IDENTICAL percent + * affix to `style: 'percent'` across all 171 locale tags tested — including + * the German/French/Russian no-break space before the sign, Turkish's PREFIX + * position (`%1.234,5`), Arabic's own percent sign plus U+061C, and the + * Bengali/Marathi percent patterns that group Western-style where their + * decimal patterns group by lakh. It is the same CONVENTION by a route that + * does not touch the value. + */ + style?: 'decimal' | 'percent' | 'percentPoints'; + minimumFractionDigits?: number; + maximumFractionDigits?: number; + notation?: 'standard' | 'compact'; +} + +/** + * The grouping policy, alone and testable: does this number get thousands + * separators? + * + * @param scale the field's declared `scale`, or `undefined` when the caller + * has no field declaration behind it + * @param currency ISO 4217 code when the number is money + */ +export function shouldGroupDisplayNumber(scale?: number, currency?: string): boolean { + // Money always groups — including money whose currency code could not be + // resolved, which still renders as an amount (just without a symbol). + if (currency) return true; + // Only an explicitly declared scale of 0 is an ordinal. `undefined !== 0`. + return scale !== 0; +} + +/** + * Format a number for DISPLAY, in the active locale, under the grouping policy + * above. + * + * Throws for a bad `currency` code exactly as `Intl.NumberFormat` does, so the + * fallbacks call sites already had (`${currency} ${value.toFixed(n)}`) keep + * working unchanged. A bad LOCALE is handled here instead of throwing: `locale` + * arrives from a server response (ADR-0053 `localization.locale`), and a + * malformed tag from a tenant config must never take a grid cell down. + */ +export function formatDisplayNumber( + value: number, + options: DisplayNumberFormatOptions = {}, +): string { + const { locale, currency, scale, style, ...passthrough } = options; + + const intlOptions: Intl.NumberFormatOptions = { ...passthrough }; + if (style === 'percentPoints') { + // The locale's percent convention WITHOUT `style: 'percent'`'s ×100 — see + // the option's doc above for the measurement that separates the two. + intlOptions.style = 'unit'; + intlOptions.unit = 'percent'; + intlOptions.unitDisplay = 'narrow'; + } else if (style) { + intlOptions.style = style; + } + if (currency) { + intlOptions.style = 'currency'; + intlOptions.currency = currency; + delete intlOptions.unit; + delete intlOptions.unitDisplay; + } + + // ⚠️ Set `useGrouping` ONLY to suppress. `useGrouping: true` is NOT the same + // as omitting the key: `true` means "always", while omitting it means "auto" + // (and "min2" under compact notation), which is the locale's own preference. + // Measured — for 1234: es-ES "auto" → `1234` but "always" → `1.234`; pl-PL + // "auto" → `1234` but "always" → `1 234`. Writing `true` here would silently + // override those locales' conventions in the name of preserving en-US output. + if (!shouldGroupDisplayNumber(scale, currency)) { + intlOptions.useGrouping = false; + } + + try { + return new Intl.NumberFormat(locale, intlOptions).format(value); + } catch { + // Retry WITHOUT the locale, keeping every other option: this rescues a + // malformed tag while still surfacing a genuinely bad `currency` to the + // caller's own catch. + return new Intl.NumberFormat(undefined, intlOptions).format(value); + } +} diff --git a/packages/fields/src/__tests__/percent-cell-vs-measure-4576.test.ts b/packages/fields/src/__tests__/percent-cell-vs-measure-4576.test.ts new file mode 100644 index 000000000..2fa8ad536 --- /dev/null +++ b/packages/fields/src/__tests__/percent-cell-vs-measure-4576.test.ts @@ -0,0 +1,101 @@ +/** + * 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#4576, asserted where the two surfaces can actually be compared. + * + * The card's complaint is a CROSS-PACKAGE one: the same stored number renders + * `1.234,5 %` through `formatPercent` (this package, the list cell) and + * `1.234,5%` through `formatMeasure` (`@object-ui/core`, the dashboard measure). + * Neither package's own suite can see that — `core` sits below this one and + * cannot import `formatPercent` — so the agreement is pinned here, the one + * place both are in scope. + * + * This file adds no implementation. `formatPercent` is untouched by #4576; it + * has rendered through `Intl` since #4553 / PR #4565 and is the SIDE the measure + * formatter moved onto. + * + * ── PREDICTIONS, written before the run ── + * RED before the fix: every locale whose percent convention has a space or a + * different sign — de, fr, es, ru, tr, ar. `formatMeasure` returned a literal + * '%' with no space, `formatPercent` returned the locale's convention. + * GREEN on both sides: en-US, ja-JP, zh-CN, whose convention is a bare + * trailing '%' — the must-not-change half, and the reason this was invisible in + * an English session for three cards running. + * + * Runner measured: node v22.22.2, ICU 78.2, machine locale en-US. + */ + +import { describe, it, expect } from 'vitest'; +import { formatMeasure } from '@object-ui/core'; +import { formatPercent } from '../index'; + +/** + * Strip the digits out of a rendered percentage, leaving only the CONVENTION — + * the sign, its position and any separator before it. Comparing conventions + * rather than whole strings is deliberate: the two formatters are reached with + * different scaling inputs and different decimal widths, and it is the + * convention, not the digits, that #4576 is about. + */ +function convention(rendered: string): string { + return rendered.replace(/[\d]/gu, '#').replace(/#+/gu, '#'); +} + +describe('a percent renders under ONE convention in a cell and in a measure (#4576)', () => { + it.each([ + ['de-DE'], + ['fr-FR'], + ['es-ES'], + ['ru-RU'], + ['tr-TR'], + ['ar-EG'], + // must-not-change half — no space in the convention, identical before and after + ['en-US'], + ['ja-JP'], + ['zh-CN'], + ])('%s renders the same percent convention on both surfaces', (locale) => { + // The same underlying value on both paths: `formatPercent` takes the STORED + // value and scales it through `percentDisplayValue`; `formatMeasure` is + // told the column is already in whole percentage points. + const cell = formatPercent(0.805, 1, locale); + const measure = formatMeasure(80.5, '0.0%', undefined, 'whole', locale); + expect(convention(measure)).toBe(convention(cell)); + }); + + it('the German case the card was filed for, spelled out', () => { + // Both `1.234,5` followed by U+00A0 and the sign. Before #4576 the measure + // had no space. + expect(formatPercent(1234.5, 1, 'de-DE')).toBe('1.234,5\u00a0%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'de-DE')).toBe('1.234,5\u00a0%'); + }); + + it('MUST NOT CHANGE: English was always identical on both paths, and stays so', () => { + expect(formatPercent(1234.5, 1, 'en-US')).toBe('1,234.5%'); + expect(formatMeasure(1234.5, '0.0%', undefined, 'whole', 'en-US')).toBe('1,234.5%'); + }); + + it('NOT a defect pin — the two still round ties differently, and that is #4565 not #4576', () => { + // Honest label rather than a quiet omission, and it records a real + // remaining gap. + // `formatPercent` divides by 100 so `Intl`'s `style: 'percent'` can multiply + // back, and that round trip is lossy at rounding ties; `formatMeasure` now + // formats the percentage points directly and keeps the authored decimal. + // Measured, 2,999 display magnitudes at or above 1 differ this way on a + // 0.005-step grid to 200 (and 27,581 of 1,200,013 forms overall). + // + // 1.005 percentage points to 2 decimals is half-up `1.01`, so the MEASURE is + // the faithful one here and the CELL is the artefact — the opposite of what + // "the cell is the reference" would suggest. It is a NARROWER divergence + // than the convention split this card closed, it predates this card at the + // `formatPercent` end, and closing it means changing `formatPercent`, which + // is outside #4576's surface. Filed separately; pinned here so the next + // reader finds it recorded rather than rediscovers it. + expect(formatPercent(1.005, 2, 'en-US')).toBe('1.00%'); + expect(formatMeasure(1.005, '0.00%', undefined, 'whole', 'en-US')).toBe('1.01%'); + }); +}); diff --git a/packages/i18n/package.json b/packages/i18n/package.json index 08adde976..64efa2bf9 100644 --- a/packages/i18n/package.json +++ b/packages/i18n/package.json @@ -36,6 +36,7 @@ "lint": "eslint ." }, "dependencies": { + "@object-ui/core": "workspace:*", "i18next": "^26.3.6", "react-i18next": "^17.0.11" }, diff --git a/packages/i18n/src/__tests__/number-display.reexport-identity.test.ts b/packages/i18n/src/__tests__/number-display.reexport-identity.test.ts new file mode 100644 index 000000000..fecd6881f --- /dev/null +++ b/packages/i18n/src/__tests__/number-display.reexport-identity.test.ts @@ -0,0 +1,91 @@ +/** + * 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#4576 — the down-move's safety net. + * + * `formatDisplayNumber`, `shouldGroupDisplayNumber` and + * `DisplayNumberFormatOptions` moved from this package into `@object-ui/core`, + * and this package re-exports them so no consumer's import path changes. + * A re-export is only worth anything if it names the SAME thing, so this file + * pins identity rather than behaviour: same function object, same type, from + * all three paths a consumer can reach them by. + * + * ── PREDICTIONS, written before the run ── + * RED before the fix, at MODULE LOAD: `@object-ui/core` exports no + * `formatDisplayNumber`, so the import fails outright — not an assertion + * failure but a resolution one. The behavioural cases below would pass on both + * sides (the implementation is byte-identical; that is the point of a move), + * which is exactly why identity, not behaviour, is what this file asserts. + * + * The three paths, and who uses each one: + * - `@object-ui/core` — the new home; `dataset-format.ts` reads it + * - `../utils/number-display` — the relative path THIS package's own + * `number-display.test.ts` has always used + * - `../index` (`@object-ui/i18n`) — the published entry `fields`, + * `components` and `plugin-dashboard` import + */ + +import { describe, it, expect } from 'vitest'; + +import { + formatDisplayNumber as fromCore, + shouldGroupDisplayNumber as groupFromCore, + type DisplayNumberFormatOptions as OptionsFromCore, +} from '@object-ui/core'; +import { + formatDisplayNumber as fromModule, + shouldGroupDisplayNumber as groupFromModule, + type DisplayNumberFormatOptions as OptionsFromModule, +} from '../utils/number-display'; +import { + formatDisplayNumber as fromEntry, + shouldGroupDisplayNumber as groupFromEntry, + type DisplayNumberFormatOptions as OptionsFromEntry, +} from '../index'; + +describe('the moved number-display symbols are re-exported, not re-implemented (#4576)', () => { + it('all three import paths resolve to the SAME function object', () => { + // Reference equality, not deep equality: a second copy of the + // implementation would pass every behavioural assertion in the repo and + // fail only here. That is the drift this card removed. + expect(fromModule).toBe(fromCore); + expect(fromEntry).toBe(fromCore); + expect(groupFromModule).toBe(groupFromCore); + expect(groupFromEntry).toBe(groupFromCore); + }); + + it('the option type is the same type, not a structural twin', () => { + // Compile-time pin, erased at runtime — `tsc -p tsconfig.test.json` is the + // only thing that checks it, which is why this package's `type-check` + // chains that project. Assignability in BOTH directions is what makes it an + // identity check: two independently declared but structurally identical + // interfaces would also pass a one-way assignment. + const viaCore: OptionsFromCore = { locale: 'de-DE', style: 'percentPoints', scale: 0 }; + const viaModule: OptionsFromModule = viaCore; + const viaEntry: OptionsFromEntry = viaModule; + const backToCore: OptionsFromCore = viaEntry; + expect(backToCore.locale).toBe('de-DE'); + expect(backToCore.style).toBe('percentPoints'); + }); + + it('and the re-exported function still behaves — the entry path formats identically', () => { + // Not a defect pin (green on both sides once the import resolves); it is + // here so a reader can see the re-export is wired to a working function and + // not merely to a name. + const options: OptionsFromEntry = { + locale: 'de-DE', + minimumFractionDigits: 1, + maximumFractionDigits: 1, + }; + expect(fromEntry(1234.5, options)).toBe(fromCore(1234.5, options)); + expect(fromEntry(1234.5, options)).toBe('1.234,5'); + expect(groupFromEntry(0, undefined)).toBe(false); + expect(groupFromEntry(0, 'USD')).toBe(true); + }); +}); diff --git a/packages/i18n/src/utils/number-display.ts b/packages/i18n/src/utils/number-display.ts index 8d37db156..ba5e8a145 100644 --- a/packages/i18n/src/utils/number-display.ts +++ b/packages/i18n/src/utils/number-display.ts @@ -7,148 +7,32 @@ */ /** - * `formatDisplayNumber` — the ONE number-display formatter behind every field - * cell, field widget and metric renderer in the console. + * RE-EXPORT ONLY — the implementation moved to `@object-ui/core` + * (`utils/number-display.ts`) in objectui#4576. Read it there; nothing is + * redeclared here, so there is no second copy to drift. * - * It exists because the same `new Intl.NumberFormat('en-US', …)` construction - * had been copied into the number cell renderer, the currency cell renderer, - * the currency widget, the compact `formatNumber` helper and the dashboard - * metric widget. Two defects therefore had five homes each, and fixing "the" - * renderer never changed the answer (objectui#4033, source thread - * objectstack#5067): + * Why it moved: `@object-ui/core`'s `dataset-format.ts` needs this exact + * policy, and it could not import it while it lived here — `core` is the + * React-free engine and this package depends on `i18next`/`react-i18next` and + * peer-depends on React. So `formatMeasure` kept a parallel `Intl` + * implementation, and the two drifted apart in the one place `Intl` and a + * hand-built string disagree: a German session read `1.234,5 %` from a list + * cell beside `1.234,5%` from a dashboard measure (objectui#4576). The function + * is pure, so the boundary was never a property of the code — only of where the + * code sat. Moving it DOWN removed the obstacle; this package gained a + * dependency on `@object-ui/core` (no cycle: `core` imports nothing from here). * - * 1. the locale was hardcoded to `en-US`, so a `zh-CN` / `de-DE` console still - * grouped and pointed decimals the US way; and - * 2. `useGrouping` was never set, so a four-digit YEAR stored as - * `Field.number({ scale: 0 })` rendered as `2,026` — in every locale, with - * no field property able to turn it off. - * - * Both policies now live here and nowhere else. Call sites bring the value and - * the display width; they do not bring a locale default and they do not decide - * grouping. - * - * ⚠️ ONE KNOWN EXCEPTION, recorded at both ends (objectui#4566). - * `formatMeasure` / `formatDimensionValue` in `@object-ui/core`'s - * `utils/dataset-format.ts` still build their own `Intl.NumberFormat`, and they - * mirror the malformed-locale retry below in a local `formatNumberInLocale`. - * That is NOT drift left unnoticed: the option mapping was measured lossless - * across 32,760 combinations (routing them through this function changes no - * byte of output, because a measure carries no field `scale` and so never - * reaches the grouping policy). What blocks the routing is the package - * boundary — `@object-ui/core` is React-free and consumed by React-free - * packages, while this one depends on `i18next`/`react-i18next` and peer-depends - * on React, and exports no pure-utility subpath. Retiring the duplicate means - * moving THIS module down into `@object-ui/core` and re-exporting it from here. - * Until then, a change to the retry policy below belongs in that mirror too. - */ - -export interface DisplayNumberFormatOptions { - /** - * BCP-47 tag for the ACTIVE display locale — in React, whatever - * `useDisplayLocale()` returns. - * - * `undefined` means "follow the runtime default", which is the honest answer - * for a non-React caller that has no locale in hand. It never means `en-US`: - * assuming US conventions for the whole world is the defect this module was - * created to remove. - */ - locale?: string; - - /** - * ISO 4217 code. When present the value is formatted as money — which also - * means grouping is kept, because a grouped amount is what every currency - * convention expects and an ordinal amount of money is not a thing. - */ - currency?: string; - - /** - * The FIELD's declared `scale` — the `s` of a `decimal(p, s)` column — and - * nothing else. This is a POLICY input, not a display width: pass it only - * when a field declaration actually said so. - * - * `scale: 0` with no currency declares a discrete integer (a year, a fiscal - * period, an ordinal), and those are not grouped. Leave `scale` undefined and - * grouping is kept — which is correct for the two cases that look similar but - * are not: - * - * - an UNDECLARED scale (`scale` is optional in the spec, so absent means - * "decimals unknown", not "integer"); and - * - a caller whose zero-decimal display comes from something other than a - * field declaration — e.g. the dashboard `MetricWidget`, whose decimals - * come from a numeral.js format pattern and whose large KPI aggregates - * are *documented* to want separators ("`1,930,000` not `1930000`"). - * - * ⚠️ INTERIM DEFAULT (objectui#4033, PM ruling 2026-08-11). Suppressing - * grouping for every scale-0 number is a transitional policy with a known, - * accepted cost: a large scale-0 COUNT loses its separators too. It is the - * better trade only until the spec gains an authorable presentation hint - * (`useGrouping` / `displayFormat` — being specified separately, contract-first, - * in the objectstack repo). When that hint lands it OVERRIDES this default, - * and this heuristic should be reduced to the fallback for fields that - * declare nothing. - */ - scale?: number; - - style?: 'decimal' | 'percent'; - minimumFractionDigits?: number; - maximumFractionDigits?: number; - notation?: 'standard' | 'compact'; -} - -/** - * The grouping policy, alone and testable: does this number get thousands - * separators? - * - * @param scale the field's declared `scale`, or `undefined` when the caller - * has no field declaration behind it - * @param currency ISO 4217 code when the number is money - */ -export function shouldGroupDisplayNumber(scale?: number, currency?: string): boolean { - // Money always groups — including money whose currency code could not be - // resolved, which still renders as an amount (just without a symbol). - if (currency) return true; - // Only an explicitly declared scale of 0 is an ordinal. `undefined !== 0`. - return scale !== 0; -} - -/** - * Format a number for DISPLAY, in the active locale, under the grouping policy - * above. - * - * Throws for a bad `currency` code exactly as `Intl.NumberFormat` does, so the - * fallbacks call sites already had (`${currency} ${value.toFixed(n)}`) keep - * working unchanged. A bad LOCALE is handled here instead of throwing: `locale` - * arrives from a server response (ADR-0053 `localization.locale`), and a - * malformed tag from a tenant config must never take a grid cell down. + * This file survives as a re-export rather than being deleted so that BOTH + * historical import paths keep working byte-compatibly — the package entry + * (`@object-ui/i18n`, used by `fields`, `components` and `plugin-dashboard`) + * and the relative `../utils/number-display` this package's own suite uses. + * `number-display.reexport-identity.test.ts` pins that the two paths resolve to + * the SAME function object and the same type, so a future edit cannot quietly + * reintroduce a copy. */ -export function formatDisplayNumber( - value: number, - options: DisplayNumberFormatOptions = {}, -): string { - const { locale, currency, scale, ...passthrough } = options; - - const intlOptions: Intl.NumberFormatOptions = { ...passthrough }; - if (currency) { - intlOptions.style = 'currency'; - intlOptions.currency = currency; - } - - // ⚠️ Set `useGrouping` ONLY to suppress. `useGrouping: true` is NOT the same - // as omitting the key: `true` means "always", while omitting it means "auto" - // (and "min2" under compact notation), which is the locale's own preference. - // Measured — for 1234: es-ES "auto" → `1234` but "always" → `1.234`; pl-PL - // "auto" → `1234` but "always" → `1 234`. Writing `true` here would silently - // override those locales' conventions in the name of preserving en-US output. - if (!shouldGroupDisplayNumber(scale, currency)) { - intlOptions.useGrouping = false; - } - try { - return new Intl.NumberFormat(locale, intlOptions).format(value); - } catch { - // Retry WITHOUT the locale, keeping every other option: this rescues a - // malformed tag while still surfacing a genuinely bad `currency` to the - // caller's own catch. - return new Intl.NumberFormat(undefined, intlOptions).format(value); - } -} +export { + formatDisplayNumber, + shouldGroupDisplayNumber, + type DisplayNumberFormatOptions, +} from '@object-ui/core'; diff --git a/packages/i18n/tsconfig.json b/packages/i18n/tsconfig.json index 64193fa97..0278cf6d3 100644 --- a/packages/i18n/tsconfig.json +++ b/packages/i18n/tsconfig.json @@ -10,5 +10,14 @@ "lib": ["ES2020", "DOM"] }, "include": ["src"], - "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx"] + "exclude": ["src/**/*.test.ts", "src/**/*.test.tsx"], + // `utils/number-display.ts` re-exports `@object-ui/core` (objectui#4576). + // The root tsconfig's `paths` map that specifier at the SOURCE tree + // (`packages/core/src`), which would make those files program inputs under + // this project's `rootDir: "src"` and fail as TS6059. The reference is what + // redirects them to core's built declarations instead — the same shape + // `packages/core/tsconfig.json` already uses for `../types`. + "references": [ + { "path": "../core" } + ] } diff --git a/packages/i18n/tsconfig.test.json b/packages/i18n/tsconfig.test.json index 1c001ee3f..169785163 100644 --- a/packages/i18n/tsconfig.test.json +++ b/packages/i18n/tsconfig.test.json @@ -27,11 +27,17 @@ // Naming `types` at all switches off automatic `@types/*` inclusion, so // both have to be listed here. "types": ["node", "@testing-library/jest-dom"], - // Drop the root tsconfig's source-tree `paths`. This package depends on no - // workspace package, so nothing here resolves through them today — but - // leaving them in would let a future test reach a sibling's `src/` as a - // program input (TS6059) instead of through a declared dependency, which is - // exactly the leak the template exists to close. + // Drop the root tsconfig's source-tree `paths` so `@object-ui/core` + // resolves through the workspace dependency's built `.d.ts` instead of + // pulling `packages/core/src` in as a program input (TS6059) — the leak + // this template exists to close. `type-check` dependsOn `^build` + // (turbo.json), so that `.d.ts` exists by the time this runs. + // + // This package DOES depend on a workspace package as of objectui#4576: + // `utils/number-display.ts` re-exports `formatDisplayNumber` and friends + // from `@object-ui/core`, which is where they now live. Checking the + // re-export identity pin against the built declarations — rather than + // against core's source — is the point: it is the published contract. "paths": {} }, // No `src/**/*.d.ts` entry, unlike plugin-map's project: this package keeps no diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 3dfce7f17..839354c74 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -1293,6 +1293,9 @@ importers: packages/i18n: dependencies: + '@object-ui/core': + specifier: workspace:* + version: link:../core i18next: specifier: ^26.3.6 version: 26.3.6(typescript@6.0.3)