Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
bff006a to
4bfed71
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for the careful slicing work and the mechanical sweep. The internal direction looks coherent: useTranslator() still carries the whole catalog, provider messages and overrides still win over the fallback, and the catalog check covers the statically knowable keys in core.
One public-contract issue needs settling before this can ship as a patch described as leaving the public API unchanged. InternationalizationContext and its value type are exported, so a consumer can call the context's translate member directly, for example translate('@astryx.pagination.next', undefined). That call previously fell back to the shipped English catalog; with the resolver no longer importing a catalog, the same call now returns the key itself whenever the provider does not supply the message. The unchanged context and provider wording still promises the shipped English default, including for consumers who render no provider.
Please either preserve that released context behavior without bringing the whole catalog back into component bundles, or make its retirement deliberate: name it in the changeset at the incompatible tier, point direct-context callers to useTranslator(), update the context and provider wording to match, and pin the chosen behavior with a test at the context boundary. Once that contract is explicit, the slicing change itself looks ready to reassess.
[Automated review]
PR Analysis Report
View Storybook for this PR · View Sandbox for this PR Generated by PR Enrichment workflow | View current CI run |
4bfed71 to
6bd62ea
Compare
|
Agreed, and fixed in 6bd62ea — the first option: the released context behavior is preserved, and the whole catalog stays out of component bundles.
The changeset stays |
github-actions
Bot
added
the
needs:design-review
There was a problem hiding this comment.
Thank you for preserving the released context contract while keeping the savings. This revision looks ready: core components render with their own English slices, while the public context and useTranslator() keep the whole-catalog fallback with or without a provider, and provider messages and overrides still win.
The context boundary is now pinned by tests, the catalog check covers slice usage across core, and I found no remaining public-behavior change beyond the compatible optional-argument widening on that translator. Apps that mount the provider or call useTranslator() still carry the whole catalog exactly as before; the saving lands where only components are rendered.
[Automated review]
6bd62ea to
2c23416
Compare
There was a problem hiding this comment.
Requesting changes for one small, fixable issue: the per-namespace English slices are generated inside the directory that packages/core/package.json exports as ./locales/*.generated.js, so after a build @astryxdesign/core/locales/en/button.generated.js and the other 77 slices resolve and typecheck as public import paths, which conflicts with the stated "no public API change". Please keep them off the public surface, for example with a null export target for ./locales/en/* or by emitting them outside that directory, and add a test that a slice path no longer resolves while locales/en.generated.js still does. Separately, a value passed straight to InternationalizationContext without the provider no longer reaches core components or useLocale/useDirection/useCollator; that usage is undocumented, so it is not blocking, but it may be worth a line in the changeset. Otherwise the change looks solid: rendered strings, provider precedence and the public context's whole-catalog fallback are preserved, and the new slice check catches the regressions it targets.
[Automated review]
| const EN_FILE = 'en.json'; | ||
| const PSEUDO_FILE = 'pseudo.json'; | ||
| /** Where the per-namespace English slices live, under GENERATED_LOCALES_DIR. */ | ||
| const EN_SLICES_DIR = 'en'; |
There was a problem hiding this comment.
These slices are written under generated-locales/, which packages/core/package.json exports as ./locales/*.generated.js. After a build, @astryxdesign/core/locales/en/button.generated.js (and the other 77 slices) resolve and typecheck from a consumer project, so they become public import paths. If they are meant to stay internal, exclude the subtree in the export map (for example a null target for ./locales/en/*, which leaves ./locales/en.generated.js working) or emit them outside the exported directory, and add a resolution test.
There was a problem hiding this comment.
Right — fixed in 37e6f1f. ./locales/*.generated.js matched the slices under generated-locales/en/ too, so they resolved from a consumer after a build.
- The exports map (via
scripts/sync-exports.js) now gives./locales/en/*anulltarget. The longer pattern wins the match, so@astryxdesign/core/locales/en/<ns>.generated.jsisERR_PACKAGE_PATH_NOT_EXPORTEDwhile./locales/en.generated.jsand every other whole catalog resolve as before. locale-exports.test.mjsresolves both kinds through Node's package resolution from a workspace consumer (createRequireon the sandbox app): the whole catalogs resolve todist/i18n/generated-locales/<locale>.generated.js, the slices throw. Verified by mutation: without thenulltarget the slice case fails.- The changeset now also names the undocumented path you flagged: a value handed straight to
InternationalizationContextwithout the provider no longer reaches core components oruseLocale/useDirection/useCollator; mountInternationalizationProvider.
There was a problem hiding this comment.
Amended to b9743a1 (same change): the resolution test used require.resolve, which also checks the target file exists, so it failed in CI where tests run without a built dist/. It now drives import.meta.resolve in a fresh Node process from the sandbox app, which consults the exports map only; passes with and without dist/, still fails without the null target.
37e6f1f to
b9743a1
Compare
b9743a1 to
34cf864
Compare
There was a problem hiding this comment.
Requesting changes for one fixable issue; the slicing itself looks sound, and rendered strings, provider precedence, the public context's whole-catalog fallback and the private export surface are all preserved. Apps that mount InternationalizationProvider still load the whole English catalog through the public context, so each rendered component's slice becomes a second copy of its strings (measured at about +0.2 KB gzip for Button and +1.7 KB for ten common components), while the changeset presents the roughly 5 KB saving as applying to any app. Please generate en.generated.ts from the namespace slices so each string ships once, or state in the changeset which apps benefit and that apps mounting the provider grow slightly. While there, CONTRIBUTING.md, the contributor section of the RTL and testing guide and the hardcoded-string lint messages still tell core authors to use useTranslator(), which the new catalog check now rejects.
[Automated review]
| '@astryxdesign/core': patch | ||
| --- | ||
|
|
||
| [perf] A component carries only its own English strings. The shipped English catalog is now emitted one namespace at a time, and every core component renders with its own slice instead of the whole 408-key catalog, so an app bundles the strings of the components it renders: two rows of buttons and links drop about 5 KB (gzip) from their first load. `useTranslator()` and `InternationalizationContext` are unchanged and still resolve every `@astryx.*` key; an `InternationalizationProvider`'s `messages` and `overrides` still win over the slice; the slices themselves are not importable. One undocumented path moves: a value handed straight to `InternationalizationContext` without the provider no longer reaches core components or `useLocale`, `useDirection` and `useCollator`, which read the provider's runtime context; mount `InternationalizationProvider`. |
There was a problem hiding this comment.
This saving holds only for apps that mount no InternationalizationProvider and call no useTranslator(). With the provider mounted, the public context still imports the whole English catalog and each rendered component's slice becomes a second copy of its strings (measured at +188 B gzip for Button and +1,703 B for ten common components against the merge base). Please either remove the duplication (see the comment on the generator) or say which apps benefit and that apps mounting the provider grow slightly.
There was a problem hiding this comment.
Reworded in 0ceb593: the saving is for an app that renders components without mounting InternationalizationProvider or calling useTranslator(); an app that mounts the provider still carries the whole catalog, now assembled from the same slices so no string ships twice, and moves by under 1 KB either way (+0.8 KB gzip for a single component, −0.8 KB for ten).
| Object.keys(runtimeCatalogs).map(getGeneratedModuleName), | ||
| ); | ||
| const removed = removeStaleModules(expectedFiles); | ||
| const slices = createNamespaceSlices(runtimeCatalogs[EN_FILE]); |
There was a problem hiding this comment.
The slices are emitted as copies beside en.generated.ts, which the loop below still renders from the full map. Generating en.generated.ts as an import-and-spread of the 78 slice modules would let the public context and the components share one copy of each string, so apps that mount the provider stop paying for them twice.
There was a problem hiding this comment.
Done in 0ceb593 — the first option. en.generated.ts is now the spread of the 78 slice modules (import buttonMessages from './en/button.generated'; … const runtimeCatalog = {...buttonMessages, …}), so a bundle that reaches both a component's slice and the whole catalog carries each string once. useComponentTranslator.test.tsx pins the assembled catalog against the authored en.json: same key set, same messages. Measured against the merge base on the built dist (esbuild, minified, React external): with the provider mounted, Button 34,155 → 34,979 B gzip (+824 B: 78 small modules cost more than one object for a single component) and ten common components 109,001 → 108,189 B gzip (−812 B); no string appears twice in either bundle. Without the provider the saving is unchanged (Button −5.3 KB, ten components −6.5 KB).
| * of the hook callers in its component directory. The consumer-facing | ||
| * `useTranslator` and `InternationalizationContext`, which carry the | ||
| * whole catalog, are not for files in packages/core/src. |
There was a problem hiding this comment.
This rule now rejects useTranslator in packages/core/src, but CONTRIBUTING.md (lines 294-304), the contributor section of packages/cli/assets/docs/tree/i18n-rtl-and-testing.doc.mjs (lines 125-140) and the @astryx/no-hardcoded-i18n-string messages still tell core authors to use useTranslator(). Please point them at useComponentTranslator(<slice>).
There was a problem hiding this comment.
Updated in 0ceb593: CONTRIBUTING.md (the "User-facing strings" section), the Developers section of i18n-rtl-and-testing.doc.mjs, and the three @astryx/no-hardcoded-i18n-string messages now send core authors to useComponentTranslator(<slice>) with the slice import, the module-level merge for several namespaces, and the generate/check commands; each names useTranslator() as the consumer-facing hook that packages depending on core keep using.
There was a problem hiding this comment.
Amended to c030c72: the CLI's shipped doc page changed, so the changeset now also names @astryxdesign/cli (patch; the packages release together), which the changeset-coverage gate required.
0ceb593 to
c030c72
Compare
There was a problem hiding this comment.
Requesting changes for one small documentation fix; the slicing itself looks sound: rendered strings, provider precedence, the public context's whole-catalog fallback and the private export surface are preserved, and the whole English catalog is now assembled from the slices so no message ships twice. The RTL & Testing guide ships to people building with Astryx, and its updated Developers section now shows the internal useComponentTranslator hook with relative imports into core source and cites the pnpm check:i18n-catalog gate, neither of which callers can use (this narrows the earlier request to update that guide). Please keep that contributor pattern in CONTRIBUTING.md, which this PR already updates, and leave only caller-facing facts in the shipped guide, such as that useTranslator() resolves every @astryx.* key and that an app bundles only the strings of the components it renders, with a pointer to the contributing guide. The branch also needs a rebase onto main.
[Automated review]
| lang: 'tsx', | ||
| label: 'Read an astryx string', | ||
| code: `import {useTranslator} from '@astryxdesign/core/i18n'; | ||
| code: `import {useComponentTranslator} from '../i18n/useComponentTranslator'; |
There was a problem hiding this comment.
This guide ships to people building with Astryx through astryx docs, but useComponentTranslator and the generated-locales/en/* slices cannot be imported from @astryxdesign/core: the i18n barrel doesn't export the hook, and ./locales/en/* is now null. spec:AST-041 keeps contributor-only material out of shipped docs. Could this example (and the pnpm check:i18n-catalog mention below) stay in CONTRIBUTING.md, which this PR already updates, with the shipped page keeping only the caller facts and a pointer there?
There was a problem hiding this comment.
Agreed — fixed in a9e0a7c. The shipped page now carries only what a caller can use: components carry their own strings, so an app bundles only the strings of the components it renders, and useTranslator() from @astryxdesign/core/i18n resolves every @astryx.* key (the original example, restored). The internal hook, the relative imports and the check command are gone from it; the contributor pattern stays in CONTRIBUTING.md, which the page points to. Also rebased onto main after the date-input prop-target change.
c030c72 to
a9e0a7c
Compare
a9e0a7c to
1712f58
Compare
|
metadata-only rewrite: commit author identity; tree unchanged ( |
1712f58 to
fffb1f4
Compare
fffb1f4 to
d485b93
Compare
…rs with its own slice resolve.ts imported the whole shipped English catalog (408 keys, 78 component namespaces, 22.6 KB raw / 6 KB gzip) as the fallback of every translation, so any app rendering one component that calls t() bundled the strings of the other hundred. The generator now also emits generated-locales/en/<namespace>.generated.ts, one per key namespace. resolve.ts imports no catalog: the English fallback is the caller's, passed per call. useComponentTranslator(slice) is the hook a core component renders with, its slice being the namespace module(s) it reads. It, useLocale, useDirection and useCollator read a new internal TranslationRuntimeContext that carries the resolver and no catalog; the public InternationalizationContext keeps its shape and its `translate` bound to the whole catalog, the provider writes both, and useTranslator() reads the public one as before, so a consumer's direct `translate(key)` and `useTranslator()` behave exactly as today. The provider's messages and overrides still win over the fallback. check:i18n-catalog gains the slice-coverage rule: every @AstryX.<ns>.* key a file in packages/core/src reads must belong to a namespace its slice carries, a file handed `t` is covered by its component directory's slices, and the consumer hook and the public context are not for files in that package. Unit tests cover the generator split, the rule, the hook, and the compiled import graph of the i18n runtime.
…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.
… surface `./locales/*.generated.js` matched the slices under generated-locales/en/ too, so `@astryxdesign/core/locales/en/button.generated.js` resolved from a consumer after a build. The exports map now gives `./locales/en/*` a null target: the longer pattern wins, the whole catalogs stay importable, and the slices are the package's own. locale-exports.test.mjs resolves both through Node's package resolution from a workspace consumer and fails without the null target. The changeset names the one undocumented path that moves (a value handed straight to InternationalizationContext without the provider).
… string ships twice An app that mounts InternationalizationProvider or calls useTranslator() carries the whole catalog through the public context; with the components' slices beside it, 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. The changeset says which apps gain the saving and how a provider-mounting app moves. CONTRIBUTING, the CLI's i18n guide and the hardcoded-string lint messages now send core authors to useComponentTranslator(<slice>), which the catalog check requires, and name useTranslator() as the consumer-facing hook.
The shipped RTL and testing guide had gained the internal useComponentTranslator pattern with relative imports into core source and the contributor check command, none of which a caller of the package can reach. The page now says what holds for a caller: components carry their own strings, so an app bundles only the strings of the components it renders, and useTranslator() resolves every @AstryX.* key; the contributor pattern stays in CONTRIBUTING.md.
d485b93 to
7d28f91
Compare
|
Superseded by a split so the review-clean runtime can land in any window while the 103-file sweep absorbs |
Primary intent
A core component should carry only its own English strings.
resolve.tsimports the whole English catalog (408 keys, 78 namespaces, 6 KB gzip) as every translation's fallback, so an app rendering one translating component (Buttonis one) bundles the other hundred's. Nothing is broken or added: no other template fits.User or maintainer impact
A consumer rendering core components without the provider or
useTranslator()loses the catalog's breadth from its first load; the same strings render.Problem and solution fit
The generator emits
generated-locales/en/<namespace>.generated.tsper key namespace and assemblesen.generated.tsfrom them.resolve.tsimports no catalog: the English fallback is passed per call. A core component renders withuseComponentTranslator(slice), the slice being the namespace module(s) it reads; it and the locale hooks read an internalTranslationRuntimeContextcarrying no catalog.Back-compat contract: the public surface keeps the whole catalog.
InternationalizationContextkeeps its shape and itstranslatestill falls back to the whole English catalog, with or without a provider;useTranslator()reads it as before; the provider writes both contexts; itsmessagesandoverridesstill win. A consumer's own components and a directtranslate(key)on the context are unaffected. A core component reading a key outside its slice renders the key and warns once in development, as a missing key does today;check:i18n-catalogfails first: every@astryx.<ns>.*key a core file reads must be in a namespace its slice carries (a file handedt: its directory's slices); importinguseTranslatororInternationalizationContextthere is an error (mutation-verified).Commits: runtime, generator, check, tests; the 103-file sweep, every hunk this shape; two review follow-ups:
No public API change; neither new module is exported.
Authority
resolve.ts's documented lookup order: step 5's English fallback is now the caller's.Change and evidence
esbuild, minified, React external, no provider:
Button34,331 → 29,020 B gzip (−5,311 B); ten components 109,269 → 102,719 B gzip (−6,550 B). With the provider mounted the whole catalog is the spread of the slices, no string twice:Button+824 B, ten components −812 B.i18n.source-build.test.mjs: the compiled resolver, runtime context and hooks import no catalog; the public context the whole one;Buttonits slice only.useComponentTranslator.test.tsx: own slice resolves; provider catalog and override win; a key outside the slice returns the key and warns; the public context'stranslatefalls back to English with and without a provider.en.json.Scope
Testing
pnpm vitest run --maxWorkers=4(whole repo, Node 24.11): 1,094 files / 22,781 tests passed; the one failure is a CLI test binding a fixed port already in use on the box.node scripts/check-i18n-catalog.mjs: 130useComponentTranslatorcalls carry the namespaces their files read.pnpm check:repo,tsc --noEmit(core),pnpm -F @astryxdesign/core build,eslint .: clean (0 errors).[perf], patch.