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

Fix i18n current locale #12839

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mtwilliams-code
Copy link
Contributor

@mtwilliams-code mtwilliams-code commented Dec 27, 2024

Changes

Testing

Adds similar logic to computeCurrentLocale as was used in #12709 to handle SSR manifests with fallback routes.
Welcome feedback from a maintainer on whether there may be a more elegant solution to solve both of these - I dont have enough background on the decision process behind having such different manifests and i18n implementation between dev/SSR/SSG.

Docs

Just corrects a bug and inconsistent behavior across modes. No new docs needed.

Copy link

changeset-bot bot commented Dec 27, 2024

🦋 Changeset detected

Latest commit: 048dd65

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 the pkg: astro Related to the core `astro` package (scope) label Dec 27, 2024
@mtwilliams-code mtwilliams-code marked this pull request as ready for review December 27, 2024 20:15
Copy link

codspeed-hq bot commented Dec 27, 2024

CodSpeed Performance Report

Merging #12839 will not alter performance

Comparing mtwilliams-code:fix-i18n-currentLocale (5cdb8a3) with main (70a9f0b)

Summary

✅ 4 untouched benchmarks

Comment on lines +596 to +600
(routeData.pattern.test(url.pathname)
? routeData
: routeData.fallbackRoutes.find((fallbackRoute) =>
fallbackRoute.pattern.test(url.pathname),
)) ?? routeData;
Copy link
Member

Choose a reason for hiding this comment

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

In the past we used these operators and they made the readability worse, and we decided to use simple if/else. We would appreciate it if we keep using them

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Will rewrite the logic here.

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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants