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

chore(i18n): reworked fallback, docs, types and validation #8995

Merged
merged 3 commits into from
Nov 3, 2023

Conversation

ematipico
Copy link
Member

Changes

There was some issue with the types of i18n.fallback, and I added another level of validation to zod.

Testing

Added a test case to cover the new logic

Docs

/cc @withastro/maintainers-docs for feedback!

@sarah11918 we can use this PR to land the correct documentation for fallback

Copy link

changeset-bot bot commented Nov 3, 2023

⚠️ No Changeset found

Latest commit: f54dd21

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@ematipico ematipico requested a review from sarah11918 November 3, 2023 11:40
@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Nov 3, 2023
@ematipico ematipico removed the pkg: astro Related to the core `astro` package (scope) label Nov 3, 2023
@ematipico ematipico changed the title chore(i18n): reworked fallback docs chore(i18n): reworked fallback, docs, types and validation Nov 3, 2023
@ematipico ematipico force-pushed the chore/rework-fallback branch from fc93dbb to 71f94fb Compare November 3, 2023 11:45
@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 3, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

These are fabulous updates! Just a few minor suggestions. Thank you for hanging in there with me as I try to get all of these options straight in my head. 😅

Comment on lines 1419 to 1421
* The content fallback strategy when navigating to pages in non-default languages that do not exist. (e.g. a translated page has not been created).
*
* This a plain object, where a key is a locale that hasn't traslated pages, and the corresponding value is the locale where you want to redirect the missing page to.
Copy link
Member

@sarah11918 sarah11918 Nov 3, 2023

Choose a reason for hiding this comment

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

Suggested change
* The content fallback strategy when navigating to pages in non-default languages that do not exist. (e.g. a translated page has not been created).
*
* This a plain object, where a key is a locale that hasn't traslated pages, and the corresponding value is the locale where you want to redirect the missing page to.
* The content fallback strategy when navigating to pages in non-default languages that do not exist (e.g. a translated page has not been created).
*
* Use this object to declare a fallback `locale` for each language you support individually. If no fallback is specified, then unavailable pages will return a 404.

This looks great!

I made up that sentence at the end, because I think it helps to say what happens if you don't configure anything.

I think a common thing to want to do here will be to make the one default fallback, and not have to list every language out individually. So, if the above is true, that's also very useful for doing this! If the above is NOT true, then I think we need to tell them how to set one default language for all other languages, as I expect this to be a common use case.

EDIT: changed "then all languages listed in locales will redirect the page to your defaultLocale"

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason why I haven't put any default fallback is because:

  • we do want to avoid redirects as much as possible as default behaviour
  • it's possible that users don't require this feature (localised market, legal documents, etc.), so it would get more complicated to opt out of the default behaviour

About this suggestion:

If no fallback is specified, then all languages listed in locales will redirect the page to your defaultLocale.

As for now, we won't redirect to the defaultLocale, which means that if the user doesn't set a fallback for a locale, the missing pages will return 404.

We can explore a redirect behaviour later on

Copy link
Member

Choose a reason for hiding this comment

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

Updated the comment (and the other one) with acceptable placeholder language that I'll check on! This is good enough to merge!

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM for the logic changes.

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Approving and my task is to make sure we're saying "returns a 404" properly and consistently before publishing! Great changes here!

@ematipico ematipico force-pushed the chore/rework-fallback branch from 71f94fb to 177ee1f Compare November 3, 2023 15:55
@ematipico ematipico merged commit 745d94e into feat/i18n-routing Nov 3, 2023
@ematipico ematipico deleted the chore/rework-fallback branch November 3, 2023 15:59
ematipico added a commit that referenced this pull request Nov 6, 2023
ematipico added a commit that referenced this pull request Nov 6, 2023
ematipico added a commit that referenced this pull request Nov 7, 2023
ematipico added a commit that referenced this pull request Nov 8, 2023
ematipico added a commit that referenced this pull request Nov 8, 2023
ematipico added a commit that referenced this pull request Nov 8, 2023
ematipico added a commit that referenced this pull request Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants