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.

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

i18n overhaul #365

Merged
merged 50 commits into from
Apr 26, 2022
Merged

i18n overhaul #365

merged 50 commits into from
Apr 26, 2022

Conversation

delucis
Copy link
Member

@delucis delucis commented Apr 21, 2022

This PR takes most of the structural steps to get #359 off the ground.

Summary

It’s a big PR and touches a lot, so here’s a summary of what’s going on:

  • All non-English pages except for “Getting Started” have been removed. (They’re archived in old-translations/ for now for reference, but we can delete them at some point in the future.)

  • Every language now has its own directory in src/i18n. This includes translations for UI strings as well as the navigation sidebar (which used to live in src/config.ts).

  • Every language now uses the same navigation sidebar structure as English. If translations are found in src/i18n/{lang}/nav.ts, these will be used instead of the English label. If a page is not yet available in this language, the page is still generated, but using fallback content from the English version. These are marked with an EN suffix in the sidebar.

    For example, here’s /de/editor-setup/. Its sidebar text is translated — “Editor-Einrichtung” — but the page is not and shows a notice explaining this. This notice can be translated in src/i18n/{lang}/ui.ts.

    image
  • src/i18n/README.md contains a guide on how to contribute translations

  • A new script provides automated scaffolding of all of the required files for a new language: pnpm run add-language

    add-language.mov

Smaller details

  • 404 pages are now translatable in the same way as other UI strings and are generated automatically for each language

  • Bare language URLs redirect to Getting Started, e.g. /de/de/getting-started

  • Following feedback on Discord, this system uses consistent slugs across locales

  • The language switcher now preserves the current page when the language changes instead of always redirecting to “Getting Started”

  • Had to upgrade to the latest Astro beta to get some bug fixes for the markdown Content component.

  • SEO: added hreflang alternate links to connect up translations across languages and made sure that pages displaying fallback content use the English URL as their canonical.

Key areas for review

  • Evaluate src/pages/[lang]/[...fallback].astro. Is this approach OK for rendering the fallback pages? Any improvements possible?

  • I ran into a few performance hurdles, see adb545d for one detailed explanation.

  • What is still needed in src/i18n/README.md? We need a list of supported languages obviously, but otherwise?

  • Should this PR also prune the number of supported languages or will we do that later? Or, in the other direction, should this PR preserve potentially out-of-date translations for now instead of moving them to old-translations/?

  • How does the translation file structure in src/i18n feel? German (de) is a good language to check as it has bits of everything translated.

Constraints introduced

  • The fallback content system relies on Astro.glob, so all pages need to be .md files rather than .astro. Might be possible once the metadata returned by Astro.glob for Astro files is improved to expand that, but for now we’ll need to stick to Markdown.

  • Because slugs are shared across locales, Markdown files need to have the same name across the different language directories and these need to match the slugs in src/i18n/en/nav.ts. I haven't added any tooling around that, but we could.

@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for astro-docs-2 ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ff252d9
🔍 Latest deploy log https://app.netlify.com/sites/astro-docs-2/deploys/626816899fd8e2000873137c
😎 Deploy Preview https://deploy-preview-365--astro-docs-2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@delucis delucis mentioned this pull request Apr 21, 2022
24 tasks
Brings in some fixes for Markdown Content component
Something about how components are rendered goes wrong on build (not during dev) if we pass `Content` out of `getStaticProps` via props. Explicitly globbing the content again inside the page itself seems to fix this 🤷
@delucis delucis marked this pull request as ready for review April 23, 2022 17:20
We tag navigation sidebar entries as “fallback” content based on if the slug exists for a given language. Previously I did this using Vite’s `import.meta.glob('../pages/**/*.md)` to grab the URLs of all pages. Although this shouldn’t load content (you need `globEager` for that), this still seemed to trigger Astro processing *all* Markdown pages. Unsurprisingly, this was slow — HMR of ~10s for a single page edit 😬

I’ve replaced the glob by recursively reading the pages directory when needed using `fs.readdir`. Avoiding globbing also means we can target *only* the relevant language instead of all pages. That brings reloads back down to 100–200ms in dev.

