Repository navigation
Mount i18n() before pages() in the Hono handler order - #18253
Conversation
🦋 Changeset detectedLatest commit: 4d235d8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
astro-author owns this pull requestFactory's code author persona works on this branch when a maintainer or Factory's reviewer requests changes, or when checks fail: it pushes follow-up commits, replies to review threads, and asks for another review. It never merges. Unassign astro-author to take the pull request back; reassign it to start a fresh budget. Rounds used: 0 of 5 — astro-author (Factory persona) |
There was a problem hiding this comment.
Verdict: approved. No blocking findings remain.
GitHub doesn't let Factory approve a pull request it opened, so this verdict is recorded as a comment.
Findings
No findings.
Merge Requirement Assessment
Status: satisfied
-
Mount
i18n()beforepages()in the documented Hono handler order (#18228). The order is swapped in two places:examples/advanced-routing/src/fetch.tsand the JSDoc usage snippet inpackages/integrations/cloudflare/src/hono.ts. This matches howpackages/astro/src/core/hono/index.tsworks:pages()returnsfetchPages(...)and never callshonoNext(), so any handler registered after it never runs.i18n()awaitshonoNext()first and then replacescontext.reswith the result offetchI18n(...). It only works when it wrapspages().
-
Regression coverage for the redirect behavior. The new
packages/astro/test/units/hono/i18n.test.ts:- builds an app with
strategy: 'pathname-prefix-always' - mounts
i18n()beforepages() - asserts that
GET /returns302withLocation: /en/
With the old order,
i18n()never runs, so the response would be the rendered page and both assertions would fail. The test matches the existing unit-test conventions (node:test,node:assert/strict, imports from builtdist/, thecreateTestApp/createPagemocks). It is picked up by thetest/units/**/*.test.tsglob, andhonois already a devDependency ofastro. - builds an app with
Review Status
Verdict: ready to merge based on static review
Context: PR #18253 plus its description, which links issue #18228
Scope: the full PR diff from base 1e5523d to head 4d235d8, covering 4 files
Changeset: present and covers @astrojs/cloudflare (patch). This is the only published package with a non-test change. The astro change is a test only, and examples/ doesn't need a changeset.
Limitations: I couldn't search the whole repository for other docs or examples that still show the old pages() then i18n() order. I only read the changed files and the files they directly depend on.
Validation: Static review only. I didn't run any project code, tests, builds, or checks.
GitHub context: collected
Git scope: PR base and head verified
This review was made by an LLM. The analysis may be wrong, and reports might be incorrect.
Changes
i18n()andpages()inexamples/advanced-routing/src/fetch.tsand the JSDoc snippet inpackages/integrations/cloudflare/src/hono.ts.pages()returns its response without calling Hono'snext(), so any handler registered after it never runs.i18n()must come beforepages()so it can post-process the page response and allowfinalizeI18nto issue redirects (e.g.GET /→302 /en/whenprefixDefaultLocaleandredirectToDefaultLocaleare both enabled). The wrong order also triggered Astro's runtime warning that the i18n handler was never called.Testing
packages/astro/test/units/hono/i18n.test.ts, which asserts that a project withprefixDefaultLocale: trueandredirectToDefaultLocale: truereturns302withLocation: /en/wheni18n()is mounted beforepages(). The test fails with the old (reversed) order, confirming the bug and the fix.Docs
examples/advanced-routingentrypoint.Closes #18228