Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for Astro.currentLocale #1841

Merged
merged 20 commits into from
Jun 5, 2024
Merged

Add support for Astro.currentLocale #1841

merged 20 commits into from
Jun 5, 2024

Conversation

HiDeoo
Copy link
Member

@HiDeoo HiDeoo commented May 8, 2024 •
edited by delucis

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:

  • If no Astro and Starlight i18n configurations are provided, the built-in default locale is used in Starlight and a matching Astro i18n configuration is generated/used.
  • If only a Starlight i18n configuration is provided, an equivalent Astro i18n configuration is generated/used.
  • If only an Astro i18n configuration is provided, the Starlight i18n configuration is updated to match it.
  • If both an Astro and Starlight i18n configurations are provided, an error is thrown.

A few additional details:

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:

Starlight i18n config Astro i18n config
SCR-20240508-jgwp SCR-20240508-jhmh

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

  • Switch back to the Starlight i18n configuration in the documentation
  • Remove the Astro.currentLocale debug infos in packages/starlight/components/Banner.astro
  • Figure out what and how much should be documented
  • Write the documentation
  • Add a changeset

HiDeoo added 7 commits May 5, 2024 10:54
@HiDeoo
chore: bump required astro version to 4.4.5
6b3d657
@HiDeoo
fix: refactor UserI18nSchema type
4c1af27
@HiDeoo
refactor: add built-in default locale
bbb95ff
@HiDeoo
feat: generate and use Astro i18n config
2ee987e
@HiDeoo
feat: add Astro → Starlight i18 config support
3dbedb2
@HiDeoo
feat: use Astro i18n config in docs for demo purposes
f8e36b2
@HiDeoo
Merge branch 'main' into hd-refactor-astro-i18n
c193d55 
* 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-bot changeset-bot
Copy link

changeset-bot bot commented May 8, 2024 •
edited

🦋 Changeset detected

Latest commit: 8b74599

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

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

@vercel Vercel
Copy link

vercel bot commented May 8, 2024 •
edited

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jun 5, 2024 5:21pm

@github-actions github-actions bot added 📚 docs Documentation website changes 🌟 core Changes to Starlight’s main package 🌟 tailwind Changes to Starlight’s Tailwind package labels May 8, 2024
@HiDeoo
Copy link
Member Author

HiDeoo commented May 14, 2024

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.

@HiDeoo
chore: bump astro version to 4.8.6
e731ab3
@HiDeoo
Merge branch 'main' into hd-refactor-astro-i18n
897f0bf 
* 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
  ...
@HiDeoo
test: update i18n-non-root-single-locale test
b079bd7
@github-actions github-actions bot removed the 🌟 tailwind Changes to Starlight’s Tailwind package label May 20, 2024
@HiDeoo HiDeoo marked this pull request as ready for review May 20, 2024 16:30
@HiDeoo HiDeoo mentioned this pull request May 22, 2024
Draft
10 tasks
delucis
delucis reviewed Jun 4, 2024
View reviewed changes
Copy link
Member

@delucis delucis left a 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/index.ts Show resolved Hide resolved
packages/starlight/utils/i18n.ts Outdated Show resolved Hide resolved
packages/starlight/utils/i18n.ts Outdated Show resolved Hide resolved
packages/starlight/utils/i18n.ts Outdated
Comment on lines 147 to 156
/** 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
);
}
Copy link
Member

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?

Copy link
Member Author

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 Show resolved Hide resolved
packages/starlight/utils/i18n.ts Outdated
Comment on lines 30 to 35
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'
);
}
Copy link
Member

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?

Copy link
Member Author

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 Show resolved Hide resolved
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+.
Copy link
Member

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?

Copy link
Member Author

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
Comment on lines 106 to 107
// This should never happen as Astro validation should prevent this case.
if (!defaultAstroLocale) throw new Error('Astro default locale not found.');
Copy link
Member

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?

Suggested change
// 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'
);
}

Copy link
Member Author

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.

@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Jun 5, 2024 •
edited

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/i18n.mdx Source changed, localizations will be marked as outdated.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@HiDeoo
Copy link
Member Author

HiDeoo commented Jun 5, 2024

Thanks a lot for the great review as usual, super appreciated and very helpful! 🙌

Updated the PR with the suggested changes and also took a first stab at documenting the new changes.

delucis
delucis reviewed Jun 5, 2024
View reviewed changes
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a small suggestion to simplify the docs, but otherwise I think this looks good to me! We can always revisit documentation later once we see what people are finding unclear.

I’ll take care of reverting the TODOs now.

docs/src/content/docs/guides/i18n.mdx Outdated Show resolved Hide resolved
@delucis delucis added the 🌟 minor Change that triggers a minor release label Jun 5, 2024
delucis
delucis approved these changes Jun 5, 2024
View reviewed changes
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving! These docs work for me, and we will have an opportunity to expand on them probably when working on #1923.

Thanks again @HiDeoo! Stellar work 🌟

@HiDeoo HiDeoo merged commit ee0cd38 into withastro:main Jun 5, 2024
12 checks passed
@HiDeoo HiDeoo deleted the hd-refactor-astro-i18n branch June 5, 2024 17:23
@astrobot-houston astrobot-houston mentioned this pull request Jun 5, 2024
Merged
thomasbnt added a commit to thomasbnt/starlight that referenced this pull request Jun 5, 2024
@thomasbnt
i18n(fr): Update guides/i18n.mdx from withastro#1841
39cae20
@thomasbnt thomasbnt mentioned this pull request Jun 5, 2024
Merged
@huyikai huyikai mentioned this pull request Jun 6, 2024
Merged
@jsparkdev jsparkdev mentioned this pull request Jun 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@delucis delucis delucis approved these changes

Assignees
No one assigned
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes 🌟 minor Change that triggers a minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@HiDeoo @astrobot-houston @delucis