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

feat(i18n): disable redirect to default language #9638

Merged
merged 26 commits into from
Jan 17, 2024

Conversation

ematipico
Copy link
Member

Changes

Closes PLT-1318

This PR adds a new option to the i18n.routing object, called redirectToDefaultLocale, which comes into play when the option prefixDefaultLocale comes into play (it's true).

This new option is true by default to maintain the current behaviour of the routing, where Astro does a redirect from / to /<defaultLocale>.

When this option is set to false, the redirect isn't triggered anymore.

List of technical changes inside the PR:

  • I added a new schema validation, where we throw an error when redirectToDefaultLocale is false and prefixDefaultLocale is false. The reason why I added an error is that this combination of options doesn't have any effect, so I want to enforce a sense of correctness when configuring the i18n.
  • I refactored some types. Before, the routing name types were copied all over the code base, and it was unmaintainable. I created a type that is now imported everywhere.
  • The core of the logic occurs in the middleware.

Testing

I added new test cases that should cover DEV, SSG and SSR

Docs

/cc @withastro/maintainers-docs for feedback!

I will create a PR in the docs repository to update the page.

Copy link

changeset-bot bot commented Jan 8, 2024

🦋 Changeset detected

Latest commit: 598a1ae

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 github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Jan 8, 2024
packages/astro/src/core/config/schema.ts Outdated Show resolved Hide resolved
packages/astro/src/i18n/middleware.ts Outdated Show resolved Hide resolved
Comment on lines +85 to +86
case 'pathname-prefix-always': {
if (url.pathname === base + '/' || url.pathname === base) {
if (trailingSlash === 'always') {
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated: It looks like the prefix is only added when trailingSlash is set to always. Shouldn't the prefix also be added for other trailingSlash configurations?

Copy link
Member Author

@ematipico ematipico Jan 11, 2024

Choose a reason for hiding this comment

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

Right, although to check the other variants, we need build.format, which is a piece of information that we don't have in SSR (in the SSR manifest), that's why I only check always. Should we store build.format in the SSRManifest?

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok that makes sense. I think it's fine to store build.format then if it helps improve things here. We can definitely improve this in a later PR.

@ematipico ematipico force-pushed the feat/redirect-default-language branch from bd56a60 to 5a864a1 Compare January 11, 2024 12:59
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.

Thanks, @ematipico ! Made some suggestions here, based on the docs PR suggestions I made. See what you think of these, and edit to taste!

.changeset/cyan-grapes-suffer.md Outdated Show resolved Hide resolved
packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
ematipico and others added 13 commits January 12, 2024 14:18
…9666)

* Remove the header script before a view transition takes place to force a reload on the next page

* Add changeset

* Save another char
)

* fix(assets): Implement all hooks in the passthrough image service

* chore: changeset
… to toolbar/apps (#9647)

* refactor(toolbar): Rename every internal reference of overlay/plugins to toolbar/apps

* refactor: rename vite plugin

* fix: update import

* nit: add setting fallback
* fix(i18n): emit an error when the index isn't found

* changeset

* Update .changeset/proud-guests-bake.md

Co-authored-by: Sarah Rainsberger <[email protected]>

* rename

* Update packages/astro/src/core/errors/errors-data.ts

Co-authored-by: Florian Lefebvre <[email protected]>

---------

Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Florian Lefebvre <[email protected]>
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Jan 12, 2024
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.

Docs is happy! Looking forward to this, @ematipico ! 🥳

@ematipico ematipico force-pushed the feat/redirect-default-language branch from 07defb4 to a5fd580 Compare January 16, 2024 09:33
@github-actions github-actions bot added semver: minor Change triggers a `minor` release and removed pkg: integration Related to any renderer integration (scope) labels Jan 16, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

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.

Looks great 👍

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved
@ematipico ematipico merged commit f1a6126 into main Jan 17, 2024
3 checks passed
@ematipico ematipico deleted the feat/redirect-default-language branch January 17, 2024 13:25
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

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

@astrobot-houston astrobot-houston mentioned this pull request Jan 17, 2024
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 semver: minor Change triggers a `minor` release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants