Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:design-review
|
Test count for the body: |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested: the slicing itself looks sound, since strings resolve the same with and without InternationalizationProvider and the catalog check and test suites pass. The one blocker is compatibility: in the latest release a value rendered straight into the public InternationalizationContext drives useLocale, useDirection, useCollator and every core component's strings, and with this change it reaches none of them, so it is a breaking change for those apps rather than an undocumented path. Please either declare it in a [breaking] Changeset that names the old usage and the migration to InternationalizationProvider (main currently plans a minor, so it is admissible) or keep that path working, and drop the wording that calls InternationalizationContext unchanged.
- Minor: the i18n docs page now says an app bundles only the strings of the components it renders, but apps that mount the provider or call
useTranslator()still carry the whole catalog, so please qualify that sentence.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [chore] The i18n runtime can hand a component its own English strings. The English catalog is also emitted one namespace at a time, the resolver takes its English fallback per call, and an internal `useComponentTranslator(slice)` lets a core component render with its own slice; the components themselves move to it in a follow-up, so nothing changes for an app yet. `useTranslator()` and `InternationalizationContext` are unchanged and still resolve every `@astryx.*` key; an `InternationalizationProvider`'s `messages` and `overrides` still win; the slices themselves are not importable. One undocumented path moves: a value handed straight to `InternationalizationContext` without the provider no longer reaches `useLocale`, `useDirection` and `useCollator`, which read the provider's runtime context; mount `InternationalizationProvider`. |
There was a problem hiding this comment.
This is a breaking change to a released public surface rather than an undocumented path: InternationalizationContext is exported from @astryxdesign/core/i18n, and in 0.6.8 a value rendered straight into it drives useLocale, useDirection, useCollator and every core component's strings, while at this head it reaches none of them. Please move it into a [breaking] Changeset (minor) that names the old usage, the new behavior and the migration to InternationalizationProvider, or keep that path working. The “nothing changes for an app yet” sentence also stops being true once both halves ship together.
There was a problem hiding this comment.
Declared [breaking] in #7357 (305efa8, the PR that moves useLocale/useDirection/useCollator to the runtime context): the changeset names the old usage — a value rendered straight into InternationalizationContext without the provider — and the migration, mount InternationalizationProvider with the same locale, messages and overrides. Keeping the path alive would put the whole catalog back into every component's bundle, which is what the change removes. The "unchanged" wording is gone from both bodies and both changesets (9f3e2d4 here).
|
|
||
| export function useLocale(): Locale { | ||
| const ctx = use(InternationalizationContext); | ||
| const ctx = use(TranslationRuntimeContext); |
There was a problem hiding this comment.
Reading only TranslationRuntimeContext here (and in useDirection, useCollator and useComponentTranslator) is what stops a value handed straight to the public InternationalizationContext from reaching these hooks. If that released path should keep working, this is where a fallback would live; otherwise the change needs the breaking declaration noted on the changeset.
There was a problem hiding this comment.
The fallback would have to be the public context's whole-catalog translate, which puts the catalog back into every component that reads it — so the path retires instead, declared [breaking] in #7357 with the migration to the provider.
| { | ||
| type: 'prose', | ||
| text: 'Astryx component authors read strings with `useTranslator()` rather than hardcoding user-facing text.', | ||
| text: "Astryx components read their own strings from the English catalog rather than hardcoding them, so an app bundles only the strings of the components it renders. Your own code reads any `@astryx.*` key with `useTranslator()`:", |
There was a problem hiding this comment.
This holds only for apps that neither mount InternationalizationProvider nor call useTranslator(); both still carry the whole catalog, and the next block teaches useTranslator(). Suggest qualifying the sentence.
There was a problem hiding this comment.
Qualified in 9f3e2d4: "An app that neither mounts InternationalizationProvider nor calls useTranslator() bundles only the strings of the components it renders; one that does carries the whole catalog, each string once." (The sentence lives in this PR, where it becomes true; #7357's copy of the guide makes no bundle claim.)
9f3e2d4 to
59f0ee1
Compare
PR Analysis Report
Modified ComponentsAlertDialog (@astryxdesign/core)
AppShell (@astryxdesign/core)
Banner (@astryxdesign/core)
Button (@astryxdesign/core)
Calendar (@astryxdesign/core)
Carousel (@astryxdesign/core)
CheckboxInput (@astryxdesign/core)
Citation (@astryxdesign/core)
CodeBlock (@astryxdesign/core)
ComplexSelector (@astryxdesign/core)
ContextMenu (@astryxdesign/core)
DateInput (@astryxdesign/core)
DateRangeInput (@astryxdesign/core)
DateTimeInput (@astryxdesign/core)
FileInput (@astryxdesign/core)
Kbd (@astryxdesign/core)
Lightbox (@astryxdesign/core)
Markdown (@astryxdesign/core)
MoreMenu (@astryxdesign/core)
MultiSelector (@astryxdesign/core)
NumberInput (@astryxdesign/core)
Outline (@astryxdesign/core)
Pagination (@astryxdesign/core)
Popover (@astryxdesign/core)
PowerSearch (@astryxdesign/core)
Spinner (@astryxdesign/core)
Switch (@astryxdesign/core)
TextArea (@astryxdesign/core)
TextInput (@astryxdesign/core)
Thumbnail (@astryxdesign/core)
TimeInput (@astryxdesign/core)
Timestamp (@astryxdesign/core)
Toast (@astryxdesign/core)
Token (@astryxdesign/core)
Tokenizer (@astryxdesign/core)
TreeList (@astryxdesign/core)
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression57 of 504 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 37 more. Generated by PR Enrichment workflow | View full report |
There was a problem hiding this comment.
This change is on hold for a maintainer decision: approval needs a maintainer to confirm, or record in the governing spec, that InternationalizationContext stops configuring Astryx in favor of InternationalizationProvider, since no current spec decides that. The slicing itself checks out: strings resolve the same with and without the provider, and a Button bundle drops about 5 KB gzip. Independently, please finish the [breaking] Changeset's migration: say how an app that supplied its own translate moves its strings (the provider has no translate input), map the context's direction to the provider's dir, and add an astryx upgrade codemod or note why one would be vacuous. Please also drop its "bundles do not change yet" clause, which contradicts the sweep's Changeset once both ship in 0.7.0.
[Automated review]
| '@astryxdesign/core': minor | ||
| --- | ||
|
|
||
| [breaking] An app that sets `InternationalizationContext` directly — rendering `<InternationalizationContext value={…}>` (or its `.Provider`) instead of `InternationalizationProvider` — no longer drives core components' strings or `useLocale`, `useDirection` and `useCollator`. Migration: mount `InternationalizationProvider`; it takes the same `locale`, `messages`, `overrides` and `dir`. The provider now writes an internal runtime context that those hooks and every core component read. Reading the public context is unchanged, so a component that reads it with `useContext` and re-mounts `InternationalizationProvider` with a derived value keeps working: `locale`, `direction`, `messages`, `overrides` and `translate`, which still resolves every `@astryx.*` key against the whole shipped English catalog, with or without a provider; `useTranslator()` is unchanged. Groundwork for components carrying only their own English strings: the English catalog is also emitted one namespace at a time, the resolver takes its English fallback per call, and an internal `useComponentTranslator(slice)` lets a core component render with its own slice; the components themselves move to it in a follow-up, so bundles do not change yet. The slices are not importable. |
There was a problem hiding this comment.
Two things in this entry:
- The migration covers
locale,messagesandoverrides, but the released context value type also requires atranslate, and when an app supplies its own, that translator produced every core component's strings in 0.6.8.InternationalizationProviderhas notranslateinput, so please say how such an app moves its strings (intomessagesoroverrides), map the context'sdirectionto the provider'sdir, and either stage anastryx upgradecodemod or note why one would be vacuous (AST-017 FR8). - "the components themselves move to it in a follow-up, so bundles do not change yet" contradicts
.changeset/i18n-components-own-slices.mdonce both ship in 0.7.0; please drop that clause.
There was a problem hiding this comment.
Both points are covered in the [breaking] entry at this head (8a08a54). The provider has no translate input, so an app that supplied its own expresses those strings as messages (a catalog per locale) or overrides (sparse, per locale), resolved in the documented order. The provider's dir takes the value's direction.
59f0ee1 to
cc58ddd
Compare
|
On the migration asks, done in #7357 (c9fbefe, which this branch is rebased on; head cc58ddd here):
The hold itself — a maintainer confirming that |
cc58ddd to
b7aa933
Compare
0bb51c4 to
ea56d9a
Compare
The upgrade transform, its fixtures and its changeset leave this PR so the runtime half stays within the review size; the breaking entry points at the stacked PR that carries them. CONTRIBUTING's bundle sentence names PowerSearch, whose caller-supplied operator keys keep the whole catalog.
… will The core changelog takes this entry verbatim, so it names the `astryx upgrade` transform and what it rewrites rather than pointing at the pull request that ships it. CONTRIBUTING's PowerSearch clause moves to the component sweep, where the hook it names exists.
…stays the package's own `astryxStylex()` aliased `@astryxdesign/core/locales/(.+).generated.js` straight to the source tree, which let a Vite app resolve the per-namespace English slices the exports map keeps out of reach with a `null` target. The capture now takes a single segment, the whole-catalog case; a slice specifier under `locales/en/` resolves to nothing the package ships. Test for the slice specifier beside the existing catalog cases.
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to accept that removal as the contract's compatibility decision, or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec 8a08a5472142451792ff8a1816681a9e9692b33d on this pull request, and the author may do so as an engineering owner. Independently of that decision:
check:changesetsfails while main declares 0.7.5, so the release owner needs to declare 0.8.0 on main before the[breaking]entry can merge.- Please land #7357 and #7969 first and rebase, so that this diff is only the component sweep.
- The
[perf]Changeset says an app with the provider "moves by under 1 KB either way", but the size table in the description shows every listed page growing 1.2 to 1.9 KB gzip with the provider mounted, so please state the measured growth.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [perf] A component carries only its own English strings. Every core component now renders with its own namespace slice of the English catalog instead of the whole catalog, so an app that renders components without mounting `InternationalizationProvider`, calling `useTranslator()` or importing `InternationalizationContext` (each of which carries the whole catalog), and that does not render `PowerSearch` (whose caller-supplied operator keys keep the whole catalog as their fallback), bundles only the strings of the components it renders: two rows of buttons and links drop about 5 KB (gzip) from their first load. An app that does mount the provider still carries the whole catalog, now assembled from the same slices so no string ships twice; it moves by under 1 KB either way (a few slice modules cost a little, many save a little). `useTranslator()` is unchanged. `astryx swizzle` points an ejected component's strings at the public `useTranslator()`: the internal translator import, the slice imports and a module-level merge of slices are rewritten, so the copy compiles in the app. |
There was a problem hiding this comment.
This says an app that mounts the provider "moves by under 1 KB either way", but the size table in the description shows every listed page growing with the provider mounted (Button 41,345 to 42,660 B, DateInput 92,003 to 93,857 B gzip, about 0.9 KB only when the provider is imported first). Please state the measured growth, or drop the figure.
There was a problem hiding this comment.
Fixed in 144793f. The entry now gives the measured sizes. Without the provider, a page importing Button, Dialog, DateInput or List loads 6.2 to 6.8 KB (gzip) less. With it mounted, the page loads 1.2 to 1.9 KB more, because the catalog spread over 83 slice modules compresses worse than one module; the growth is about 0.9 KB when the provider is imported before the components. It also drops the old "about 5 KB" figure from the October 11 base.
| '@astryxdesign/core': minor | ||
| --- | ||
|
|
||
| [breaking] An app that sets `InternationalizationContext` directly — rendering `<InternationalizationContext value={…}>` (or its `.Provider`) instead of `InternationalizationProvider` — no longer configures Astryx: it drives neither `useLocale`, `useDirection` and `useCollator`, nor the components that read the locale or direction through them (the date, date-time, date-range and number inputs, Calendar, Chat, PowerSearch, Table sorting and filtering, Timestamp), nor any core component's strings — every component now renders with its own English slice through an internal translator, and `DateTimeInput`'s touch field reads `useLocale()`. One deliberate exception: a `PowerSearch` operator `i18nKey` from outside PowerSearch's own namespace (a key of another Astryx component, or of the app's catalog) still resolves through the public context's `translate`, since a caller may supply any key there; the shipped default operators are PowerSearch's own and read the slice like the rest of the component. Migration: mount `InternationalizationProvider`; its `locale`, `messages` and `overrides` take the value's fields of the same name, and its `dir` takes the value's `direction`. The provider has no `translate` input: an app that supplied its own `translate` expresses those strings as `messages` (a catalog per locale) or `overrides` (sparse, per locale) and the provider resolves them in the documented order. Run `astryx upgrade`: its `migrate-internationalization-context` transform rewrites a variable value to the provider's `locale`, `dir` and `messages` with a TODO for its `overrides` and `translate`, and an inline object literal key by key with a TODO for `translate`; any other value, and any element that spreads props, is left as written behind a TODO saying why. The provider now writes an internal runtime context that those hooks read. Reading the public context is unchanged, so a component that reads it with `useContext` and re-mounts `InternationalizationProvider` with a derived value keeps working: `locale`, `direction`, `messages`, `overrides` and `translate`, which still resolves every `@astryx.*` key against the whole shipped English catalog, with or without a provider; `useTranslator()` is unchanged. The English catalog is also emitted one namespace at a time, the resolver takes its English fallback per call, and an internal `useComponentTranslator(slice)` is what a core component renders with. The slices are not importable. |
There was a problem hiding this comment.
check:changesets refuses this entry while main declares 0.7.5 (AST-017 FR31). It can merge once an engineering owner accepts the raw-context retirement and the release owner declares 0.8.0 on main (FR48).
There was a problem hiding this comment.
Agreed, there is nothing to change in the entry itself. It stays refused until an engineering owner rules on the raw-context retirement and the release owner declares 0.8.0 on main. The description's Authority section names both decisions. Checked at 144793f.
…erence The docsite's server routes trace this package's files, and the test ran its resolver from apps/sandbox, a path the tracer follows: the search route then shipped the whole sandbox app (190.3 MB, over the docsite's 150 MB function budget), so every preview deploy of this branch failed. The package's self-reference applies the same exports map a consumer's import does, so the test now resolves from the package's own directory; the route is back to 59.0 MB (main: 58.9 MB), and the test still fails when the ./locales/en/* null export is removed.
…vider
`astryx upgrade` runs `migrate-internationalization-context`, the codemod
for the retirement that the stacked runtime change declares: a
`<InternationalizationContext value={v}>` (or `.Provider`) whose value is a
variable maps `locale`, `direction` (as `dir`) and `messages` to
`<InternationalizationProvider>` with a TODO for `overrides` and
`translate`; an inline object literal maps key by key with a TODO for
`translate`; any other value — a call, a conditional, a literal with a
spread, an empty literal — and any element that spreads props stay as
written behind a TODO that says why (a spread after `value` may override
it; a spread anywhere may carry `dir`, `messages` or `overrides`, which the
context ignored but the provider would read). Only an element whose name
resolves, unshadowed, to the Astryx import is touched: blocks, loop heads,
catch clauses, parameters (private methods too), hoisted `var`s, named
class and function expressions, `static {}` blocks, namespace bodies
(including `export`, `import X =`, `declare function`, `enum`, nested and
dotted namespaces) all shadow it; a type-only import proves nothing. The
provider is imported under a free alias when the module already binds its
name, and an emptied import goes with its comments carried forward and
the directive left intact. A second run changes nothing.
Fixtures cover each shape; one typechecks the rewritten output with tsc
against the real i18n source types; one is the read-and-re-mount shape every
real consumer found by a code search has (the transform leaves it alone).
…ir own English slice
Mechanical sweep over every core component that called useTranslator():
- import {useTranslator} from '../i18n';
+ import {useComponentTranslator} from '../i18n/useComponentTranslator';
+ import buttonMessages from '../i18n/generated-locales/en/button.generated';
- const t = useTranslator();
+ const t = useComponentTranslator(buttonMessages);
A file that reads several namespaces merges their slices once at module
level (`const messages = {...dateInputMessages, ...timeInputMessages}`) and
passes that. Two PowerSearch files hand `t` to helpers and carry the helpers'
namespace. No call site, key or string changes; check:i18n-catalog verifies
that every slice covers the keys its file reads.
…re files render with their own With every component on its slice, an app that mounts InternationalizationProvider or calls useTranslator() carried the whole catalog through the public context beside the slices, so each rendered component's strings were in the bundle twice. en.generated.ts is now the spread of the 78 slice modules, so a bundle that reaches both carries every string once; a pinned test checks the assembled catalog against the authored keys and messages, and the compiled Button imports its slice and not the whole catalog. check:i18n-catalog gains the slice-coverage rule, here with its first callers: every @AstryX.<ns>.* key a slice-rendering file reads must belong to a namespace its slice carries, a file handed t is covered by its component directory, and an import of useTranslator or InternationalizationContext inside packages/core/src is refused: both carry the whole catalog, and a component renders with useComponentTranslator(<its slice>) and reads the locale through useLocale/useDirection.
…seTranslator() `astryx swizzle` rewrote `../i18n/useComponentTranslator` and the English slice imports to `@astryxdesign/core/i18n`, which exports neither, so every swept component's ejected copy failed to compile (TS2305, TS1192). The copy step now points the copy at the app's public translator: the hook import becomes `useTranslator` (a type-only `TranslatorFn` rides along), the slice imports and a module-level merge of slices go, and each call becomes `useTranslator()`. Tests: a fixture with a merged slice set, and Button as swept — nothing its rewritten i18n imports name is internal. Verified by ejecting Spinner, Button and DateRangeInput into the sandbox app: no i18n error from tsc. The internal useCatalogTranslator (caller-supplied keys, PowerSearch) is the public useTranslator under another name, so its import becomes `useTranslator as useCatalogTranslator`, keeping both call sites. The barrel test now covers every core component directory and resolves an `X as Y` import to X; without the alias it fails on PowerSearch. Ejected PowerSearch into the sandbox app: four aliased imports, no i18n error from tsc.
…allback PowerSearch's `operator.i18nKey` is public and unrestricted — any `@astryx.*` key, or one from the app's own catalog — so resolving it through the PowerSearch slice lost the shipped fallback a caller had: with no provider or a partial catalog, `@astryx.table.filter.apply` came back as the raw key instead of "Apply". A new internal `useCatalogTranslator()` reads the public context's whole-catalog `translate`; the token, the editor popover, the search source and the search surface pass it to `resolveOperatorLabel`, while their own fixed strings keep reading the PowerSearch slice. The slice-coverage check counts the files that import it so the carve-out stays visible (4 today, all PowerSearch), and a type-only import clause no longer registers as a value import. Tests: the token and the whole search → token → editor path with an out-of-namespace shipped key and no provider; the compiled hook imports the public context and Button does not import it. A key in PowerSearch’s own namespace (every shipped default operator) still resolves through the slice like the rest of the component’s strings, so under a raw context value the search box and a shipped operator read one source; only a key from another namespace takes the whole-catalog path. resolveOperatorLabel takes both translators and routes by namespace.
A page importing Button, Dialog, DateInput or List loads 6.2 to 6.8 KB (gzip) less without the provider, and 1.2 to 1.9 KB more with it mounted, because the catalog spread over 83 slice modules compresses worse than one module (about 0.9 KB when the provider is imported first). The entry said "under 1 KB either way" and "about 5 KB", from the October 11 base.
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to accept that removal and record it in the provider contract as its compatibility decision, or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec d576f18e404cba283670572c47c5baf876ff409e on this pull request, and the author may do so as an engineering owner. Independently of that decision:
check:changesetsfails while main declares 0.7.5, so the release owner needs to declare 0.8.0 on main before the[breaking]entry can merge.- Please land #7357 and #7969 first and rebase, so that this diff is only the component sweep.
[Automated review]
# Conflicts: # packages/cli/assets/codemods/transforms/next/index.mjs
# Conflicts: # packages/core/src/Markdown/Markdown.tsx
Main's Popover presentation (a bottom sheet on a phone) added a close label read through useTranslator(), which carries the whole English catalog, so check:i18n-catalog failed at Popover.tsx after main was merged in. Popover now renders with useComponentTranslator(popoverMessages), like every other core component in this sweep. Red first: check:i18n-catalog (1 slice coverage problem at Popover/Popover.tsx:67 before, none now).
Main's CommandPaletteEmpty audit (9600039) added a doc that matches "search" (112 to 113), and this sweep already added one: the merge kept 113 on both sides, so "search search, text and JSON" read 114 against a golden of 113. Regenerated in the branch, as the test's header says once the icon-set feature is on main: only the count changes (JSON matchCount and the text header), and the diff names no experimental surface. Red first: experimental-isolation's "search search" case (expected 113, received 114 before; 34 of 34 pass now).
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to accept that removal and record it in the provider contract as its compatibility and migration decision, or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec fa9550df7468cfb5e1f568c503888f5d960dba71 on this pull request, and the author may do so as an engineering owner. Independently of that decision:
check:changesetsfails while main declares 0.7.5, so the release owner needs to declare 0.8.0 on main before the[breaking]entry can merge.- Please land #7357 and #7969 first and rebase, so that this diff is only the component sweep.
[Automated review]
|
Owner question: retiring direct Decision: either accept retiring direct Pick: accept the retirement. #7969's codemod migrates a raw Blocks: only this pull request, which is already waiting on the release gate (0.8.0). |
# Conflicts: # packages/core/src/AlertDialog/AlertDialog.tsx # packages/core/src/Tokenizer/Tokenizer.tsx
Main's Tokenizer touch sheet (meta#7977) added TokenizerBottomSheet.tsx, which read its strings through useTranslator(), the whole English catalog. It now renders with useComponentTranslator over the three namespaces it reads (textInput, tokenizer, typeahead), like every core component in this sweep. Red first: check:i18n-catalog (a slice coverage problem at TokenizerBottomSheet.tsx before, none now).
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to accept that removal as its compatibility and migration decision (and record it in the provider contract), or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec f4f1f69c7158f363eed040aaee5440ce22badb15 on this pull request, and the author may do so as an engineering owner; the owner question posted here proposes the retirement but does not record it. Independently of that decision:
check:changesetsfails while main declares 0.7.5, so the release owner needs to declare 0.8.0 on main before the[breaking]entry can merge.- Please land #7357 and #7969 first and rebase, so that this diff is only the component sweep.
[Automated review]
# Conflicts: # packages/core/src/Chat/ChatPastedTextToken.tsx # packages/core/src/FileInput/FileInput.tsx
Main's meta#7849 localized Kbd's key names through useTranslator(), the whole English catalog. Kbd now renders with useComponentTranslator over the kbd namespace it reads, like every core component in this sweep. Red first: check:i18n-catalog (six slice coverage problems in Kbd.tsx before, none now).
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to accept it as that contract's compatibility and migration decision, or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec 9d495a7bda0fe1100523dd6981b1045c22209762 on this pull request, and the author may do so as an engineering owner; the [breaking] entry also needs main to declare 0.8.0, because check:changesets refuses it while main declares 0.7.5. Independently of that decision:
- Please land #7357 and #7969 first and rebase, so that this diff is the roughly 1,800-line component sweep rather than 3,796 authored lines.
.changeset/i18n-components-own-slices.mdcounts 83 slice modules, but this head emits 84; please drop or correct the count.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [perf] A component carries only its own English strings. Every core component now renders with its own namespace slice of the English catalog instead of the whole catalog, so an app that renders components without mounting `InternationalizationProvider`, calling `useTranslator()` or importing `InternationalizationContext` (each of which carries the whole catalog), and that does not render `PowerSearch` (whose caller-supplied operator keys keep the whole catalog as their fallback), bundles only the strings of the components it renders: a page importing `Button`, `Dialog`, `DateInput` or `List` loads 6.2 to 6.8 KB (gzip) less JavaScript. An app that does mount the provider still carries the whole catalog, now assembled from the same slices so no string ships twice; such a page loads 1.2 to 1.9 KB (gzip) more, because the catalog spread over 83 slice modules compresses worse than one module (about 0.9 KB more when the provider is imported before the components). `useTranslator()` is unchanged. `astryx swizzle` points an ejected component's strings at the public `useTranslator()`: the internal translator import, the slice imports and a module-level merge of slices are rewritten, so the copy compiles in the app. |
There was a problem hiding this comment.
en.json has 84 namespaces at this head and generate-i18n-runtime.mjs emits 84 English slices, so “spread over 83 slice modules” is stale. Dropping the count, as was done for the key count, keeps the entry from going stale again.
There was a problem hiding this comment.
Fixed in f96ddb4: the entry says "its slice modules", with no count.
The changeset said the catalog is spread over 83 slice modules; with main's kbd namespace this head emits 84, and the count changes with every new namespace. It now says the catalog is spread over its slice modules.
There was a problem hiding this comment.
This pull request is on hold for an owner decision: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so retiring direct InternationalizationContext configuration needs an engineering owner to update that contract for this intentional change, accepting the retirement as its compatibility and migration decision, or to say that the direct-context path must keep working. An engineering owner records that decision by commenting /approve-spec f96ddb4edf87c352db2d0b6d249aea446e2ac6c8 on this pull request, and the author may do so as an engineering owner; the [breaking] entry also needs main to declare 0.8.0, because check:changesets refuses it while main declares 0.7.5. Independently of that decision, please land #7357 and #7969 first and rebase, so that this diff is the roughly 1,800-line component sweep rather than 3,796 authored lines.
[Automated review]
Primary intent
Every core component renders with its own English slice, so an app bundles the strings of the components it renders rather than the whole catalog. Merge order: #7357 (runtime), #7969 (codemod), then this. No other template fits.
User or maintainer impact
InternationalizationProviderloads 6.2 to 6.8 KB less gzip (table below).PowerSearchkeeps the whole catalog for its caller-supplied operator keys.InternationalizationContextno longer drives core components' strings; feat(cli): migrate a raw InternationalizationContext value to the provider #7969's codemod migrates it.const t = useTranslator()line.Problem and solution fit
What led to it: Astryx's i18n work, in three pull requests. Every component read the whole English catalog through
useTranslator(), so every page carried every string.Options:
@astryx.<ns>.*, so the sweep is mechanical and each component imports one or two slices. The cost is the provider-mounted growth and the conflicts above.The sweep, per file:
check:i18n-cataloggains a rule: a component's keys must be in its slices, andpackages/core/srcmay not importuseTranslatororInternationalizationContext.Authority
Owner decision (pending): retiring direct
InternationalizationContextconfiguration is a breaking change; #7969's codemod is the migration; the alternative is to keep the direct path working. Proposed: accept.The release owner's 0.8.0 plan for the
[breaking]entry gates the stack too.Change and evidence
Gzip, the PR size report's measure, taken against main 41e8b4e:
ButtonDialog,DialogHeaderDateInputList,ListItemScope
Testing
i18n.source-build.test.mjsbuildsButtonand checks it imports its own slice, not the catalog or the context.useComponentTranslator.test.tsxchecks the catalog assembled from the slices equalsen.json, so a mounted provider shows every string.pnpm check:i18n-catalogpasses: every component's keys are in its slices.check:changesetsis red on the release gate ([breaking]while main plans patch 0.7.5); a Breadcrumbs focus case that can fail under load is the race test(Breadcrumbs): the hover-focus case hovers a row the menu's first frame did not focus #8383 fixes; a PowerShell cold-start timeout in the CLI'spackage-manager.test.mjsis fixed by test(cli): PowerShell's cold start no longer times out the first commandInFolder test #8530.