-
-
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(i18n): render 404.astro
when i18n is enabled
#12525
Conversation
🦋 Changeset detectedLatest commit: 341d311 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 |
3b2f790
to
8deb774
Compare
CodSpeed Performance ReportMerging #12525 will not alter performanceComparing Summary
|
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.
I’m not really familiar with how the routing here works, but the header idea seems OK to me in principle. I spotted a few details, but don’t really impact the core changes I don’t think.
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
b27341b
to
341d311
Compare
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.
With the same caveats as the previous review that I don’t really know my way around this code, LGTM!
The use of the headers is weird (it's internal code). I'll see if I can find a better way to deal for these internal cases. |
Changes
Closes PLT-2671
Closes #12509
The issue was caused by the i18n middleware doing all it's "routing strategy checks" when rendering the
404.astro
component. This should be an expection, because all the "routing strategy checks" were already done when rendering the non-existent route.To fix the issue, we have an internal header that we attach to a
Response
. We check that header at the very beginning of the middleware.Additional change
I removed the recursions that we use in development, which isn't needed, and it should be things a bit more easy to debug. To remove the recursion, we just create a new
RenderContext
with the/404
route, and render it.Testing
Tested it against the issue of reproduction. Current tests should pass. Added a new test.
Docs
N/A