Skip to content

fix: i18n 404 #12321

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.

Sign up for GitHub

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

Closed
wants to merge 5 commits into from
Closed

fix: i18n 404 #12321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cce1d0c
feat: add test fixture
florian-lefebvre Oct 28, 2024
97ad877
fix
florian-lefebvre Oct 28, 2024
47a3847
Merge branch 'main' into fix/i18n-404
florian-lefebvre Oct 28, 2024
e65fccb
chore: remove useless files
florian-lefebvre Oct 28, 2024
7f2b88e
fix: fixture
florian-lefebvre Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rude-frogs-hear.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a case where root 404 and 500 custom routes would not be taken into account when using i18n features
4 changes: 4 additions & 0 deletions packages/astro/src/i18n/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ export function requestHasLocale(locales: Locales) {

export function requestIs404Or500(request: Request, base = '') {
const url = new URL(request.url);
// needed otherwise the check becomes //404, which is always wrong
if (base === '/') {
base = '';
}
Comment on lines +24 to +27
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the correct fix, and I don't think the test is accurate either. We have prefixDefaultLocale: true, so I would expect /404 to be prefixed, hence /en/404. However, I don't expect /404 to exist. Having both seems incorrect.

However, if you think this is acceptable, we need to update the docs. For example, pages/index.astro is required even when we have prefixDefaultLocale: true and redictToDefaultLocale: true. So we need to document that if users use 404.astro, Astro will create a 404.html under each locale.

Also, I would add more test cases, for example if prefixDefaultLocale: false, I wouldn't expect to have /en/404.html. Also, I would create a test case without fallback.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both seems incorrect.

Why do you think that? If someone visits an invalid locale at the moment, they will see the host 404, not the user 404 no matter if it's set. So I think supporting this case is valid. But I'm not an i18n expert so let me know!

So we need to document that if users use 404.astro, Astro will create a 404.html under each locale.

This is not what this fix is about I believe, it's about supporting a custom root 404. That doesn't prevent users from having a custom 404 under a locale.

Also, I would add more test cases

Definitely agree, thanks for the pointers! I'm not super familiar with i18n in Astro so that helps a ton

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think that?

Because the PR description is empty, I find it difficult to understand what we're trying to fix 😅

If someone visits an invalid locale at the moment, they will see the host 404, not the user 404 no matter if it's set. So I think supporting this case is valid. But I'm not an i18n expert so let me know!

Since the host always renders /404.html, how can a developer tell the host to render /ru/404.html for an invalid URL under the /ru locale? (It's an example.)

I am not saying that the fix is incorrect, I am saying I am missing a lot of context from the PR. I believe the fix is correct, but I would add more tests to it, especially for existing fixtures where we don't use [lang] and we have fixed pages.


return url.pathname.startsWith(`${base}/404`) || url.pathname.startsWith(`${base}/500`);
}
Expand Down
18 changes: 18 additions & 0 deletions packages/astro/test/fixtures/i18n-routing-404/astro.config.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { defineConfig } from "astro/config";

export default defineConfig({
i18n: {
defaultLocale: 'en',
locales: ['en', 'ja', 'ko', 'zh'],
routing: {
prefixDefaultLocale: true,
fallbackType: 'rewrite',
redirectToDefaultLocale: true,
},
fallback: {
ja: 'en',
ko: 'en',
zh: 'en',
},
},
})
8 changes: 8 additions & 0 deletions packages/astro/test/fixtures/i18n-routing-404/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "@test/i18n-routing-404",
"version": "0.0.0",
"private": true,
"dependencies": {
"astro": "workspace:*"
}
}
11 changes: 11 additions & 0 deletions packages/astro/test/fixtures/i18n-routing-404/src/i18n/locales.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
export type Locales = Record<'en' | 'zh' | 'ja' | 'ko', string>;

export const LOCALE_NAMES: Locales = {
en: 'English',
zh: '中文',
ja: '日本語',
ko: '한국어',
};

export const KNOWN_LANGUAGE_CODES = Object.keys(LOCALE_NAMES);
export const defaultLang = 'en';
4 changes: 4 additions & 0 deletions packages/astro/test/fixtures/i18n-routing-404/src/pages/404.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<div>
<h1>404</h1>
<p>You're in the wrong part of town.</p>
</div>
14 changes: 14 additions & 0 deletions packages/astro/test/fixtures/i18n-routing-404/src/pages/[lang]/404.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
import { KNOWN_LANGUAGE_CODES } from "../../i18n/locales";
export const getStaticPaths = () => {
return KNOWN_LANGUAGE_CODES.flatMap((langKey) => ({
params: { lang: langKey },
}));
};
---

<div>
<h1>404</h1>
<p>You're in the wrong part of this localized town.</p>
</div>
Empty file added packages/astro/test/fixtures/i18n-routing-404/src/pages/index.astro
View file
Open in desktop
Empty file.
23 changes: 23 additions & 0 deletions packages/astro/test/i18n-routing.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1176,6 +1176,29 @@ describe('[SSG] i18n routing', () => {
});
});
});

describe('404s', () => {
/** @type {import('./test-utils').Fixture} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: './fixtures/i18n-routing-404/',
});
await fixture.build();
});

it('should output both root and localized 404s', async () => {
let html = await fixture.readFile('/en/404/index.html');
assert.equal(html.includes("You're in the wrong part of this localized town."), true);

html = await fixture.readFile('/ja/404/index.html');
assert.equal(html.includes("You're in the wrong part of this localized town."), true);

html = await fixture.readFile('/404.html');
assert.equal(html.includes("You're in the wrong part of town."), true);
});
});
});
describe('[SSR] i18n routing', () => {
let app;
Expand Down
6 changes: 6 additions & 0 deletions pnpm-lock.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading