From 2b4e4512261bd517421e4be9446797e25b6574a9 Mon Sep 17 00:00:00 2001 From: Chris Swithinbank Date: Tue, 19 Nov 2024 12:39:19 +0100 Subject: [PATCH 1/4] Respect `trailingSlash: 'never'` for root URLs in language switcher --- .../i18n-root-locale/localizedUrl.test.ts | 65 +++++++++++++++---- packages/starlight/components/Head.astro | 3 +- .../starlight/components/LanguageSelect.astro | 3 +- packages/starlight/utils/localizedUrl.ts | 10 ++- 4 files changed, 64 insertions(+), 17 deletions(-) diff --git a/packages/starlight/__tests__/i18n-root-locale/localizedUrl.test.ts b/packages/starlight/__tests__/i18n-root-locale/localizedUrl.test.ts index 51ed335de44..326a6c3d692 100644 --- a/packages/starlight/__tests__/i18n-root-locale/localizedUrl.test.ts +++ b/packages/starlight/__tests__/i18n-root-locale/localizedUrl.test.ts @@ -4,73 +4,110 @@ import { localizedUrl } from '../../utils/localizedUrl'; describe('with `build.output: "directory"`', () => { test('it has no effect if locale matches', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it has no effect if locale matches for index', () => { const url = new URL('https://example.com/en/'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it changes locale to requested locale', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, 'ar').href).toBe('https://example.com/ar/guide/'); + expect(localizedUrl(url, 'ar', 'ignore').href).toBe('https://example.com/ar/guide/'); }); test('it changes locale to requested locale for index', () => { const url = new URL('https://example.com/en/'); - expect(localizedUrl(url, 'ar').href).toBe('https://example.com/ar/'); + expect(localizedUrl(url, 'ar', 'ignore').href).toBe('https://example.com/ar/'); }); test('it can change to root locale', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, undefined).href).toBe('https://example.com/guide/'); + expect(localizedUrl(url, undefined, 'ignore').href).toBe('https://example.com/guide/'); }); test('it can change from root locale', () => { const url = new URL('https://example.com/guide/'); - expect(localizedUrl(url, 'en').href).toBe('https://example.com/en/guide/'); + expect(localizedUrl(url, 'en', 'ignore').href).toBe('https://example.com/en/guide/'); + }); +}); + +describe('with `trailingSlash: "never"`', () => { + test('it has no effect if locale matches', () => { + const url = new URL('https://example.com/en/guide'); + expect(localizedUrl(url, 'en', 'never').href).toBe(url.href); + }); + + test('it has no effect if locale matches for index', () => { + const url = new URL('https://example.com/en'); + expect(localizedUrl(url, 'en', 'never').href).toBe(url.href); + }); + + test('it changes locale to requested locale', () => { + const url = new URL('https://example.com/en/guide'); + expect(localizedUrl(url, 'ar', 'never').href).toBe('https://example.com/ar/guide'); + }); + + test('it changes locale to requested locale for index', () => { + const url = new URL('https://example.com/en'); + expect(localizedUrl(url, 'ar', 'never').href).toBe('https://example.com/ar'); + }); + + test('it can change to root locale', () => { + const url = new URL('https://example.com/en/guide'); + expect(localizedUrl(url, undefined, 'ignore').href).toBe('https://example.com/guide'); + }); + + test('it can change from root locale', () => { + const url = new URL('https://example.com/guide'); + expect(localizedUrl(url, 'en', 'ignore').href).toBe('https://example.com/en/guide'); + }); + + test('it can change from root index', () => { + const url = new URL('https://example.com'); + expect(localizedUrl(url, 'en', 'never').href).toBe('https://example.com/en'); }); }); describe('with `build.output: "file"`', () => { test('it has no effect if locale matches', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it has no effect if locale matches for index', () => { const url = new URL('https://example.com/en.html'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it changes locale to requested locale', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, 'ar').href).toBe('https://example.com/ar/guide.html'); + expect(localizedUrl(url, 'ar', 'ignore').href).toBe('https://example.com/ar/guide.html'); }); test('it changes locale to requested locale for index', () => { const url = new URL('https://example.com/en.html'); - expect(localizedUrl(url, 'ar').href).toBe('https://example.com/ar.html'); + expect(localizedUrl(url, 'ar', 'ignore').href).toBe('https://example.com/ar.html'); }); test('it can change to root locale', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, undefined).href).toBe('https://example.com/guide.html'); + expect(localizedUrl(url, undefined, 'ignore').href).toBe('https://example.com/guide.html'); }); test('it can change to root locale from index', () => { const url = new URL('https://example.com/en.html'); - expect(localizedUrl(url, undefined).href).toBe('https://example.com/index.html'); + expect(localizedUrl(url, undefined, 'ignore').href).toBe('https://example.com/index.html'); }); test('it can change from root locale', () => { const url = new URL('https://example.com/guide.html'); - expect(localizedUrl(url, 'en').href).toBe('https://example.com/en/guide.html'); + expect(localizedUrl(url, 'en', 'ignore').href).toBe('https://example.com/en/guide.html'); }); test('it can change from root locale from index', () => { const url = new URL('https://example.com/index.html'); - expect(localizedUrl(url, 'en').href).toBe('https://example.com/en.html'); + expect(localizedUrl(url, 'en', 'ignore').href).toBe('https://example.com/en.html'); }); }); diff --git a/packages/starlight/components/Head.astro b/packages/starlight/components/Head.astro index d0fe067dc38..3a66e7c5253 100644 --- a/packages/starlight/components/Head.astro +++ b/packages/starlight/components/Head.astro @@ -1,5 +1,6 @@ --- import type { z } from 'astro/zod'; +import context from 'virtual:starlight/project-context'; import config from 'virtual:starlight/user-config'; import { version } from '../package.json'; import type { HeadConfigSchema } from '../schemas/head'; @@ -66,7 +67,7 @@ if (canonical && config.isMultilingual) { attrs: { rel: 'alternate', hreflang: localeOpts.lang, - href: localizedUrl(canonical, locale).href, + href: localizedUrl(canonical, locale, context.trailingSlash).href, }, }); } diff --git a/packages/starlight/components/LanguageSelect.astro b/packages/starlight/components/LanguageSelect.astro index 42999eac9b2..dd9dbb6c1d4 100644 --- a/packages/starlight/components/LanguageSelect.astro +++ b/packages/starlight/components/LanguageSelect.astro @@ -1,4 +1,5 @@ --- +import context from 'virtual:starlight/project-context'; import config from 'virtual:starlight/user-config'; import { localizedUrl } from '../utils/localizedUrl'; import Select from './Select.astro'; @@ -8,7 +9,7 @@ import type { Props } from '../props'; * Get the equivalent of the current page path for the passed locale. */ function localizedPathname(locale: string | undefined): string { - return localizedUrl(Astro.url, locale).pathname; + return localizedUrl(Astro.url, locale, context.trailingSlash).pathname; } --- diff --git a/packages/starlight/utils/localizedUrl.ts b/packages/starlight/utils/localizedUrl.ts index e6df9068ecd..49449cf769f 100644 --- a/packages/starlight/utils/localizedUrl.ts +++ b/packages/starlight/utils/localizedUrl.ts @@ -1,10 +1,15 @@ import config from 'virtual:starlight/user-config'; import { stripTrailingSlash } from './path'; +import type { AstroConfig } from 'astro'; /** * Get the equivalent of the passed URL for the passed locale. */ -export function localizedUrl(url: URL, locale: string | undefined): URL { +export function localizedUrl( + url: URL, + locale: string | undefined, + trailingSlash: AstroConfig['trailingSlash'] +): URL { // Create a new URL object to void mutating the global. url = new URL(url); if (!config.locales) { @@ -41,5 +46,8 @@ export function localizedUrl(url: URL, locale: string | undefined): URL { } // Restore base if (hasBase) url.pathname = base + url.pathname; + if (trailingSlash === 'never' && url.pathname.at(-1) === '/') { + url.pathname = url.pathname.slice(0, -1); + } return url; } From 3baac1a9267744dc3664c4b52ceb1a35097e61c9 Mon Sep 17 00:00:00 2001 From: Chris Swithinbank Date: Tue, 19 Nov 2024 12:44:08 +0100 Subject: [PATCH 2/4] Add changeset --- .changeset/thick-cooks-call.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/thick-cooks-call.md diff --git a/.changeset/thick-cooks-call.md b/.changeset/thick-cooks-call.md new file mode 100644 index 00000000000..6b3e49cadc0 --- /dev/null +++ b/.changeset/thick-cooks-call.md @@ -0,0 +1,5 @@ +--- +'@astrojs/starlight': patch +--- + +Fixes an edge case to correctly avoid a trailing slash when navigating from a root locale homepage to another language via Starlight’s language switcher when `trailingSlash: 'never'` is set From 6e1dee4dfceda11e08712eb99b158c0c46fbce45 Mon Sep 17 00:00:00 2001 From: Chris Swithinbank Date: Tue, 19 Nov 2024 12:53:23 +0100 Subject: [PATCH 3/4] Update more tests I missed --- .../__tests__/basics/localizedUrl.test.ts | 8 ++++---- .../localizedUrl.test.ts | 8 ++++---- .../__tests__/i18n/localizedUrl.test.ts | 16 ++++++++-------- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/starlight/__tests__/basics/localizedUrl.test.ts b/packages/starlight/__tests__/basics/localizedUrl.test.ts index f65529cbfa5..480699cde73 100644 --- a/packages/starlight/__tests__/basics/localizedUrl.test.ts +++ b/packages/starlight/__tests__/basics/localizedUrl.test.ts @@ -4,23 +4,23 @@ import { localizedUrl } from '../../utils/localizedUrl'; describe('with `build.output: "directory"`', () => { test('it has no effect in a monolingual project', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, undefined).href).toBe(url.href); + expect(localizedUrl(url, undefined, 'ignore').href).toBe(url.href); }); test('has no effect on index route in a monolingual project', () => { const url = new URL('https://example.com/'); - expect(localizedUrl(url, undefined).href).toBe(url.href); + expect(localizedUrl(url, undefined, 'ignore').href).toBe(url.href); }); }); describe('with `build.output: "file"`', () => { test('it has no effect in a monolingual project', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, undefined).href).toBe(url.href); + expect(localizedUrl(url, undefined, 'ignore').href).toBe(url.href); }); test('has no effect on index route in a monolingual project', () => { const url = new URL('https://example.com/index.html'); - expect(localizedUrl(url, undefined).href).toBe(url.href); + expect(localizedUrl(url, undefined, 'ignore').href).toBe(url.href); }); }); diff --git a/packages/starlight/__tests__/i18n-non-root-single-locale/localizedUrl.test.ts b/packages/starlight/__tests__/i18n-non-root-single-locale/localizedUrl.test.ts index 49ef69aa7f3..3ebf6de570f 100644 --- a/packages/starlight/__tests__/i18n-non-root-single-locale/localizedUrl.test.ts +++ b/packages/starlight/__tests__/i18n-non-root-single-locale/localizedUrl.test.ts @@ -4,23 +4,23 @@ import { localizedUrl } from '../../utils/localizedUrl'; describe('with `build.output: "directory"`', () => { test('it has no effect in a monolingual project with a non-root single locale', () => { const url = new URL('https://example.com/fr/guide/'); - expect(localizedUrl(url, 'fr').href).toBe(url.href); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe(url.href); }); test('has no effect on index route in a monolingual project with a non-root single locale', () => { const url = new URL('https://example.com/fr/'); - expect(localizedUrl(url, 'fr').href).toBe(url.href); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe(url.href); }); }); describe('with `build.output: "file"`', () => { test('it has no effect in a monolingual project with a non-root single locale', () => { const url = new URL('https://example.com/fr/guide.html'); - expect(localizedUrl(url, 'fr').href).toBe(url.href); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe(url.href); }); test('has no effect on index route in a monolingual project with a non-root single locale', () => { const url = new URL('https://example.com/fr.html'); - expect(localizedUrl(url, 'fr').href).toBe(url.href); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe(url.href); }); }); diff --git a/packages/starlight/__tests__/i18n/localizedUrl.test.ts b/packages/starlight/__tests__/i18n/localizedUrl.test.ts index fb4ec2d7c19..b138af3752b 100644 --- a/packages/starlight/__tests__/i18n/localizedUrl.test.ts +++ b/packages/starlight/__tests__/i18n/localizedUrl.test.ts @@ -4,43 +4,43 @@ import { localizedUrl } from '../../utils/localizedUrl'; describe('with `build.output: "directory"`', () => { test('it has no effect if locale matches', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it has no effect if locale matches for index', () => { const url = new URL('https://example.com/en/'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it changes locale to requested locale', () => { const url = new URL('https://example.com/en/guide/'); - expect(localizedUrl(url, 'fr').href).toBe('https://example.com/fr/guide/'); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe('https://example.com/fr/guide/'); }); test('it changes locale to requested locale for index', () => { const url = new URL('https://example.com/en/'); - expect(localizedUrl(url, 'fr').href).toBe('https://example.com/fr/'); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe('https://example.com/fr/'); }); }); describe('with `build.output: "file"`', () => { test('it has no effect if locale matches', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it has no effect if locale matches for index', () => { const url = new URL('https://example.com/en.html'); - expect(localizedUrl(url, 'en').href).toBe(url.href); + expect(localizedUrl(url, 'en', 'ignore').href).toBe(url.href); }); test('it changes locale to requested locale', () => { const url = new URL('https://example.com/en/guide.html'); - expect(localizedUrl(url, 'fr').href).toBe('https://example.com/fr/guide.html'); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe('https://example.com/fr/guide.html'); }); test('it changes locale to requested locale for index', () => { const url = new URL('https://example.com/en.html'); - expect(localizedUrl(url, 'fr').href).toBe('https://example.com/fr.html'); + expect(localizedUrl(url, 'fr', 'ignore').href).toBe('https://example.com/fr.html'); }); }); From e8b0e2a84e835155c48a9c60418f87971078a711 Mon Sep 17 00:00:00 2001 From: Chris Swithinbank Date: Tue, 19 Nov 2024 18:17:44 +0100 Subject: [PATCH 4/4] Re-use existing `stripTrailingSlash()` utility --- packages/starlight/utils/localizedUrl.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/starlight/utils/localizedUrl.ts b/packages/starlight/utils/localizedUrl.ts index 49449cf769f..5b9debee66b 100644 --- a/packages/starlight/utils/localizedUrl.ts +++ b/packages/starlight/utils/localizedUrl.ts @@ -46,8 +46,6 @@ export function localizedUrl( } // Restore base if (hasBase) url.pathname = base + url.pathname; - if (trailingSlash === 'never' && url.pathname.at(-1) === '/') { - url.pathname = url.pathname.slice(0, -1); - } + if (trailingSlash === 'never') url.pathname = stripTrailingSlash(url.pathname); return url; }