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
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for splitting this out: the runtime change looks sound, since the public InternationalizationContext and useTranslator() keep the whole English fallback and the new modules and slices stay off the public surface. One fix is needed before approval: the shipped CLI guide (i18n-rtl-and-testing.doc.mjs) now says an app bundles only the strings of the components it renders, which isn't true at this revision because every core component still reads through useTranslator(), and it stays untrue for apps that mount InternationalizationProvider, which imports the whole catalog. Please drop that clause here, or state the condition precisely once components actually move to their slices.
[Automated review]
| { | ||
| 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 promises per-component bundling, but at this revision every core component still reads through useTranslator(), whose context module imports the whole English catalog. Even after components move to slices, an app that mounts InternationalizationProvider (as this guide's pseudo-locale example does) still bundles the whole catalog. Suggest dropping the bundling clause here, or stating that condition once components migrate.
There was a problem hiding this comment.
Dropped in 305efa8: at this revision the guide makes no bundle claim — components still read through useTranslator() here. The qualified sentence (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) lands with the components in #7358. Also in 305efa8: the raw-context path's retirement is now declared [breaking] (core, minor; main plans 0.7.0) with the migration to the provider, since the hooks reading the runtime context are what retires it, and this is the PR that moves them.
305efa8 to
98c0131
Compare
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 Regression48 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 28 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
There was a problem hiding this comment.
This is waiting on a maintainer: retiring the raw InternationalizationContext configuration path changes released public behavior, and no current spec record owns that i18n contract yet, so a maintainer needs to identify or record the governing spec that makes InternationalizationProvider the only supported configuration path, including what happens to a raw value's custom translate. Independently, please correct the release notes before this merges:
- The
[breaking]Changeset says a raw context value no longer drives core components' strings, but at this revision onlyuseLocale,useDirectionanduseCollatorstop following it, and the provider has no equivalent for a raw value'stranslate. CONTRIBUTING.mdstill says an app bundles only the strings of the components it renders, which does not hold yet, or later for apps that mount the provider.
[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.
At this revision a raw InternationalizationContext value still drives core components' strings: they resolve through useTranslator(), which returns the public context's translate, and no component calls useComponentTranslator yet. Only useLocale, useDirection and useCollator stop following it. Please scope this entry to those hooks (and the components that call them), note that a raw value's own translate has no provider equivalent, and extend the entry when #7358 moves the components.
There was a problem hiding this comment.
Scoped in 24085fa: the entry now says a raw value no longer drives useLocale, useDirection and useCollator, nor the components that read the locale or direction through them (the date, time and number inputs, Calendar, Chat, PowerSearch, Table sorting, Timestamp); that its strings still follow such a value through useTranslator() until the components move to their slices, which #7358's entry takes up; and that a raw value's own translate has no provider input — those strings become messages or overrides, and the codemod leaves a translate key behind a TODO.
| Components read user-facing strings with `useComponentTranslator(<slice>)` | ||
| rather than hardcoding them. The slice is the component's own namespace of the | ||
| English catalog, generated under `src/i18n/generated-locales/en/`, so an app | ||
| bundles only the strings of the components it renders: |
There was a problem hiding this comment.
This holds only for apps that neither mount InternationalizationProvider nor call useTranslator(): the provider imports the public context module, which imports the whole English catalog, and no component renders with a slice yet. Suggest qualifying it that way, or leaving it out until the components move, as the CLI guide now does.
There was a problem hiding this comment.
Qualified in 24085fa: "so an app that neither mounts InternationalizationProvider nor calls useTranslator() (both carry the whole catalog) bundles only the strings of the components it renders".
There was a problem hiding this comment.
This is waiting on a maintainer: retiring the raw InternationalizationContext configuration path changes released public behavior, and no current spec record owns that i18n contract yet, so a maintainer needs to identify or record the governing spec that makes InternationalizationProvider the only supported way to configure locale, direction and strings, including what happens to a raw value's custom translate. Thanks for scoping the release note and the contributing guide; independently, these still need fixing before this can merge:
- The
astryx upgradetransform drops a customtranslatewithout a TODO when the value is a variable or expression, rewrites a locally shadowed name, references an unboundInternationalizationProviderwhen the provider is imported under an alias, and mounts a provider without the ambient catalogs for a spread value. - Its tests need a second-run idempotence case plus shadowed and type-only cases, as the codemod contract requires.
- The
[breaking]and[feat]Changesets should say what the transform leaves behind for a non-literal value, and the breaking entry's component list misses Table filtering and touchDateTimeInput, which still reads the context directly.
[Automated review]
| name = name.object; | ||
| } | ||
| if (name.type === 'JSXIdentifier') { | ||
| return contextNames.has(name.name) ? '' : null; |
There was a problem hiding this comment.
This matches the JSX name against the file's imports by name only, so a local binding that shadows the import (for example const Ctx = MyContext under import {InternationalizationContext as Ctx}) is rewritten into an Astryx provider. spec:AST-040 FR1 requires a resolved binding that is not shadowed at the edit site, and leaving the content unchanged otherwise; checking the binding through scope would fix it, with a shadowed fixture to prove it.
There was a problem hiding this comment.
Fixed in 01b084d: the element's name must resolve through scope to the module-level import (path.scope.lookup(name) landing on Program), as the field-input codemod does; a shadowed local (a parameter, or const Ctx = MyContext) is left unchanged. Fixtures: a shadowed parameter and a shadowed const, plus a type-only import beside a same-named local createContext, all untouched.
| ); | ||
| } | ||
| } else { | ||
| for (const [key, prop] of PROP_FOR_KEY) { |
There was a problem hiding this comment.
For a value that is not an object literal this maps four fields and drops value.translate with no TODO, although translate is a required field of InternationalizationContextValue; an expression such as makeI18n() is also copied into all four props and evaluated four times. Leaving non-literal values unchanged behind an accurate TODO, or binding the value once and keeping translate behind the TODO, would avoid both.
There was a problem hiding this comment.
Fixed in 01b084d: only an Identifier value maps field by field (a variable read is pure), with the translate TODO on every rewrite since every context value has one; any other expression — a call, a conditional, a literal with a spread — is left unchanged behind a TODO that says why (a call would run once per prop; a spread of the ambient value would mount a provider without the app's catalogs). Fixture: value={makeI18n()} stays, with the TODO.
| const providerName = | ||
| namespace === '' | ||
| ? j.jsxIdentifier(PROVIDER) |
There was a problem hiding this comment.
When the file already imports the provider under an alias (InternationalizationProvider as IP), hasProvider is true so no specifier is added, but this still renders <InternationalizationProvider>, which is unbound, and the context import is removed. Reusing the existing local name would fix it.
There was a problem hiding this comment.
Fixed in 01b084d: the file's existing local name for the provider is reused (<IP …>/</IP> under InternationalizationProvider as IP) and no specifier is added; the fixture asserts no unbound <InternationalizationProvider appears and the import line is unchanged.
| if (leftovers.length > 0) { | ||
| // Keep what has no provider input in sight, behind a TODO. | ||
| const kept = j.jsxExpressionContainer(j.objectExpression(leftovers)); | ||
| const attribute = j.jsxAttribute( | ||
| j.jsxIdentifier('data-astryx-upgrade-leftover'), | ||
| kept, | ||
| ); | ||
| attribute.comments = [j.commentBlock(TODO, true, false)]; | ||
| opening.attributes.push(attribute); | ||
| } |
There was a problem hiding this comment.
For a spread such as {...ambient, locale: 'de'} this keeps ...ambient in an attribute the provider ignores, and the provider takes no catalogs from its parent, so that subtree loses the app's messages and overrides; the TODO text only talks about translate. A spread value is safer left unchanged behind a TODO that says why.
There was a problem hiding this comment.
Fixed in 01b084d: a spread value is left unchanged behind the expression TODO, which says why (a provider takes no catalogs from a parent, so the subtree would lose messages and overrides). The old lossy data-astryx-upgrade-leftover attribute is gone; the fixture asserts {...ambient, locale: 'de'} stays as written and no provider is mounted.
| // A spread cannot be mapped to props; it stays behind the TODO. | ||
| expect(output).toContain("locale={'de'}"); | ||
| expect(output).toContain('...ambient'); |
There was a problem hiding this comment.
This asserts the lossy spread output noted on the transform. The suite also needs the second-run idempotence case and the shadowed and type-only cases that spec:AST-017 FR8 and spec:AST-040 FR1 list.
There was a problem hiding this comment.
Updated in 01b084d: the spread fixture now asserts the value is left unchanged behind the TODO; added the second-run idempotence case (a rewritten file, a kept-literal translate, and an untouched call: the second run returns no change), the shadowed cases (a parameter and a local const), and the type-only case.
| '@astryxdesign/core': minor | ||
| --- | ||
|
|
||
| [breaking] An app that sets `InternationalizationContext` directly — rendering `<InternationalizationContext value={…}>` (or its `.Provider`) instead of `InternationalizationProvider` — no longer drives `useLocale`, `useDirection` and `useCollator`, nor the components that read the locale or direction through them (the date, time and number inputs, Calendar, Chat, PowerSearch, Table sorting, Timestamp). Its strings still follow such a value through `useTranslator()` until the components move to their own slices, which stops that too. 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. `astryx upgrade` runs `migrate-internationalization-context`, which performs the rewrite and leaves a `translate` key behind a TODO. 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. 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 slices are not importable. |
There was a problem hiding this comment.
Two parts of this don't hold yet: the transform leaves a translate TODO only for an inline object literal (a variable or expression value loses translate silently), and touch DateTimeInput still follows a raw value's locale because TouchDateTimeField reads the context directly. Table filtering also reads useLocale() but isn't listed.
There was a problem hiding this comment.
Fixed in 01b084d: the entry now lists the date, date-time, date-range and number inputs, Calendar, Chat, PowerSearch, Table sorting and filtering, and Timestamp, and says that a raw value's strings still follow through useTranslator() — as does the locale of DateTimeInput's touch field — until the components move in #7358. The [breaking] and [feat] entries both say what the transform leaves behind: a variable or inline literal is rewritten with a TODO for translate; a call, a spread or a conditional is left unchanged behind a TODO saying why.
There was a problem hiding this comment.
This is still waiting on a maintainer: retiring the raw InternationalizationContext configuration path changes released public behavior, and no current spec record owns that i18n contract yet, so a maintainer needs to identify or record the governing spec that makes InternationalizationProvider the only supported way to configure locale, direction and strings, including what happens to a raw value's custom translate. Thanks for the codemod and release-note fixes; the earlier points are resolved. One small fix remains in migrate-internationalization-context: its binding check treats a block-scoped declaration as belonging to the enclosing function or module, so a same-named local declared in a module-level block is rewritten into the provider while a real use beside a block-level redeclaration inside a function is skipped, and resolving block scopes, with a fixture for each case, would close it.
[Automated review]
| const scope = path.scope?.lookup(name); | ||
| return scope?.path.node.type === 'Program'; |
There was a problem hiding this comment.
This resolves the name through a scope model that gives blocks no scope of their own, so a block-level redeclaration is misjudged both ways:
import {InternationalizationContext as Ctx} from '@astryxdesign/core/i18n';
// Module level: rewritten into <InternationalizationProvider>, though Ctx here is a local.
{
const Ctx = MyOwnContext;
out = <Ctx value={v}><A /></Ctx>;
}
// Inside a function: the unshadowed <Ctx value={w}> is left unchanged, with no TODO.
function F({flag}) {
if (flag) {
const Ctx = Other;
return <Ctx value={v}><A /></Ctx>;
}
return <Ctx value={w}><B /></Ctx>;
}spec:AST-040 FR1 needs a binding “not shadowed at the edit site”, and its verification fails when a same-named local changes or a proven usage is not edited. Resolving block-scoped declarations between the element and the module (or declining whenever the name has a declaration other than the import), with a fixture for each case, would close it.
There was a problem hiding this comment.
Fixed in 8c1233e: the binding check now walks from the element to the module itself instead of the generic scope lookup, and a declaration of the name in any block, switch, loop head, function parameter list, named function expression or catch clause on the way shadows the import; at the module, only the import may declare it. Your fixture is the test, extended: the module-level block, the if block, a for (const Ctx of …) head and a catch (Ctx) all stay as written, and the unshadowed <Ctx value={w}> beside them is rewritten to the provider with the import kept.
There was a problem hiding this comment.
This is still waiting on a maintainer: retiring the raw InternationalizationContext configuration path changes released public behavior, and no current spec record owns that i18n contract yet, so a maintainer needs to identify or record the governing spec that makes InternationalizationProvider the only supported way to configure locale, direction and strings, including what happens to a raw value's custom translate. Thanks for the block-scope fix, which now holds; two cases in migrate-internationalization-context still need fixing before this can merge:
- When the file already declares or imports its own
InternationalizationProvider(an app wrapper of that name, say), the transform adds a second binding of that name, so the rewritten module no longer compiles; import the provider under a free alias there, or leave the element behind the TODO. - A
vardeclared in a nested block or loop head of the enclosing function, or a named class expression, shadows the import but is not detected, so an element naming that local is still rewritten into the provider; counting avaras its whole function's binding and a class expression's name like a function expression's, with fixtures, would close it.
[Automated review]
| const providerName = | ||
| namespace === '' | ||
| ? j.jsxIdentifier(providerLocalName ?? PROVIDER) | ||
| : j.jsxMemberExpression( | ||
| j.jsxIdentifier(namespace), | ||
| j.jsxIdentifier(PROVIDER), | ||
| ); | ||
| if (namespace === '' && providerLocalName === null) { | ||
| needsNamedProvider = true; |
There was a problem hiding this comment.
When the file has no Astryx provider import, this renames the element to InternationalizationProvider and later appends that specifier to the Astryx import, without checking whether the module already binds that name. A file that imports InternationalizationProvider from its own module, or declares an app wrapper of that name around the raw context, ends up with two bindings, which Babel rejects as a duplicate declaration, and in the wrapper case the rewritten element names the wrapper itself. Checking that the name is free first, and otherwise importing the provider under a free alias or leaving the element behind the TODO, with a fixture for each shape, would avoid it.
There was a problem hiding this comment.
Fixed in 9f2dca3: the transform collects every identifier the module binds (declarations, imports, parameters, catch clauses, class and function names) and, when InternationalizationProvider is already taken and no Astryx provider import exists, imports the provider under a free alias (InternationalizationProvider2, counting up) and renders that. Fixtures: an app wrapper named InternationalizationProvider around the raw context (its body is rewritten to <InternationalizationProvider2 …>, the wrapper keeps its name), and an own-module import {InternationalizationProvider} from './provider' beside it.
| const isImportBinding = (path, name) => { | ||
| for (let p = path.parent; p; p = p.parent) { | ||
| const node = p.node; | ||
| switch (node.type) { | ||
| case 'Program': | ||
| return !bodyDeclares(node.body, name); |
There was a problem hiding this comment.
This walk only sees declarations on the element's own ancestors, but a var hoists to its whole function and a named class expression binds its name inside its body. So if (flag) { var Ctx = Other; } return <Ctx value={w}>…, for (var Ctx of list) {} return <Ctx value={w}>… and const Shell = class Ctx { render() { return <Ctx value={v}>… } } are still rewritten into the provider, which spec:AST-040 FR1 rules out (“not shadowed at the edit site”). Counting a var anywhere in the enclosing function as that function's binding, and treating a class expression's name like a function expression's, with fixtures, would close it.
There was a problem hiding this comment.
Fixed in 9f2dca3: a var anywhere inside the enclosing function (or the module), found without crossing into nested functions, counts as that function's binding — so if (flag) { var Ctx = Other; } and for (var Ctx of list) {} shadow the later <Ctx value={w}>; and a named class expression's own name shadows inside its body like a named function expression's. Your three shapes are the fixture, with an unshadowed module-level use beside them that is still rewritten.
There was a problem hiding this comment.
This is still waiting on a maintainer: retiring the raw InternationalizationContext configuration path changes released public behavior, and no current spec record owns that i18n contract yet, so a maintainer needs to identify or record the governing spec that makes InternationalizationProvider the only supported way to configure locale, direction and strings, including what happens to a raw value's custom translate. Thanks for the alias and hoisting fixes, which hold; independently of that decision, two things need changing before this can merge:
- The pull request is now 2,024 authored changed lines, over the 2,000-line scope threshold; dropping the formatting-only reflow of
no-hardcoded-i18n-string.js(keeping its message edits) brings it under, and moving the slice-coverage rule to #7358, where its first callers land, would trim it further. migrate-internationalization-contextdoesn't check the provider name it writes: an inlinetypeimport of the provider leaves<InternationalizationProvider2>unbound and a local that shadows the file's provider import captures the element, so resolve that name to the provider's value import at the edit site (or leave the element behind the TODO), and give an element that only spreads props the same TODO.
[Automated review]
| const providerNeedsImport = !astryxImports.some((/** @type {any} */ path) => | ||
| (path.node.specifiers ?? []).some( | ||
| (/** @type {any} */ s) => | ||
| s.type === 'ImportSpecifier' && s.imported?.name === PROVIDER, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
This counts a type specifier named InternationalizationProvider as an existing import, but the scan above skips type specifiers when it records providerLocalName (line 97). With import {InternationalizationContext, type InternationalizationProvider} from '@astryxdesign/core/i18n', the free-alias step picks InternationalizationProvider2, no import is added and the context import is dropped, so the rewritten element is unbound. Skipping type specifiers here too, with a fixture, would fix it.
There was a problem hiding this comment.
Fixed in 092fb23: a type specifier of the provider no longer counts as its import (the same importKind !== 'type' test as the scan above), so the named import is added under the free alias. Fixture: import {InternationalizationContext, type InternationalizationProvider} → the element renders <InternationalizationProvider2 …> and the import gains InternationalizationProvider as InternationalizationProvider2.
| const providerName = | ||
| namespace === '' | ||
| ? j.jsxIdentifier(providerLocalName) |
There was a problem hiding this comment.
The name written here isn't checked at the edit site the way the context name is. If a local shadows the file's provider import (const InternationalizationProvider = makeLocalProvider(); inside the component), the rewritten element resolves to that local. Requiring isImportBinding(path, providerLocalName) before renaming, and otherwise leaving the element behind the TODO, would close it.
There was a problem hiding this comment.
Fixed in 092fb23: before renaming, the provider's local name must itself satisfy isImportBinding(path, providerLocalName) at the edit site; otherwise the element stays behind the expression TODO. Fixture: const InternationalizationProvider = makeLocalProvider(); inside the component leaves <InternationalizationContext value={i18n}> as written, with the TODO.
| if ( | ||
| !valueAttribute || | ||
| valueAttribute.value?.type !== 'JSXExpressionContainer' | ||
| ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
An element that sets the context only through spread props (<InternationalizationContext {...props}>) returns here without the TODO, so the upgrade leaves no marker on it. Giving it the expression TODO would match how other values the transform can't rewrite are handled.
There was a problem hiding this comment.
Fixed in 092fb23: an element with no value attribute but a spread (<InternationalizationContext {...props}>) gets the expression TODO on the spread, so the upgrade leaves its marker; the fixture asserts the element is unchanged and the TODO present.
…; 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 CLI's strict typecheck refused `string | null` where a string goes; the name is resolved once, up front, and never null.
… qualify the check-rule mentions
`overrides` is the one optional field of a context value, so a variable
whose inferred type lacks it would fail to typecheck as `v.overrides`;
a variable now maps `locale`, `direction` (as `dir`) and `messages`, and
its TODO names `overrides` beside `translate`. A fixture typechecks the
rewritten output against the real i18n source types with tsc, and shows the
naive `overrides={v.overrides}` is TS2339. CONTRIBUTING and the comments in
resolve.ts, resolve.test.ts and useComponentTranslator.ts say the
slice-coverage rule lands with the component sweep.
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.
ef84cea to
799e580
Compare
There was a problem hiding this comment.
This is on hold for owner decisions, not for changes from you: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so an engineering owner needs to accept or reject, and record in that record, that a value rendered directly into InternationalizationContext (or its .Provider) no longer configures useLocale, useDirection and useCollator, with no provider input for a custom translate; they record the decision by commenting /approve-spec <full head SHA> on this pull request, which you may do as an engineering owner. If it is accepted, the release owner also needs to name 0.8.0 as main's next plan, because check:changesets refuses the [breaking] entry while main declares the patch 0.7.5; the alternative is a deprecation that keeps a raw value working. Because the [breaking] entry tells readers to run migrate-internationalization-context, #7969 needs to land in the same release.
[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.
799e580 to
f9267e0
Compare
There was a problem hiding this comment.
This is on hold for owner decisions, not for changes from you: component:InternationalizationProvider lists the provider's context behavior and hooks as stable public API, so an engineering owner needs to accept or reject, and record in that record, that a value rendered directly into InternationalizationContext (or its .Provider) no longer configures useLocale, useDirection and useCollator, with no provider input for a custom translate; they record the decision by commenting /approve-spec <full head SHA> on this pull request, which you may do as an engineering owner. If it is accepted, the release owner also needs to name 0.8.0 as main's next plan, because check:changesets refuses the [breaking] entry while main declares the patch 0.7.5; the alternative is a deprecation that keeps a raw value working. Because the [breaking] entry tells readers to run migrate-internationalization-context, #7969 needs to land in the same release.
[Automated review]
…8075) ## User impact Source builds through `astryxStylex` (Storybook's too) lose `defineConsts` values read from another file: a media-query key fails the build ("Invalid empty selector"); a value ships `var(--x…)` that nothing declares, which browsers drop. Plain `@stylexjs/unplugin` resolves both. ## Problem and solution fit StyleX compiles a constant read to a placeholder, `var(--<prefix><hash>)`, that the final CSS pass replaces. Two defects break that: - **Missing import compensation.** Under the `{stylexOptions}` form, the babel wrapper's StyleX instance runs without `treeshakeCompensation`, so the TypeScript transform drops the now-unused import and the defining module never loads. - **Per-layer, per-prefix substitution.** A constant defined across the layer split, or in a package (library prefix) and read by an app (product prefix), stays a placeholder. Fix: - The wrapper defaults to `treeshakeCompensation: true`, as the unplugin does. - Every layer gets every constant under every prefix; constant rules emit no CSS. - A production build warns, naming the property and its rule, when its stylesheet reads a generated property no emitted stylesheet declares. No record admits an `astryxStylex` option for it, so there is none. ## Expected behavior and authority StyleX inlines `defineConsts` values where used, media-query keys included, as `@stylexjs/unplugin` does. The prebuilt stylesheets already fail on the condition the warning reports. ## Smallest restoration `packages/build/src/vite.ts`, with the detector in internal `undefinedVariables.ts` (`./vite` exports only `astryxStylex` and `LIGHTNINGCSS_TARGETS`); an explicit `treeshakeCompensation` still wins. ## Evidence - **Before:** the new `stylex-consts` fixture (sibling, cross-layer and package constants as keys and values) fails the legacy form ("Invalid empty selector"); the zero-config form ships placeholders. - **After:** both forms emit `min-height:52px`, `padding-top:13px`, `padding-bottom:17px` and both media rules. - **Unchanged:** the Storybook build on `main` and with this change is byte-identical (`astryx-stylex-9dbf4f45.css`) and passes the check. - **Regression tests:** both forms; cross-layer and cross-prefix reads; a plain export read as a token warns; a custom `classNamePrefix`; an app's own `--xl`/`--xlarge` isn't reported. Open siblings #7357 and #7358 also change `vite.ts`; both merge with this change without a conflict there. ## Scope - [x] One defect is restored. - [x] Separable contradictory or unsettled tagalongs were removed or split. - [x] No new public API, visual representation, default, or interaction model is hidden under the bug-fix label. The warning has its own `[feat]` changeset and adds no option. - [x] Public text and artifacts contain no internal Meta context. ## Testing - Local gates at 8f66f86: build, type check, `pnpm test:ui` (16,446), the node suites, lint, Storybook, `pnpm check:knowledge` and changeset coverage. - `pnpm exec vitest run packages/build scripts/build-css apps/storybook/.storybook/main.test.ts`: 161 passed, 6 skipped (browser-only). This includes the new build tests (both forms, cross-layer, cross-prefix, the warning, a custom product prefix, an app's own properties) and the detector unit tests; each new test fails without the change. - `pnpm -F @astryxdesign/build build`; `tsc -p packages/build/tsconfig.build.json --noEmit`. - Changeset coverage: ships to `@astryxdesign/build`, two changesets (`[fix]` for the correction, `[feat]` for the warning); `pnpm check:changesets` passes. - `pnpm storybook:build` on `main` and with this change: identical StyleX CSS (`astryx-stylex-9dbf4f45.css`), and the check reports nothing. `pnpm -F @astryxdesign/sandbox build`: no report either.
Primary intent
Let a core component carry only its own English strings.
resolve.tsimports the whole English catalog (523 keys, 83 namespaces) as every translation's fallback, so a page rendering one translating component bundles every component's strings. This is the runtime half; #7358 moves the components. No other template fits.User or maintainer impact
Button41,086 → 41,027 B gzip). With perf(i18n): every core component renders with its own English slice #7358, a page drops 6.2 to 6.8 KB gzip without the provider and grows 1.2 to 1.9 KB with it mounted, because the whole catalog spread over 83 slice modules compresses worse than one module.useTranslatorreads in 19 open pull requests, listed there.[breaking]: an app that renders<InternationalizationContext value={…}>(or its.Provider) instead ofInternationalizationProviderno longer drivesuseLocale,useDirectionanduseCollator, nor the components reading the locale through them. Migration: mount the provider (locale,messages,overrides;dirfordirection; an owntranslatebecomesmessagesoroverrides). Theastryx upgradecodemod is feat(cli): migrate a raw InternationalizationContext value to the provider #7969.Problem and solution fit
The generator also emits
generated-locales/en/<namespace>.generated.tsper namespace; the exports map gives./locales/en/*anulltarget.resolve.tstakes the English fallback per call. A core component renders withuseComponentTranslator(slice); it and the locale hooks read an internalTranslationRuntimeContextcarrying no catalog.InternationalizationContextkeeps its shape and its whole-catalog fallback,useTranslator()is unchanged, and the provider writes both contexts. A deprecation that kept a raw value working would have the hooks read the public context module, which imports the whole catalog, so every component would carry it again.Authority
resolve.ts's lookup order: step 5's fallback is now the caller's. Two owner decisions remain. The owners ofcomponent:InternationalizationProvider, which lists the context behavior as stable API, accept or reject retiring the raw-context path. The release owner plans 0.8.0, without whichcheck:changesetsrefuses the[breaking]entry (main declares 0.7.5).Change and evidence
i18n.source-build.test.mjs: the compiled resolver, runtime context and hooks import no catalog; the public context carries it.locale-exports.test.mjs:locales/en.generated.jsresolves through the package exports;locales/en/button.generated.jsisERR_PACKAGE_PATH_NOT_EXPORTED.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.Scope
Testing
pnpm build, the docsite previews,pnpm -F @astryxdesign/docsite build) passes.locale-exports.test.mjsresolves through the package's self-reference, so the docsite's search route traces 59.0 MB (main: 58.9 MB) instead of shipping the sandbox app (190.3 MB, over its 150 MB budget), which had failed every preview deploy of this branch.pnpm vitest run packages/core: 473 of 474 files and 13,145 tests passed;DropdownMenuLazySheet.test.tsxtimes out its 10 s import hook when it runs early in a full run, on main too (test(DropdownMenu): catch an eager sheet import when it is requested, not by a timer #8353 replaces that timer), and passes alone. The generator, catalog-check, build and lint-rule suites pass.check:i18n-catalog,check:i18n-runtime,build:i18n, core typecheck,eslint(includingno-raw-intl-locale),pnpm -F @astryxdesign/core build,sync:exports:check: clean.check:changesetsrefuses the[breaking]entry, as above.component-size.js: gzip JavaScript an import loads eagerly, React and StyleX included) at this head and at perf(i18n): every core component renders with its own English slice #7358's.