feat(i18n): expand fallback system by ematipico · Pull Request #11677 · withastro/astro

ematipico · 2024-08-12T10:15:46Z

Changes

This PR adds a new feature for i18n. A new configuration called i18n.routing.fallbackType is added. The value of this new field is "redirect" | "rewrite".

The "redirect" variant is the default value and matches the current behaviour of the fallback system. With "rewrite", we use the .rewrite() function that was stabilised two minors ago

This PR also fixes a case where RenderContent.pathname was incorrectly computed. The pathname isn't supposed to have the base in its URL. This led to some issues that were fixed in this PR.

Testing

I added new test for the rewrite.
I updated a test for the fix, which contained an incorrect behaviour.

Docs

withastro/docs#9133

changeset-bot · 2024-08-12T10:15:51Z

🦋 Changeset detected

Latest commit: a42d513

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

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

github-actions

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

ematipico · 2024-08-12T10:17:51Z

packages/astro/test/fixtures/rewrite-trailing-slash-never/src/pages/foo.astro

@@ -1,3 +1,3 @@
 ---
-return Astro.rewrite("/")
+return Astro.rewrite("/base")


This was an incorrect rewrite. The fix uncovered this, and I changed it. The fixture contains a base which means doing / should result to a 404 instead.

.changeset/lazy-feet-join.md

packages/astro/src/core/routing/rewrite.ts

sarah11918 · 2024-08-19T18:05:29Z

Re: the patch changeset, is it possible that people tried to "work around" this bug, and may have to update their projects now accordingly?

e.g. Could someone have written return Astro.rewrite("/") and expect it to include the base, thinking that this is just how the feature worked, and now they need to add it in manually to preserve their existing behaviour? If so, we'd probably need to explain a little more in that changeset.

Am waiting to look at the rest of the docs too closely while I see Matthew still asking questions!

ematipico · 2024-08-21T09:36:32Z

@sarah11918 I have updated the changeset regarding the bug fix.

ematipico

!preview

.changeset/blue-pens-divide.md

packages/astro/test/fixtures/i18n-routing-fallback/astro.config.mjs

packages/astro/src/core/routing/rewrite.ts

sarah11918 · 2024-08-22T13:56:45Z

packages/astro/src/@types/astro.ts

 					redirectToDefaultLocale?: boolean;

