-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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: return correct locale in root 404 and 500 page with i18n #12365
Conversation
|
computedLocale = computeCurrentLocale(url.pathname, locales, defaultLocale); | ||
} | ||
const pathname = | ||
routeData.pathname && routeData.pathname !== '/404' ? routeData.pathname : url.pathname; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 404 check isn't strong enough, because it fail for /404/
, for example. You should actually use routeData.pattern.test('/404')
. Also, while we are at it, we should include 500
too, since users can have a 500.astro
I think you should create a function in routing/match.ts
, calling is isRoute404or500()
, which accept a RouteData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I will make those changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added requested changes. PR is ready for re-review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ematipico PTAL!
CodSpeed Performance ReportMerging #12365 will not alter performanceComparing Summary
|
The test failing seems to be a fluke, and it's not related to your changes. Thank you for the fix! |
Changes
This PR fixes the issue where
Astro.currentLocale
was not correctly returning the locale for custom 404 and 500 pages in projects using i18n.Closes #12356
Testing
Manually verified in dev server and SSR.
Added Dev and SSR tests custom for vertifying locale in custom 404 pages.
Docs
It is not needed as it is a bug fix