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.
|
github-actions
Bot
added
the
needs:design-review
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewComponent 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 Regression55 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 35 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is on hold for a maintainer decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, and component:InternationalizationProvider lists the provider's context behavior as stable public API. That record's owners need to update it to accept the retirement (including that a custom translate has no provider input and that this codemod is the migration) or reject it, and the [breaking] entry also needs main to declare the minor before it can merge. Separately, one fix is needed: packages/cli/assets/codemods/transforms/next/index.mjs still exports an empty list, so astryx upgrade never runs migrate-internationalization-context; please restore the manifest entry that was dropped when the codemod moved out of #7357.
[Automated review]
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [feat] `astryx upgrade` runs `migrate-internationalization-context`, which rewrites `<InternationalizationContext value={…}>` (or its `.Provider`) to `<InternationalizationProvider locale dir messages overrides>`: a variable maps `locale`, `direction` (as `dir`) and `messages` — the fields every context value has — and its TODO names `overrides` and `translate`, which the variable's type may lack and the provider has no input for; an inline object literal maps key by key, `overrides` included, with a TODO for `translate` (a literal's `translate` key is kept beside it). Any other value — a call, a spread into a literal, a conditional — is left unchanged behind a TODO saying why. It rewrites JSX whose name resolves, unshadowed, to an Astryx import (named, aliased, or namespace), reuses the file's own provider import name, and leaves a reader of the context alone. |
There was a problem hiding this comment.
This says astryx upgrade runs the transform, but packages/cli/assets/codemods/transforms/next/index.mjs still exports [] and the codemod registry runs only manifest entries, so at this head the transform never runs. Restoring the manifest entry, dropped when the codemod moved out of #7357, makes this sentence true.
There was a problem hiding this comment.
Registered in bb41827 — the split had dropped the manifest entry, which is exactly the kind of gap that ships silently. transforms/next/index.mjs now exports {name: 'migrate-internationalization-context', transform, meta}, and the fixture file opens with a test that the manifest lists the transform by name with the same transform and meta identities.
e41a9d5 to
bb41827
Compare
There was a problem hiding this comment.
This is on hold for a maintainer decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. That record's owners need to update it to accept the retirement (including that a custom translate has no provider input and that this transform is the migration) or reject it, and main must declare the minor before the [breaking] entry can merge. The restored manifest entry and its test hold, so astryx upgrade will run the transform once it is promoted. Once the decision is made, one change remains: against main this branch is 2,015 authored lines because it carries #7357, so please land #7357 first and rebase, leaving this pull request's own 1,202-line commit.
[Automated review]
bb41827 to
fd3e204
Compare
There was a problem hiding this comment.
This is on hold for a maintainer decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. That record's owners need to update it to accept the retirement (including that a custom translate has no provider input and that this transform is the migration) or reject it, and main must declare the minor before the [breaking] entry can merge. The transform itself holds up: its fixtures, the tsc case and a story it rewrote, which renders identically to the base, cover the migration. Once the decision is made, one change remains: against main this branch is 2,050 authored lines because it carries #7357, so please land #7357 first and rebase, leaving this pull request's own 1,202-line commit.
[Automated review]
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. This is the runtime half, with no change for an app yet; the components move to it in a follow-up. The generator also emits generated-locales/en/<namespace>.generated.ts, one per key namespace, and the exports map gives ./locales/en/* a null target so the slices stay the package's own while every whole catalog stays importable. 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; 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 slice-rendering file reads must belong to a namespace its slice carries, and a file handed `t` is covered by its component directory's slices. Tests pin the resolver, the hook, the public context boundary (a direct `translate` of a core key with no provider → the English string), the compiled import graph of the runtime, the export surface through Node's package resolution, the generator split and the coverage rule. CONTRIBUTING and the hardcoded-string lint messages describe the component pattern; the CLI's i18n guide keeps to what a caller can use.
A value rendered straight into the public InternationalizationContext, without InternationalizationProvider, drove useLocale, useDirection, useCollator and every component's strings in the last release; with the hooks reading the provider's runtime context it reaches none of them. The changeset says so at the breaking tier, with the migration (mount the provider), and the context module's header says which readers each context reaches. The shipped i18n guide qualifies which apps the slicing saves bytes for, and carries its own docs changeset.
…vider
`astryx upgrade` runs migrate-internationalization-context: a
<InternationalizationContext value={v}> (or .Provider) becomes
<InternationalizationProvider locale={v.locale} dir={v.direction}
messages={v.messages} overrides={v.overrides}>; an inline object maps key by
key and a `translate` key, which has no provider input, stays behind a TODO.
Only JSX bound to an Astryx import is rewritten; a file that only reads the
context is left alone. The breaking changeset now carries the full migration:
direction → dir, an own `translate` → messages/overrides, and the codemod.
…; qualify the bundle claim At this revision a raw context value still drives the components' strings through useTranslator(); only useLocale, useDirection and useCollator — and the components reading the locale through them — stop following it, and the sweep's changeset takes the strings when the components move. CONTRIBUTING names the apps the slicing saves bytes for.
…e safely Per the consumer file-modification contract: an element is rewritten only when its name resolves, unshadowed, to the Astryx import (a shadowed local and a type-only import prove nothing); a variable value maps field by field and an inline object literal key by key, each with a TODO for the value's `translate`, which the provider has no input for; any other value — a call, a spread into a literal, a conditional — is left unchanged behind a TODO that says why, since a call would run once per prop and a spread of the ambient value would mount a provider without the app's catalogs. An existing aliased provider import is reused instead of referencing an unbound name. A second run changes nothing. Fixtures cover each case, including idempotence, shadowing and type-only imports; both changesets say what the transform leaves behind, and the breaking entry's component list is corrected.
The generic scope model gives a block no scope of its own, so a same-named local declared in a module-level block was rewritten into the provider and a real use beside a block-level redeclaration inside a function was skipped. The binding check now walks from the element to the module and treats a declaration of the name in any block, switch, loop head, function parameter list, named function expression or catch clause on the way as a shadow. Fixture: four shadowed uses stay, the one unshadowed use beside them is rewritten, and the import is kept.
…and sees hoisted vars A module that already binds InternationalizationProvider — its own wrapper of that name, or an import from its own module — now gets the Astryx provider under a free alias instead of a second binding of the same name. A `var` anywhere in the enclosing function (a nested block, a loop head) and a named class expression's own name shadow the import like a parameter does. Fixtures for each shape.
…m the runtime half The provider name written must itself resolve to the provider's value import at the edit site: a `type` specifier of the provider no longer counts as its import, a local that shadows the provider import leaves the element behind the TODO, and an element whose value arrives through spread props gets the same TODO. Fixtures for each. The lint rule's message edits stay; its formatting reflow is dropped. The slice-coverage rule of check:i18n-catalog moves to the component sweep, where its first callers land.
The codemod changeset, the breaking entry and the transform's description match the transform: a variable maps locale, dir and messages and leaves overrides and translate to the TODO; an inline literal maps key by key.
…alone A code search found three uses of InternationalizationContext outside Astryx (two internal, one public), all of one shape: read the ambient value with useContext, re-mount InternationalizationProvider with a derived one. The transform over those three files returned no change; this fixture is that shape.
… provider When a local shadows the provider import at the edit site, the TODO said the value was an expression; it now names the shadowing local as the cause.
… on its attribute
An empty value={{}} has nothing to map; its TODO sat on the opening element,
where the printer rendered it as text between the tags. It now stays on the
value attribute, as a comment.
…blocks and namespace bodies
A class private method's parameter, a declaration in a class `static {}`
block and one in a TypeScript `namespace` body shadow the import like any
other scope on the way to the module; a `var` search no longer crosses into a
private method or a static block. Fixture for each. The context and codemod
headers describe a component as reading the runtime context once it adopts
its slice, not before.
…shadow; drops an emptied import `export const Ctx` inside a namespace body declares the name like a bare `const`; the statement-list check now looks through `export`. An Astryx import declaration the transform leaves with no specifier is removed instead of staying behind as a bare side-effect import. Fixtures for both.
…ions; keeps an emptied import's comments In a namespace body, `import Ctx = Other.Context`, `declare function Ctx` and `enum Ctx` declare the name like a const does. When an emptied Astryx import is removed, the comments that sat on it (a license header) move to the next statement, and the printer's stray semicolon after a directive is taken out, so `'use client';` stays as written. Fixtures for each.
`namespace Ctx {}` inside a namespace body declares the name, so
`<Ctx.Provider value={v}>` under it stays as written; and a module that
declares `namespace InternationalizationProvider` binds that name, so the
provider import takes a free alias and the rewritten element renders the
alias rather than the namespace. Fixtures for both.
…own name
Inside `namespace Outer.Ctx {}`, `Ctx` names that namespace, so a
`<Ctx.Provider value={v}>` there stays as written, and inside
`namespace Outer.i18n {}` the namespace-import form is left alone rather
than becoming an unbound member. A TSModuleDeclaration on the element's path
binds its last segment. Fixture beside the namespace cases.
…talog The contributing guide's bundle sentence names the third way an app pulls in every string: the public context module imports the English catalog for its default translate.
JSX attributes apply in order, so in `<InternationalizationContext value={v}
{...props}>` a `props.value` wins and the effective value is not provable
from the source; the element now stays as written behind a TODO that says
so. A spread BEFORE `value` is overridden by it and is kept in place: the
mapped props take `value`'s slot in the attribute list, so the spread still
comes first and is still overridden. Fixtures for both orders, and for a
spread on each side.
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.
fd3e204 to
0c24a5e
Compare
There was a problem hiding this comment.
This is on hold for an engineering-owner decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. An engineering owner (the author may, as one) records the decision by commenting /approve-spec <full head SHA> on this pull request, accepting the retirement (including that a custom translate has no provider input and that this transform is the migration) with that record's Compatibility and migration claim amended on main, or rejects it; main must also declare the minor before the [breaking] entry can merge. The transform itself holds up: its fixtures, the tsc case and a story it rewrote, which renders identically to the base, cover the migration. Once the decision is made, one change remains: against main this branch is 2,050 authored lines because it carries #7357, so please land #7357 first and rebase, leaving this pull request's own 1,202-line commit.
[Automated review]
…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).
0c24a5e to
8500a50
Compare
There was a problem hiding this comment.
This is on hold for an engineering-owner decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. An engineering owner (the author may, as one) records the decision by commenting /approve-spec <full head SHA> on this pull request, accepting the retirement (including that a custom translate has no provider input and that this transform is the migration) with that record's Compatibility and migration claim amended on main, or rejecting it; main must also declare the minor before the [breaking] entry can merge. If the retirement is accepted, two changes remain:
- In
.jsand.jsxfiles the transform turns an accessor or method in an inlinevalueliteral into an unparseable or function-valued prop; admitting only plain properties in its literal check, with ababel-parser fixture, fixes it. - Against
mainthis branch is 2,054 authored lines because it carries #7357; please land #7357 first and rebase, leaving this pull request's own 1,202-line commit.
[Automated review]
| value.type === 'ObjectExpression' && | ||
| value.properties.every( | ||
| (/** @type {any} */ p) => | ||
| (p.type === 'Property' || p.type === 'ObjectProperty') && | ||
| !p.computed && | ||
| (p.key.type === 'Identifier' || | ||
| p.key.type === 'StringLiteral' || | ||
| p.key.type === 'Literal'), | ||
| ) |
There was a problem hiding this comment.
In .js, .jsx, .mjs and .cjs files the runner parses with babel (run-codemod.mjs:381), where an accessor or a method is a Property with kind: 'get' or method: true, so this check admits it. get locale() { return store.locale; } becomes locale={() { return store.locale; }}, which the runner's re-parse rejects, failing the whole file without a TODO, and messages() { return m; } becomes messages={function() { return m; }}. The same literals in a .tsx file get the TODO. Admitting only plain properties fixes it:
(p.type === 'ObjectProperty' ||
(p.type === 'Property' && p.kind === 'init' && !p.method)) &&A fixture run through jscodeshift.withParser('babel') would cover that path; every case today uses tsx.
There was a problem hiding this comment.
Fixed in 5b4e841: the literal check admits only plain key: value entries, so a getter or a method in an inline value keeps the expression TODO, as a call or a spread does. A fixture parsed with the babel parser covers both and fails without the fix.
# Conflicts: # packages/cli/assets/codemods/transforms/next/index.mjs
# Conflicts: # packages/core/src/Markdown/Markdown.tsx
There was a problem hiding this comment.
This is on hold for an engineering-owner decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. An engineering owner (the author may, as one) records the decision by commenting /approve-spec <full head SHA> on this pull request, accepting the retirement (including that a custom translate has no provider input and that this transform is the migration) with that record's Compatibility and migration claim amended on main, or rejecting it; main must also declare the minor before the [breaking] entry can merge. If the retirement is accepted, two changes remain:
- In
.jsand.jsxfiles the transform turns a getter or method in an inlinevalueliteral into an unparseable or function-valued prop; admitting only plain properties in its literal check, with ababel-parser fixture, fixes it. - Against
mainthis branch is 2,050 authored lines because it carries #7357; please land #7357 first and rebase, leaving this pull request's own 1,198 lines.
[Automated review]
…ehind the TODO In .js files the runner parses with babel, which reports an accessor or a method in an object literal as a Property (kind 'get', method: true). The literal check admitted them, so a getter for locale became a function-valued prop. Only plain key: value entries are mapped now; any other literal keeps the expression TODO, as a call or a spread does.
There was a problem hiding this comment.
This is on hold for an engineering-owner decision: stacked on #7357, this branch makes a value rendered straight into InternationalizationContext (or its .Provider) stop configuring useLocale, useDirection and useCollator, while component:InternationalizationProvider lists the provider's context behavior as stable public API. An engineering owner (the author may, as one) records the decision by commenting /approve-spec <full head SHA> on this pull request, accepting the retirement (including that a custom translate has no provider input and that this transform is the migration) with that record's Compatibility and migration claim amended on main, or rejecting it; main must also declare the minor before the [breaking] entry can merge. The transform itself holds up: getter and method literals in .js files now stay behind the TODO, and its fixtures, its tsc case and a story it rewrote, which renders like the base, cover the migration. Once the decision is made, one change remains: against main this branch is 2,077 authored lines because it carries #7357, so please land #7357 first and merge or rebase main, leaving this pull request's own 1,225 lines.
[Automated review]
User need
Apps that configure Astryx's translations by rendering a value into
InternationalizationContextstop working when they take #7357, which retires that path ([breaking]there).astryx upgradeis where a version's mechanical migrations live, so this rewrite should run for them instead of every app editing by hand.Problem and solution fit
What led to it: Astryx's i18n work (#7357) splits the English catalog into per-component slices, so a page without the provider loads 6.2 to 6.8 KB less gzip. Review asked for the migration in its own pull request; this is it.
One staged transform,
migrate-internationalization-context:<InternationalizationContext value={i18n}>(or.Provider) becomes<InternationalizationProvider locale={i18n.locale} dir={i18n.direction} messages={i18n.messages}>, with a TODO foroverridesandtranslate.Current specification
spec:AST-040FR1 (edit only a resolved, unshadowed binding) andspec:AST-017FR8's migration evidence. Two owner decisions gate the stack: whethercomponent:InternationalizationProvider's owners accept retiring the raw-context path, and the 0.8.0 release plan for #7357's[breaking]entry.Public delta and caller burden
astryx upgradegains the transform once the release moves it fromtransforms/next/into its version folder. A caller reviews any TODO it leaves.Design and alternatives
vars, namespaces) because the generic scope model gives blocks no scope of their own.Evidence
The fixtures in
transforms/next/__tests__/migrate-internationalization-context.test.mjspair each input with its output: every rewritten and declined shape, shadowing, type-only imports, alias reuse, comments and a second run that changes nothing. One typechecks the output against the real i18n types.Scope
Testing
pnpm vitest run packages/cli/assets/codemods. All pass, each fixture's input rewritten to its expected output, and the typechecked fixture compiles.check:changesetsis red on the maintainers' release gate, because this branch carries perf(i18n): let a component carry only its own English strings #7357's[breaking]entry while main plans patch 0.7.5. This PR's own changeset is a patch[feat].Stacked on #7357, merged after it.