There is a disadvantage: if new pages are added while running `dev`, the navbar may be cached for previously visited pages and because we’re not using `glob`, Vite/Astro don’t know we’re depending on files, so won’t invalidate that cache. Not sure if there’s a workaround? But still, this solution seems preferable to waiting 10 seconds each time you make an edit.
@sarah11918
Copy link
Member

Woo!

I went through and tested everything listed in the preview, looked at the new file structure and all that seems to be working well. 🥳 Pretty slick!

To address the points raised in "areas for review" and "constraints" here are my thoughts/observations!

Key Areas For Review

  1. Fallback: @tony-sull are you satisfied with the method used in src/pages/[lang]/[...fallback].astro ? Any improvements/suggestions you'd make?

  2. Performance Issue: Similarly, I'm tagging @tony-sull specifically on this note

  3. i18n README: I think this is absolutely enough to start! I might want us to eventually add (off the top of my head):

  • list of words NOT to translate (even just a simple guideline to start -- probably if it's a proper noun/starts with a capital letter in English, no translation necessary). This will also mean looping in Core to get their takes: SSR? Astro component script?

  • maybe guidance on PR titles e.g. [typo/code fix] vs [translation] once we get into a rhythm? or descriptions? Something that maybe gives a heads up as to whether this is a quick fix that doesn't necessarily require looping in translators/editors, but something that any Maintainer could probably just quickly address, even if they didn't know the language.

(There were a couple of sentences ending in quotation marks where I'd tuck the punctuation in, but there's nothing blocking here.)

  1. Languages/pages to include: Since we don't yet have a decision on the languages to support, and since this looks pretty darn ready to go... my recommendation would be:
  • KEEP EXISTING LANGUAGES (Getting Started) and don't worry about pruning yet if we are ready to approve this before a decision is made.
  • MOVE ALL TRANSLATIONS (other than Getting Started) and do not have any out-of-date (assume it's all out of date, even if some pages have been recently updated for very certain fixes)
  1. File Structure: I personally have no issues with the file structure. :)

Contraints

  1. All pages must be Markdown: I think the only non .md page in question is "Install"? Because of the tab switching? You don't seem to have had a problem recreating the functionality in this PR, by creating 2 separage Markdown pages now. Does it require extra work for the translators or any special guidance? And, since we don't have another language done to see what this looks like in, do we have a way to translate the tab titles in the tab components? (Automatic CLI and Manual Setup) I'll ping @FredKSchott to look at this new set up for /en/install pages in particular, since he made ethe original pages.

  2. It doesn't bother me that slugs are shared and Markdown files need to have the same names, she says, non-translating...

}

const { fallback } = Astro.params
const englishPages = await Astro.glob('../en/**/*.md');
Copy link
Member

Choose a reason for hiding this comment

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

FYI you should be able to do this:

const { fallback } = Astro.params
const foo = await import(`../en/${fallback}.md`);

however, it looks like its not returning the right thing due to a Vite bug, so Astro.glob() is fine until that gets resolved.

Copy link
Member

@FredKSchott FredKSchott left a comment

Choose a reason for hiding this comment

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

@delucis this looks great, thank you for tackling something so huge in scope with such an attention to detail! Those two things are usually not easy to do together :)

I've given it a look and I don't have too many comments! Given the scope I'd prefer to merge this sooner rather than later to prevent merge conflicts.

I want to take a look at some of the perf issues you've run into, and maybe suggest a couple of follow up PRs. Sadly, some of them may be in Vite instead of Astro, and how they eagerly scan/build files in a project.

@FredKSchott FredKSchott merged commit b268cae into withastro:main Apr 26, 2022
@FredKSchott
Copy link
Member

Suggested follow-ups, based on the convo above:

  • a lint script that warns if a language file (ex: /de/foobar.md) doesn't have a matching English file (ex: /en/foobar.md). We could run this in CI to make sure all PRs are passing.
  • some perf improvements (@FredKSchott to PR)
  • decide on final official languages to support (@FredKSchott to post in Discord)

RE project layout: At this level of complexity, I would suggest that we begin to look into moving all markdown content out of src/pages and into a separate src/docs or src/content directory, and then converting all src/pages/ to be just the page "shells" that are responsible for importing that content using Astro.glob(). But I'm not sure if it sounds like we have perf issues lingering that might make this hard, based on the above. Maybe more investigation is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants