-
-
Notifications
You must be signed in to change notification settings - Fork 501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for Astro.currentLocale
#1841
Conversation
* main: i18n(zh-cn): Update docs about synced-tabs (withastro#1834) i18n(zh-cn): Update some docs about withastro#1620 & withastro#1613 (withastro#1835) Add more diagnostic help to error messages thrown by `<Steps>` (withastro#1838) i18n(zh-cn): Update components.mdx (withastro#1836) i18n(zh-cn): Update community-content.mdx (withastro#1833) Improve type checking job (withastro#1831) [ci] format [ci] release (withastro#1832) i18n(ru): update ru.json (withastro#1826) Fix `<Tabs>` sync issue with inconsistent use of `icon` on `<TabItem>` components (withastro#1811) Enable `BASE_URL` tests (withastro#1828)
🦋 Changeset detectedLatest commit: 8b74599 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
* main: i18n(zh-cn): Update `components.mdx` translation (withastro#1852)
Updated the PR, bumped the required Astro version to 4.8.0 to match #1846 and also updated the code to handle the Astro i18n manual routing strategy added in Astro 4.6.0. |
* main: (45 commits) i18n(tr): add `showcase.mdx` (withastro#1896) [ci] format i18n(zh-cn): Update `authoring-content.md` (withastro#1901) [ci] format [ci] release (withastro#1897) [ci] format i18n(tr): update `authoring-content.md` (withastro#1887) Micro-optimization: clean up `<details>` CSS (withastro#1892) [ci] format i18n(tr): update `components.md` file (withastro#1889) [ci] format i18n(tr): add css-and-tailwind (withastro#1893) [ci] format [ci] release (withastro#1890) Adds custom styles for `<details>` and `<summary>` elements in Markdown content (withastro#1735) Update @astrojs/mdx to v3 (withastro#1846) [ci] format [ci] release (withastro#1869) Add BlueSky social icon (withastro#1871) [ci] format ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the very thorough work on this @HiDeoo! Especially with all the tricky mapping between the two config shapes. I think this is looking very good. Most of my review comments are internal details, so could also be handled in follow-up work if you preferred.
Going to think a bit about documentation now.
packages/starlight/utils/i18n.ts
Outdated
/** Check if the Starlight built-in default locale is used in a Starlight config. */ | ||
function isUsingBuiltInDefaultLocale(config: StarlightConfig) { | ||
return ( | ||
!config.isMultilingual && | ||
config.locales === undefined && | ||
config.defaultLocale.label === BuiltInDefaultLocale.label && | ||
config.defaultLocale.lang === BuiltInDefaultLocale.lang && | ||
config.defaultLocale.dir === BuiltInDefaultLocale.dir | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a little bit fragile. I wouldn’t be opposed to exposing something in the user config schema directly where we have that information instead of trying to infer this here. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love it. I was not super happy about this function but totally missed that opportunity, thanks a lot.
packages/starlight/utils/i18n.ts
Outdated
if (astroI18nConfig.fallback) { | ||
throw new AstroError( | ||
'Starlight is not compatible with the `fallback` option in the Astro i18n configuration.', | ||
'Starlight uses its own fallback strategy showing readers content for a missing page in the default language.\nSee more at https://starlight.astro.build/guides/i18n/#fallback-content' | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this is definitely the case? I would expect fallback
not to break Starlight — we’d just ignore it — so it could maybe still be used for other pages in the site?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I guess this hints that my wording is not good at all and the decision to have this check too maybe.
You are totally right and my idea was that a user could maybe think that the Astro fallback
option could control Starlight behavior too. But I guess indeed, the use case you mentioned could be a valid one too so I guess we should just remove this check.
packages/starlight/utils/i18n.ts
Outdated
if (!label || lang === label) throw new Error('Label not found.'); | ||
return { | ||
label: label[0]?.toLocaleUpperCase(locale) + label.slice(1), | ||
// `textInfo` is not part of the `Intl.Locale` type but is available in Node.js 18.0.0+. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be worth a // @ts-expect-error
here instead of casting the type? That way we might know once TypeScript fixes this? Could also cause issues though I guess if the fix relies on a specific TS version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, altho we still need a cast to explicitly get 'ltr' | 'rtl'
.
packages/starlight/utils/i18n.ts
Outdated
// This should never happen as Astro validation should prevent this case. | ||
if (!defaultAstroLocale) throw new Error('Astro default locale not found.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we ask for new issues in case anyone ever hits this? Or do you think it’s literally impossible and the throw
is just for typing?
// This should never happen as Astro validation should prevent this case. | |
if (!defaultAstroLocale) throw new Error('Astro default locale not found.'); | |
// This should never happen as Astro validation should prevent this case. | |
if (!defaultAstroLocale) { | |
throw new AstroError( | |
'Astro default locale not found.', | |
'This should never happen. Please open a new issue: https://github.com/withastro/starlight/issues/new' | |
); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Neat, I only added the bug report template to the URL query params if that's fine.
Description
This PR adds support for using
Astro.currentLocale
in Starlight, and also technically adds support for using an Astro i18n configuration instead of a Starlight i18n configuration.First, a new "concept" of Starlight built-in default locale has been introduced. This matches the current English one, except now it's something generated once, based on a BCP 47 tag, and can be used everywhere in the codebase instead of hardcoding
'en'
,'English'
or'ltr'
everywhere.Then, 4 main possibilities are supported when processing both i18n configurations:
A few additional details:
v4.4.5
v4.8.0
just like Update @astrojs/mdx to v3 #1846 so that the following PRs can also be relied upon:Astro.request.headers
is not available in "static" output mode astro#10196test.each
, etc. but I think considering it's a lot of data massaging with internal shapes, with pretty different and identified cases, I personally found that the extra verbosity was a bit better for readability in this case.Regarding the use of an Astro i18n configuration, some inference is done when it comes to locale data, such as labels and text direction. This is due to the fact that Astro doesn't provide a way to specify these in the configuration, so they are inferred using
Intl
features compatible with the minimum Node.js version supported by Astro.For comparison, in the Starlight documentation, here is a language selector labels comparison between the current Starlight i18n configuration and the equivalent inferred Astro i18n configuration:
There are some differences but the code is pretty similar to #1012 where we already got a lot of approvals, so I would think this should be fine, and if needed, people can always switch to the Starlight i18n configuration for more control.
Remaining tasks
Astro.currentLocale
debug infos inpackages/starlight/components/Banner.astro