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): render 404.astro when i18n is enabled #12525

Merged
merged 4 commits into from
Nov 26, 2024
Merged

Conversation

ematipico
Copy link
Member

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

Copy link

changeset-bot bot commented Nov 25, 2024

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Nov 25, 2024
@ematipico ematipico force-pushed the fix/404-with-i81n branch 3 times, most recently from 3b2f790 to 8deb774 Compare November 25, 2024 16:03
Copy link

codspeed-hq bot commented Nov 25, 2024

CodSpeed Performance Report

Merging #12525 will not alter performance

Comparing fix/404-with-i81n (341d311) with main (8b0e36c)

Summary

✅ 6 untouched benchmarks

Copy link
Member

@delucis delucis left a 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.

.changeset/hot-dingos-dress.md Outdated Show resolved Hide resolved
pnpm-lock.yaml Outdated Show resolved Hide resolved
Copy link
Member

@delucis delucis left a 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!

@ematipico
Copy link
Member Author

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.

@ematipico ematipico merged commit cf0d8b0 into main Nov 26, 2024
15 checks passed
@ematipico ematipico deleted the fix/404-with-i81n branch November 26, 2024 15:21
@astrobot-houston astrobot-houston mentioned this pull request Nov 26, 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)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

404 page doesn't show with i18n enabled
3 participants