+					/**
+					 * @docs


I have some questions first here @ematipico

Is this top-level i18n config? Is this entry in the correct place? Right now, it would be showing up between two i18n.routing.* entries.

Shouldn't it also be near i18n.fallback? And, this doesn't replace it, but is a separate, same-level configuration? (that's up around line 1567).

I think we should also see an example of both this and fallback configured together in one code sample, since I'm assuming they'd need to work together. Without any i18n.fallback configured, you default to showing 404s for pages that don't exist, so you can't have a strategy?

Is this top-level i18n config? Is this entry in the correct place? Right now, it would be showing up between two i18n.routing.* entries.

Shouldn't it also be near i18n.fallback? And, this doesn't replace it, but is a separate, same-level configuration? (that's up around line 1567).

It should be i18n.routing.fallbackType. I will update the code, because it's incorrect.

I think we should also see an example of both this and fallback configured together in one code sample, since I'm assuming they'd need to work together. Without any i18n.fallback configured, you default to showing 404s for pages that don't exist, so you can't have a strategy?

I updated the docs with an example here 2447fb9 (#11677)

sarah11918

This looks great @ematipico ! It's so exciting to see this feature continue to develop and allow people to control more and more!

I left some suggestions below for your consideration, so as always, correct what needs correcting and see what you think!

.changeset/blue-pens-divide.md

packages/astro/src/@types/astro.ts

.changeset/lazy-feet-join.md

sarah11918 · 2024-08-22T22:59:08Z

Should this be added to the 4.15 milestone? I have it in mine for docs!

sarah11918

This looks great @ematipico !

The content here is now good, but I think we could also update i18n.fallback itself now to make sure that there's reference to this additional configuration someone might make besides just setting that.

i18n.fallback is no longer just "the fallback strategy" because the strategy now combines these two (separated in diffrerent parts of the API) settings.

We could update the entry for i18n.fallback to be something like:

An option to configure the fallback language content when navigating to pages that do not exist (e.g. a translated page has not been created).

Use this object to declare a fallback locale route for each language you support. If no fallback is specified, then unavailable pages will return a 404.

You can also optionally configure i18n.routing.fallbackType, to select whether to execute this fallback content as a redirect (default) or a rewrite.

What do you think about something like this?

I will also note, entirely up to you to consider/ignore as you see fit:

Even though this is a routing action, having it as a configuration on i18n.routing instead of i18n.fallback feels like a clunky/awkward story for docs to tell. I think needing to cross-link these two settings, where one depends on having the other set, because these settings are in different parts of the API feels a bit strange.

If I got to wave my magic wand (and, admitting I know nothing about what's underlying here), i18n.fallback.type (or i18n.fallback.redirect as a boolean, if you're not expecting any other option here) feels like a much more natural thing for users to configure.

packages/astro/src/@types/astro.ts

ematipico · 2024-08-27T07:46:07Z

If I got to wave my magic wand (and, admitting I know nothing about what's underlying here), i18n.fallback.type (or i18n.fallback.redirect as a boolean, if you're not expecting any other option here) feels like a much more natural thing for users to configure.

I know, and I thought about it at the very beginning. However, i18n.fallback contains a list of locales and having a type doesn't fit in. That's why I changed it for i18n.fallbackType.

sarah11918

Just an extra space, but approving for docs! Thank you @ematipico !

.changeset/blue-pens-divide.md

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

merge day

github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review semver: minor Change triggers a `minor` release labels Aug 12, 2024

github-actions bot previously requested changes Aug 12, 2024

View reviewed changes

ematipico commented Aug 12, 2024

View reviewed changes

ematipico force-pushed the feature/fallback-rewrite branch from 71e2a41 to 996ad3c Compare August 19, 2024 13:29

ematipico mentioned this pull request Aug 19, 2024

feat(i18n): document new i18n logic withastro/docs#9133

Merged

ematipico marked this pull request as ready for review August 19, 2024 13:40

matthewp reviewed Aug 19, 2024

View reviewed changes

.changeset/lazy-feet-join.md Outdated Show resolved Hide resolved

matthewp reviewed Aug 19, 2024

View reviewed changes

packages/astro/src/core/routing/rewrite.ts Outdated Show resolved Hide resolved

ematipico requested review from matthewp and sarah11918 August 21, 2024 09:36

ematipico force-pushed the feature/fallback-rewrite branch 2 times, most recently from e0ba330 to 34df8db Compare August 21, 2024 13:25

withastro deleted a comment from github-actions bot Aug 21, 2024

ematipico commented Aug 21, 2024

View reviewed changes

bluwy reviewed Aug 22, 2024

View reviewed changes

.changeset/blue-pens-divide.md Outdated Show resolved Hide resolved

packages/astro/test/fixtures/i18n-routing-fallback/astro.config.mjs Outdated Show resolved Hide resolved

packages/astro/src/core/routing/rewrite.ts Show resolved Hide resolved

ematipico requested a review from bluwy August 22, 2024 12:18

bluwy approved these changes Aug 22, 2024

View reviewed changes

sarah11918 reviewed Aug 22, 2024

View reviewed changes

ematipico requested a review from sarah11918 August 22, 2024 14:46

sarah11918 reviewed Aug 22, 2024

View reviewed changes

ematipico added this to the 4.15 milestone Aug 23, 2024

sarah11918 reviewed Aug 26, 2024

View reviewed changes

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

sarah11918 approved these changes Aug 27, 2024

View reviewed changes

.changeset/blue-pens-divide.md Outdated Show resolved Hide resolved

github-actions bot added the docs pr label Aug 28, 2024

ematipico and others added 13 commits August 28, 2024 10:40

feat(i18n): expand fallback system

a7391ef

rebase

7827980

apply feedback

872dae3

better changeset

e273fab

update tests too

fb1192e

apply feedback

67c2030

Update .changeset/blue-pens-divide.md

b0bb20d

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>

update docs

b95f32d

nitpick

0d6884a

Apply suggestions from code review

80b1c6a

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

Update .changeset/blue-pens-divide.md

6f54756

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

Apply suggestions from code review

2089a3f

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

Update .changeset/blue-pens-divide.md

f712640

Co-authored-by: Sarah Rainsberger <sarah@rainsberger.ca>

ematipico force-pushed the feature/fallback-rewrite branch from 4febc1b to f712640 Compare August 28, 2024 09:45

fix regression

a42d513

ematipico merged commit cb356a5 into main Aug 28, 2024

ematipico deleted the feature/fallback-rewrite branch August 28, 2024 11:16

astrobot-houston mentioned this pull request Aug 29, 2024

[ci] release #11857

Merged

Uh oh!

Conversation

ematipico commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Docs

Uh oh!

changeset-bot bot commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sarah11918 commented Aug 19, 2024

Uh oh!

ematipico commented Aug 21, 2024

Uh oh!

ematipico left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarah11918 Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

ematipico Aug 22, 2024

Choose a reason for hiding this comment

Uh oh!

sarah11918 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sarah11918 commented Aug 22, 2024

Uh oh!

sarah11918 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ematipico commented Aug 27, 2024

Uh oh!

sarah11918 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ematipico commented Aug 12, 2024 •

edited

Loading

changeset-bot bot commented Aug 12, 2024 •

edited

Loading

ematipico Aug 12, 2024 •

edited

Loading

sarah11918 left a comment •

edited

Loading