Accept ICU skeletons as an argument style - #90
Conversation
🦋 Changeset detectedLatest commit: 44fb239 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
dependencies
|
Warning Review limit reached
Next review available in: 45 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughThe PR adds ICU skeleton validation, MessageFormat 1-to-2 conversion, locale-aware runtime handlers, skeleton-aware types, tests, and documentation. It also replaces the previous ICU formatter integration with the local compiler. ChangesICU skeleton formatting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SourceMessage
participant MessageFormatParser
participant toMessage
participant functions
participant MessageFormat
SourceMessage->>MessageFormatParser: parse MessageFormat 1 source
MessageFormatParser->>toMessage: provide parsed tokens
toMessage->>MessageFormat: create MessageFormat 2 message
MessageFormat->>functions: resolve formatting handler
functions-->>MessageFormat: return formatted value
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR replaces the upstream MF1 converter with an in-tree conversion pipeline so ICU number and date skeletons can travel as resolved Intl options.
Confidence Score: 4/5The partial-skeleton fallback failure and invalid rounded duration output should be fixed before merging. Partially invalid date skeletons silently retain recognized fields instead of falling back, while duration values near a minute boundary can render an unnormalized 60-second component. Files Needing Attention: packages/integration/src/messageformat/styles.ts, packages/integration/src/messageformat/values.ts Prompt To Fix All With AI### Issue 1
packages/integration/src/messageformat/styles.ts:59-63
**Partial skeleton errors are ignored**
When a date or time skeleton contains both supported and unsupported fields, `getDateTimeFormatOptions` records an error but returns a non-empty option bag, so the unsupported fields are silently discarded instead of activating the documented default-format fallback.
```suggestion
const opt = getDateTimeFormatOptions(tokens, (_type, message) => errors.push(message));
if (errors.length > 0) throw new StyleError(errors[0]!);
// A skeleton that yielded nothing usable is a typo, not a format. Falling
// back to the locale default here would silently show the wrong fields.
if (Object.keys(opt).length === 0) throw new StyleError(`Empty skeleton ${style}`);
return opt;
```
### Issue 2
packages/integration/src/messageformat/values.ts:138-147
**Rounded seconds do not carry**
When fractional seconds round to `60.000`, the duration formatter emits that value unchanged instead of carrying it into the minutes, causing invalid output such as `0:60.000` or `59:60.000`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "Accept ICU skeletons as an argument styl..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const opt = getDateTimeFormatOptions(tokens, (_type, message) => errors.push(message)); | ||
| // A skeleton that yielded nothing usable is a typo, not a format. Falling | ||
| // back to the locale default here would silently show the wrong fields. | ||
| if (Object.keys(opt).length === 0) throw new StyleError(errors[0] ?? `Empty skeleton ${style}`); | ||
| return opt; |
There was a problem hiding this comment.
When a date or time skeleton contains both supported and unsupported fields, getDateTimeFormatOptions records an error but returns a non-empty option bag, so the unsupported fields are silently discarded instead of activating the documented default-format fallback.
| const opt = getDateTimeFormatOptions(tokens, (_type, message) => errors.push(message)); | |
| // A skeleton that yielded nothing usable is a typo, not a format. Falling | |
| // back to the locale default here would silently show the wrong fields. | |
| if (Object.keys(opt).length === 0) throw new StyleError(errors[0] ?? `Empty skeleton ${style}`); | |
| return opt; | |
| const opt = getDateTimeFormatOptions(tokens, (_type, message) => errors.push(message)); | |
| if (errors.length > 0) throw new StyleError(errors[0]!); | |
| // A skeleton that yielded nothing usable is a typo, not a format. Falling | |
| // back to the locale default here would silently show the wrong fields. | |
| if (Object.keys(opt).length === 0) throw new StyleError(`Empty skeleton ${style}`); | |
| return opt; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/integration/src/messageformat/styles.ts
Line: 59-63
Comment:
**Partial skeleton errors are ignored**
When a date or time skeleton contains both supported and unsupported fields, `getDateTimeFormatOptions` records an error but returns a non-empty option bag, so the unsupported fields are silently discarded instead of activating the documented default-format fallback.
```suggestion
const opt = getDateTimeFormatOptions(tokens, (_type, message) => errors.push(message));
if (errors.length > 0) throw new StyleError(errors[0]!);
// A skeleton that yielded nothing usable is a typo, not a format. Falling
// back to the locale default here would silently show the wrong fields.
if (Object.keys(opt).length === 0) throw new StyleError(`Empty skeleton ${style}`);
return opt;
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Confirmed and fixed in 44fb239.
Verified it directly: ::yMMMdqqqq records The field q is not supported by Intl.DateTimeFormat and still returns {year, month, day}, so the quarter was being dropped in silence. Same for a literal part, ::yMMMd'x'.
Taking it for a second reason as well: validateArgumentStyle in @saykit/config already rejected on any error, so the build and the runtime disagreed about what a valid skeleton was. A hand-edited catalogue never passes through the build check, and the runtime leniency was the only guard there.
Now a skeleton fails whole, which routes it to the documented default-format fallback. I kept the empty-skeleton case as a separate check so :: still reports something readable rather than an empty message, and added tests both here and in the config suite so the two stay in step.
| const secs = value % 60; | ||
| const parts: (string | number)[] = [Math.round(secs) === secs ? secs : secs.toFixed(3)]; | ||
| if (value < 60) { | ||
| // One `:` is always written, so a sub-minute duration still reads as a | ||
| // duration rather than as a bare number of seconds. | ||
| parts.unshift(0); | ||
| } else { | ||
| value = Math.round((value - Number(parts[0])) / 60); | ||
| parts.unshift(value % 60); | ||
| if (value >= 60) parts.unshift(Math.round((value - Number(parts[0])) / 60)); |
There was a problem hiding this comment.
When fractional seconds round to 60.000, the duration formatter emits that value unchanged instead of carrying it into the minutes, causing invalid output such as 0:60.000 or 59:60.000.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/integration/src/messageformat/values.ts
Line: 138-147
Comment:
**Rounded seconds do not carry**
When fractional seconds round to `60.000`, the duration formatter emits that value unchanged instead of carrying it into the minutes, causing invalid output such as `0:60.000` or `59:60.000`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Confirmed and fixed in 44fb239.
Reproduced all three cases you predicted:
59.9999 -> 0:60.000
119.9999 -> 1:60.000
3599.9999 -> 59:60.000
The cause was rounding the seconds field in place, after the value had already been split, so a field that rounded up to a whole minute had nowhere to carry to. (Inherited from upstream, which has the same defect.)
Fixed by rounding to the millisecond before splitting, then decomposing from the rounded total, which also let the minute/hour arithmetic collapse into plain Math.floor rather than the running subtraction it was doing. 59.9999 now gives 1:00, 3599.9999 gives 1:00:00, and 59.5 still gives 0:59.500. Tests added for the carry cases.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
packages/integration/src/messageformat/convert.ts (1)
200-206: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake the key comparator consistent.
The comparator is asymmetric. For two exact numbers,
compare(a, b)andcompare(b, a)both return-1, because the first condition matches in each direction.Array.prototype.sortrequirescompare(a, b)andcompare(b, a)to have opposite signs.The current output is still correct, because exact keys never compete for the same value, and the number-before-category and
other-last pairs both resolve correctly. The risk is future change: any rule that depends on the order among exact keys will read an arbitrary order.A rank function makes the intended order explicit and total.
♻️ Proposed refactor
function sortKeys(keys: (string | number)[]) { - return Array.from(new Set(keys)).sort((a, b) => { - if (typeof a === 'number' || b === 'other') return -1; - if (typeof b === 'number' || a === 'other') return 1; - return 0; - }); + // Exact numbers first, then CLDR categories, then `other`. + const rank = (key: string | number) => (typeof key === 'number' ? 0 : key === 'other' ? 2 : 1); + return Array.from(new Set(keys)).sort((a, b) => rank(a) - rank(b)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integration/src/messageformat/convert.ts` around lines 200 - 206, Update sortKeys by introducing a rank-based ordering for each key category, then compare the ranks and return 0 for keys with equal rank. Preserve the intended order of numeric keys before category keys and “other” keys last, while ensuring exact-key comparisons are antisymmetric and deterministic.packages/integration/src/messageformat/functions.ts (1)
53-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBuild
Intl.PluralRulesonce per placeholder, not once per selection.
selectKeyconstructs a newIntl.PluralRuleson every call. A plural selector runs on every format of the message, so this repeats for each call. The inputs are fixed for the placeholder:ctx.locales,ctx.localeMatcher, andordinal.
values.tsalready memoises itsIntlformatters, and states that constructing one is the expensive half of formatting. Apply the same treatment here.♻️ Proposed refactor
'say:plural': (ctx, opt, operand) => { const { offset = 0, ordinal = false } = options<PluralOptions>(opt.options); const value = numeric(operand); const shifted = typeof value === 'bigint' ? value - BigInt(offset) : value - offset; const result = number(ctx.locales as string[], shifted, {}); + let rules: Intl.PluralRules | undefined; // The offset number is what `#` prints, but the original is what the value // *is* — so a second selector reading this one offsets from the number the // message was given rather than from an already-shifted one. result.valueOf = () => value; result.selectKey = (keys) => { const exact = String(value); if (keys.has(exact)) return exact; // `Intl.PluralRules` takes a number, never a bigint. - const category = new Intl.PluralRules(ctx.locales as string[], { - localeMatcher: ctx.localeMatcher, - type: ordinal ? 'ordinal' : 'cardinal', - }).select(Number(shifted)); + rules ??= new Intl.PluralRules(ctx.locales as string[], { + localeMatcher: ctx.localeMatcher, + type: ordinal ? 'ordinal' : 'cardinal', + }); + const category = rules.select(Number(shifted)); return keys.has(category) ? category : null; }; return result; },The handler itself still runs per format call, so this removes the repeat within a single format rather than across formats. To cache across formats, key the rules by locale and
ordinalin a module-levelMap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integration/src/messageformat/functions.ts` around lines 53 - 62, Move Intl.PluralRules construction out of the per-call selectKey handler and create one instance when the placeholder setup runs, reusing it inside result.selectKey with the fixed ctx.locales, ctx.localeMatcher, and ordinal options. Preserve the existing plural selection and bigint-to-number conversion, and do not add module-level caching unless the surrounding implementation already provides an appropriate cache.packages/integration/package.json (1)
44-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the prerelease skeleton dependency decision.
@messageformat/date-skeletonand@messageformat/number-skeletonare pinned to thenextprerelease2.0.0-0, but no stable2.xis available. If this prerelease API is intentional for the published integration package, add a short// or@deprecatedreason/ comment explaining why stable support is not used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/integration/package.json` around lines 44 - 47, Document the intentional prerelease choice beside the `@messageformat/date-skeleton` and `@messageformat/number-skeleton` dependencies in package.json, explaining that stable 2.x support is unavailable and the 2.0.0-0 API is required for the published integration package.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/integration/src/messageformat/index.test.ts`:
- Around line 47-51: Rename the test case around numeric(undefined) so its
description states that a missing number produces NaN rather than zero or an
error. Keep the existing assertion and implementation unchanged.
In `@packages/integration/src/messageformat/styles.ts`:
- Around line 66-69: In the named-style lookup within the date/time style
handling, replace the inherited-property check using `style in named` with an
own-property check such as `Object.hasOwn`. Preserve the existing empty-style
`named.medium` fallback and `StyleError` behavior for unsupported styles,
limiting successful lookups to the catalogue’s four named styles.
In `@website/content/core-concepts/messages.mdx`:
- Around line 436-439: Replace the uncopyable single-space inline example in the
whitespace documentation with a compilable JSX expression such as the supported
preserved-space form, and update the conflicting guidance in the React
integration documentation to match the actual extraction behavior for literal
string expressions.
- Around line 301-315: The Skeletons section should distinguish three cases: use
a genuinely unsupported skeleton to demonstrate build-time resolution failure,
use “meduim” for malformed style syntax, and use a dynamic style value to
demonstrate runtime fallback. Update the surrounding explanation and examples
accordingly, making clear that “qqqq” is valid ICU skeleton syntax even though
“::qqqq” cannot be resolved.
In `@website/content/reference/api/saykit.mdx`:
- Around line 215-221: Update the documentation surrounding the say.plural
example to state that single-key object wrappers name and interpolate their
contained value, while object variables and inline objects with multiple keys,
spreads, or computed keys retain value-based behavior; clarify that other object
expressions do not create named placeholders.
---
Nitpick comments:
In `@packages/integration/package.json`:
- Around line 44-47: Document the intentional prerelease choice beside the
`@messageformat/date-skeleton` and `@messageformat/number-skeleton` dependencies in
package.json, explaining that stable 2.x support is unavailable and the 2.0.0-0
API is required for the published integration package.
In `@packages/integration/src/messageformat/convert.ts`:
- Around line 200-206: Update sortKeys by introducing a rank-based ordering for
each key category, then compare the ranks and return 0 for keys with equal rank.
Preserve the intended order of numeric keys before category keys and “other”
keys last, while ensuring exact-key comparisons are antisymmetric and
deterministic.
In `@packages/integration/src/messageformat/functions.ts`:
- Around line 53-62: Move Intl.PluralRules construction out of the per-call
selectKey handler and create one instance when the placeholder setup runs,
reusing it inside result.selectKey with the fixed ctx.locales,
ctx.localeMatcher, and ordinal options. Preserve the existing plural selection
and bigint-to-number conversion, and do not add module-level caching unless the
surrounding implementation already provides an appropriate cache.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: db622c0d-e776-4a52-9357-d43561024e52
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
.changeset/tidy-pugs-tickle.mdpackages/config/package.jsonpackages/config/src/features/messages/format.test.tspackages/config/src/features/messages/format.tspackages/integration-react/src/runtime/index.tspackages/integration/package.jsonpackages/integration/src/messageformat/convert.tspackages/integration/src/messageformat/functions.tspackages/integration/src/messageformat/index.test.tspackages/integration/src/messageformat/index.tspackages/integration/src/messageformat/options.tspackages/integration/src/messageformat/styles.tspackages/integration/src/messageformat/values.tspackages/integration/src/runtime.test.tspackages/integration/src/runtime.tspackages/integration/src/types.tspackages/transform-js/src/parser.test.tswebsite/content/core-concepts/messages.mdxwebsite/content/integrations/react.mdxwebsite/content/reference/api/saykit.mdx
|
Thanks both — pushed 44fb239. Fixed (both P1s were real, and I reproduced each before changing anything):
Skipped, with reasons in the threads: the single-key-object boundary in One extra: chasing the whitespace comment turned up a genuine contradiction between the two docs pages. The JSX parser reads a literal string child as the text it renders as — there is a passing test on 638 tests pass, |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ You are interacting with an AI system. |
|
All six fixes verified against the diff — each one is exactly right:
The test coverage for the duration carry cases ( This is in good shape. Happy to approve. |
@saykit/config
@saykit/format-json
@saykit/format-po
saykit
@saykit/carbon
@saykit/react
babel-plugin-saykit
unplugin-saykit
@saykit/transform-js
@saykit/transform-jsx
commit: |
|
The preview build of |
github-actions
Bot
removed
the
preview
1 participant
@messageformat/icu-messageformat-1renders an argument's style into MF2's own option vocabulary, and that vocabulary tops out at{ length, fields }for a date.{d, date, ::yyyyMMdd}has nowhere to land, so it survives only as an unrecognisedmf1:argStylethat the formatter reports as a bad option.This replaces that package with an in-tree conversion so a style can skip the vocabulary. Both skeleton parsers already emit
Intloption bags, so a style now resolves to one when the message is compiled, rides through the message data whole, and goes straight toIntlat format time —::yyyyMMddand::currency/EURarrive by the same route asshortandinteger, and cost the same.Layout
packages/integration/src/messageformat/, split along the pipeline:styles.tsIntloptions — the part upstream cannot dooptions.tsconvert.tsvalues.ts/functions.tsValidated at build time
validateArgumentStylenow accepts skeletons and checks them by resolving them with the same parsers the runtime formats with, so::qqqqfails with a file and a line rather than reaching a reader. An unresolvable style still falls back to the bare formatter at runtime — a message authored once and read by everyone is better slightly wrong than broken.Two bugs fixed along the way
Both found while covering the new folder:
toPartsdroppeddir, losing bidi information forformatToPartsconsumers.duplicate-declaration, since both selectors declared the same name. Confirmed upstream throws identically, so not a regression — later selectors now get a generated name and alocaldeclaration, and a plural reports the number the message was given rather than the already-offset one.Notes
::percentwrites the sign but does not scale (0.25→0.25%). The namedpercentstyle scales too, which is::percent scale/100. That is ICU's semantics; both are tested.packages/integration/src/messageformatis at 100% statements, branches, and functions. Three genuinely unreachable guards carry/* v8 ignore */with the reason rather than a contorted test.630 tests pass; build, lint, and typecheck clean across all packages.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes