Skip to content

fix(i18n): render 404.astro when i18n is enabled#12525

Merged
ematipico merged 4 commits into
mainfrom
fix/404-with-i81n
Nov 26, 2024
Merged

fix(i18n): render 404.astro when i18n is enabled#12525
ematipico merged 4 commits into
mainfrom
fix/404-with-i81n

Conversation

@ematipico

Copy link
Copy Markdown
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

@changeset-bot

changeset-bot Bot commented Nov 25, 2024

Copy link
Copy Markdown

🦋 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
@codspeed-hq

codspeed-hq Bot commented Nov 25, 2024

Copy link
Copy Markdown

CodSpeed Performance Report

Merging #12525 will not alter performance

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

Summary

✅ 6 untouched benchmarks

@delucis delucis left a comment

Copy link
Copy Markdown
Member

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.

Comment thread .changeset/hot-dingos-dress.md Outdated
Comment thread packages/astro/src/vite-plugin-astro-server/route.ts
Comment thread pnpm-lock.yaml

@delucis delucis left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
@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