From 19eb6fe00430e8f88675aff2a19ddcc0a8cf512b Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 13 Nov 2023 14:21:04 -0500 Subject: [PATCH 1/3] feat(i18n): only pages --- packages/astro/src/core/app/index.ts | 6 ++- packages/astro/src/core/build/generate.ts | 3 +- packages/astro/src/i18n/middleware.ts | 26 ++++++++- .../src/vite-plugin-astro-server/route.ts | 6 ++- .../src/pages/api/test.json.js | 7 +++ packages/astro/test/i18-routing.test.js | 53 +++++++++++++++++++ 6 files changed, 94 insertions(+), 7 deletions(-) create mode 100644 packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 2d5cd16ed6bb..bade522d7342 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -169,8 +169,10 @@ export class App { let i18nMiddleware = createI18nMiddleware( this.#manifest.i18n, this.#manifest.base, - this.#manifest.trailingSlash - ); + this.#manifest.routes, + this.#manifest.trailingSlash + + ); if (i18nMiddleware) { if (mod.onRequest) { this.#pipeline.setMiddlewareFunction( diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 6d8f51df2acd..31a863c3d97e 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -279,7 +279,8 @@ async function generatePage( const i18nMiddleware = createI18nMiddleware( pipeline.getManifest().i18n, pipeline.getManifest().base, - pipeline.getManifest().trailingSlash + pipeline.getManifest().trailingSlash, + pipeline.getManifest().routes ); if (config.experimental.i18n && i18nMiddleware) { if (onRequest) { diff --git a/packages/astro/src/i18n/middleware.ts b/packages/astro/src/i18n/middleware.ts index 7d94d79679ea..d7556ccebecd 100644 --- a/packages/astro/src/i18n/middleware.ts +++ b/packages/astro/src/i18n/middleware.ts @@ -1,5 +1,6 @@ import { appendForwardSlash, joinPaths } from '@astrojs/internal-helpers/path'; import type { MiddlewareEndpointHandler, SSRManifest } from '../@types/astro.js'; +import type { RouteInfo } from '../core/app/types.js'; // Checks if the pathname doesn't have any locale, exception for the defaultLocale, which is ignored on purpose function checkIsLocaleFree(pathname: string, locales: string[]): boolean { @@ -15,19 +16,29 @@ function checkIsLocaleFree(pathname: string, locales: string[]): boolean { export function createI18nMiddleware( i18n: SSRManifest['i18n'], base: SSRManifest['base'], - trailingSlash: SSRManifest['trailingSlash'] + trailingSlash: SSRManifest['trailingSlash'], + + routes: SSRManifest['routes'] ): MiddlewareEndpointHandler | undefined { if (!i18n) { return undefined; } + const pageRoutes = routes.filter((route) => { + return route.routeData.type === 'page'; + }); + return async (context, next) => { if (!i18n) { return await next(); } - const { locales, defaultLocale, fallback } = i18n; const url = context.url; + if (!isPathnameARoutePage(pageRoutes, url.pathname)) { + return await next(); + } + + const { locales, defaultLocale, fallback } = i18n; const response = await next(); @@ -83,3 +94,14 @@ export function createI18nMiddleware( return response; }; } + +/** + * Check whether the pathname of the current request belongs to a route of type page. + * @param pageRoutes + * @param pathname + */ +function isPathnameARoutePage(pageRoutes: RouteInfo[], pathname: string) { + return pageRoutes.some((route) => { + return route.routeData.route === pathname; + }); +} diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 580ceb0b6468..eccbe40a4ef7 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -280,8 +280,10 @@ export async function handleRoute({ const i18Middleware = createI18nMiddleware( config.experimental.i18n, config.base, - config.trailingSlash - ); + config.trailingSlash, + manifest.routes + + ); if (i18Middleware) { if (onRequest) { diff --git a/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js new file mode 100644 index 000000000000..76f3cd4465de --- /dev/null +++ b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js @@ -0,0 +1,7 @@ +export const GET = () => { + return new Response(JSON.stringify({ lorem: 'ipsum' }), { + headers: { + 'content-type': 'application/json', + }, + }); +}; diff --git a/packages/astro/test/i18-routing.test.js b/packages/astro/test/i18-routing.test.js index d9a96ef6eb9f..cac6dc3e211d 100644 --- a/packages/astro/test/i18-routing.test.js +++ b/packages/astro/test/i18-routing.test.js @@ -992,3 +992,56 @@ describe('[SSR] i18n routing', () => { }); }); }); + +describe('i18n routing does not break assets and endpoints', () => { + describe('assets', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/core-image-base/', + experimental: { + i18n: { + defaultLocale: 'en', + locales: ['en', 'es'], + }, + }, + base: '/blog', + }); + await fixture.build(); + }); + + it('should render the image', async () => { + const html = await fixture.readFile('/index.html'); + const $ = cheerio.load(html); + const src = $('#local img').attr('src'); + expect(src.length).to.be.greaterThan(0); + expect(src.startsWith('/blog')).to.be.true; + }); + }); + + describe('endpoint', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + + let app; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/i18n-routing-prefix-always/', + output: 'server', + adapter: testAdapter(), + }); + await fixture.build(); + app = await fixture.loadTestAdapterApp(); + }); + + it('should return the expected data', async () => { + let request = new Request('http://example.com/new-site/api/test'); + let response = await app.render(request); + expect(response.status).to.equal(200); + expect(await response.text()).includes('lorem'); + }); + }); +}); From 069415d43cc790f0baf6b629535bb2c28a6b32bd Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 13 Nov 2023 15:04:40 -0500 Subject: [PATCH 2/3] feat(i18n): apply specific routing logic only to pages --- .changeset/empty-turtles-wave.md | 5 +++ packages/astro/src/core/app/index.ts | 7 ++-- packages/astro/src/core/build/generate.ts | 2 +- packages/astro/src/i18n/middleware.ts | 42 +++++++++++++------ .../src/vite-plugin-astro-server/route.ts | 5 +-- .../src/pages/{api => }/test.json.js | 0 packages/astro/test/i18-routing.test.js | 2 +- 7 files changed, 42 insertions(+), 21 deletions(-) create mode 100644 .changeset/empty-turtles-wave.md rename packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/{api => }/test.json.js (100%) diff --git a/.changeset/empty-turtles-wave.md b/.changeset/empty-turtles-wave.md new file mode 100644 index 000000000000..3605ca3d645d --- /dev/null +++ b/.changeset/empty-turtles-wave.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +The `routingStrategy` `prefix-always` should not force its logic to endpoints. This fixes some regression with `astro:assets` and `@astrojs/rss`. diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index bade522d7342..7fbe7fa4ce40 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -169,10 +169,9 @@ export class App { let i18nMiddleware = createI18nMiddleware( this.#manifest.i18n, this.#manifest.base, - this.#manifest.routes, - this.#manifest.trailingSlash - - ); + this.#manifest.trailingSlash, + this.#manifest.routes.map((route) => route.routeData) + ); if (i18nMiddleware) { if (mod.onRequest) { this.#pipeline.setMiddlewareFunction( diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 31a863c3d97e..961d78ed4865 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -280,7 +280,7 @@ async function generatePage( pipeline.getManifest().i18n, pipeline.getManifest().base, pipeline.getManifest().trailingSlash, - pipeline.getManifest().routes + pipeline.getManifest().routes.map((route) => route.routeData) ); if (config.experimental.i18n && i18nMiddleware) { if (onRequest) { diff --git a/packages/astro/src/i18n/middleware.ts b/packages/astro/src/i18n/middleware.ts index d7556ccebecd..017f062ba040 100644 --- a/packages/astro/src/i18n/middleware.ts +++ b/packages/astro/src/i18n/middleware.ts @@ -1,5 +1,5 @@ import { appendForwardSlash, joinPaths } from '@astrojs/internal-helpers/path'; -import type { MiddlewareEndpointHandler, SSRManifest } from '../@types/astro.js'; +import type { MiddlewareEndpointHandler, RouteData, SSRManifest } from '../@types/astro.js'; import type { RouteInfo } from '../core/app/types.js'; // Checks if the pathname doesn't have any locale, exception for the defaultLocale, which is ignored on purpose @@ -17,15 +17,15 @@ export function createI18nMiddleware( i18n: SSRManifest['i18n'], base: SSRManifest['base'], trailingSlash: SSRManifest['trailingSlash'], - - routes: SSRManifest['routes'] + routes: RouteData[] ): MiddlewareEndpointHandler | undefined { if (!i18n) { return undefined; } - const pageRoutes = routes.filter((route) => { - return route.routeData.type === 'page'; + // we get all the routes that aren't pages + const nonPagesRoutes = routes.filter((route) => { + return route.type !== 'page'; }); return async (context, next) => { @@ -34,8 +34,12 @@ export function createI18nMiddleware( } const url = context.url; - if (!isPathnameARoutePage(pageRoutes, url.pathname)) { - return await next(); + // We get the pathname + // Internally, Astro removes the `base` from the manifest data of the routes. + // We have to make sure that we remove it from the pathname of the request + let astroPathname = url.pathname; + if (astroPathname.startsWith(base) && base !== '/') { + astroPathname = astroPathname.slice(base.length); } const { locales, defaultLocale, fallback } = i18n; @@ -54,6 +58,11 @@ export function createI18nMiddleware( headers: response.headers, }); } else if (i18n.routingStrategy === 'prefix-always') { + // We want to do this check only here, because `prefix-other-locales` assumes that non localized folder are valid + if (shouldSkipRoute(astroPathname, nonPagesRoutes, locales)) { + return await next(); + } + if (url.pathname === base + '/' || url.pathname === base) { if (trailingSlash === 'always') { return context.redirect(`${appendForwardSlash(joinPaths(base, i18n.defaultLocale))}`); @@ -96,12 +105,21 @@ export function createI18nMiddleware( } /** - * Check whether the pathname of the current request belongs to a route of type page. - * @param pageRoutes - * @param pathname + * Checks whether a route should be skipped from the middleware logic. A route should be not be skipped when: + * - it's the home + * - it contains any locale + * - the pathname belongs to a route that is not a page */ -function isPathnameARoutePage(pageRoutes: RouteInfo[], pathname: string) { +function shouldSkipRoute(pathname: string, pageRoutes: RouteData[], locales: string[]) { + if (!pathname.length || pathname === '/') { + return false; + } + + if (locales.some((locale) => pathname.includes(`/${locale}`))) { + return false; + } + return pageRoutes.some((route) => { - return route.routeData.route === pathname; + return !route.pattern.test(pathname); }); } diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index eccbe40a4ef7..616015563272 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -281,9 +281,8 @@ export async function handleRoute({ config.experimental.i18n, config.base, config.trailingSlash, - manifest.routes - - ); + manifestData.routes + ); if (i18Middleware) { if (onRequest) { diff --git a/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/test.json.js similarity index 100% rename from packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/api/test.json.js rename to packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/test.json.js diff --git a/packages/astro/test/i18-routing.test.js b/packages/astro/test/i18-routing.test.js index cac6dc3e211d..a7e8b318d371 100644 --- a/packages/astro/test/i18-routing.test.js +++ b/packages/astro/test/i18-routing.test.js @@ -1038,7 +1038,7 @@ describe('i18n routing does not break assets and endpoints', () => { }); it('should return the expected data', async () => { - let request = new Request('http://example.com/new-site/api/test'); + let request = new Request('http://example.com/new-site/test.json'); let response = await app.render(request); expect(response.status).to.equal(200); expect(await response.text()).includes('lorem'); From 50835807544ead7240a0a80508dca5b56b2b34c4 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Wed, 15 Nov 2023 11:31:51 -0500 Subject: [PATCH 3/3] Use pipeline hooks to bind data to the request --- packages/astro/src/core/app/index.ts | 6 +- packages/astro/src/core/build/generate.ts | 6 +- packages/astro/src/core/pipeline.ts | 20 +++++++ packages/astro/src/i18n/middleware.ts | 55 ++++++------------- .../src/vite-plugin-astro-server/route.ts | 6 +- 5 files changed, 47 insertions(+), 46 deletions(-) diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 7fbe7fa4ce40..ec799011907b 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -6,7 +6,7 @@ import type { SSRElement, SSRManifest, } from '../../@types/astro.js'; -import { createI18nMiddleware } from '../../i18n/middleware.js'; +import { createI18nMiddleware, i18nPipelineHook } from '../../i18n/middleware.js'; import type { SinglePageBuiltModule } from '../build/types.js'; import { getSetCookiesFromResponse } from '../cookies/index.js'; import { consoleLogDestination } from '../logger/console.js'; @@ -169,8 +169,7 @@ export class App { let i18nMiddleware = createI18nMiddleware( this.#manifest.i18n, this.#manifest.base, - this.#manifest.trailingSlash, - this.#manifest.routes.map((route) => route.routeData) + this.#manifest.trailingSlash ); if (i18nMiddleware) { if (mod.onRequest) { @@ -180,6 +179,7 @@ export class App { } else { this.#pipeline.setMiddlewareFunction(i18nMiddleware); } + this.#pipeline.onBeforeRenderRoute(i18nPipelineHook); } else { if (mod.onRequest) { this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler); diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 961d78ed4865..02837cf69cc8 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -30,7 +30,7 @@ import { removeLeadingForwardSlash, removeTrailingForwardSlash, } from '../../core/path.js'; -import { createI18nMiddleware } from '../../i18n/middleware.js'; +import { createI18nMiddleware, i18nPipelineHook } from '../../i18n/middleware.js'; import { runHookBuildGenerated } from '../../integrations/index.js'; import { getOutputDirectory, isServerLikeOutput } from '../../prerender/utils.js'; import { PAGE_SCRIPT_ID } from '../../vite-plugin-scripts/index.js'; @@ -279,8 +279,7 @@ async function generatePage( const i18nMiddleware = createI18nMiddleware( pipeline.getManifest().i18n, pipeline.getManifest().base, - pipeline.getManifest().trailingSlash, - pipeline.getManifest().routes.map((route) => route.routeData) + pipeline.getManifest().trailingSlash ); if (config.experimental.i18n && i18nMiddleware) { if (onRequest) { @@ -290,6 +289,7 @@ async function generatePage( } else { pipeline.setMiddlewareFunction(i18nMiddleware); } + pipeline.onBeforeRenderRoute(i18nPipelineHook); } else if (onRequest) { pipeline.setMiddlewareFunction(onRequest as MiddlewareEndpointHandler); } diff --git a/packages/astro/src/core/pipeline.ts b/packages/astro/src/core/pipeline.ts index 3d33146c6fa9..bd203b437415 100644 --- a/packages/astro/src/core/pipeline.ts +++ b/packages/astro/src/core/pipeline.ts @@ -15,6 +15,12 @@ type EndpointResultHandler = ( result: Response ) => Promise | Response; +type PipelineHooks = { + before: PipelineHookFunction[]; +}; + +export type PipelineHookFunction = (ctx: RenderContext, mod: ComponentInstance | undefined) => void; + /** * This is the basic class of a pipeline. * @@ -23,6 +29,9 @@ type EndpointResultHandler = ( export class Pipeline { env: Environment; #onRequest?: MiddlewareEndpointHandler; + #hooks: PipelineHooks = { + before: [], + }; /** * The handler accepts the *original* `Request` and result returned by the endpoint. * It must return a `Response`. @@ -75,6 +84,9 @@ export class Pipeline { renderContext: RenderContext, componentInstance: ComponentInstance | undefined ): Promise { + for (const hook of this.#hooks.before) { + hook(renderContext, componentInstance); + } const result = await this.#tryRenderRoute( renderContext, this.env, @@ -158,4 +170,12 @@ export class Pipeline { throw new Error(`Couldn't find route of type [${renderContext.route.type}]`); } } + + /** + * Store a function that will be called before starting the rendering phase. + * @param fn + */ + onBeforeRenderRoute(fn: PipelineHookFunction) { + this.#hooks.before.push(fn); + } } diff --git a/packages/astro/src/i18n/middleware.ts b/packages/astro/src/i18n/middleware.ts index 017f062ba040..ff467ecb3131 100644 --- a/packages/astro/src/i18n/middleware.ts +++ b/packages/astro/src/i18n/middleware.ts @@ -1,6 +1,9 @@ import { appendForwardSlash, joinPaths } from '@astrojs/internal-helpers/path'; import type { MiddlewareEndpointHandler, RouteData, SSRManifest } from '../@types/astro.js'; import type { RouteInfo } from '../core/app/types.js'; +import type { PipelineHookFunction } from '../core/pipeline.js'; + +const routeDataSymbol = Symbol.for('astro.routeData'); // Checks if the pathname doesn't have any locale, exception for the defaultLocale, which is ignored on purpose function checkIsLocaleFree(pathname: string, locales: string[]): boolean { @@ -16,34 +19,30 @@ function checkIsLocaleFree(pathname: string, locales: string[]): boolean { export function createI18nMiddleware( i18n: SSRManifest['i18n'], base: SSRManifest['base'], - trailingSlash: SSRManifest['trailingSlash'], - routes: RouteData[] + trailingSlash: SSRManifest['trailingSlash'] ): MiddlewareEndpointHandler | undefined { if (!i18n) { return undefined; } - // we get all the routes that aren't pages - const nonPagesRoutes = routes.filter((route) => { - return route.type !== 'page'; - }); - return async (context, next) => { if (!i18n) { return await next(); } - const url = context.url; - // We get the pathname - // Internally, Astro removes the `base` from the manifest data of the routes. - // We have to make sure that we remove it from the pathname of the request - let astroPathname = url.pathname; - if (astroPathname.startsWith(base) && base !== '/') { - astroPathname = astroPathname.slice(base.length); + const routeData = Reflect.get(context.request, routeDataSymbol); + if (routeData) { + // If the route we're processing is not a page, then we ignore it + if ( + (routeData as RouteData).type !== 'page' && + (routeData as RouteData).type !== 'fallback' + ) { + return await next(); + } } + const url = context.url; const { locales, defaultLocale, fallback } = i18n; - const response = await next(); if (response instanceof Response) { @@ -58,11 +57,6 @@ export function createI18nMiddleware( headers: response.headers, }); } else if (i18n.routingStrategy === 'prefix-always') { - // We want to do this check only here, because `prefix-other-locales` assumes that non localized folder are valid - if (shouldSkipRoute(astroPathname, nonPagesRoutes, locales)) { - return await next(); - } - if (url.pathname === base + '/' || url.pathname === base) { if (trailingSlash === 'always') { return context.redirect(`${appendForwardSlash(joinPaths(base, i18n.defaultLocale))}`); @@ -105,21 +99,8 @@ export function createI18nMiddleware( } /** - * Checks whether a route should be skipped from the middleware logic. A route should be not be skipped when: - * - it's the home - * - it contains any locale - * - the pathname belongs to a route that is not a page + * This pipeline hook attaches a `RouteData` object to the `Request` */ -function shouldSkipRoute(pathname: string, pageRoutes: RouteData[], locales: string[]) { - if (!pathname.length || pathname === '/') { - return false; - } - - if (locales.some((locale) => pathname.includes(`/${locale}`))) { - return false; - } - - return pageRoutes.some((route) => { - return !route.pattern.test(pathname); - }); -} +export const i18nPipelineHook: PipelineHookFunction = (ctx) => { + Reflect.set(ctx.request, routeDataSymbol, ctx.route); +}; diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 616015563272..7468f881907a 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -20,7 +20,7 @@ import { import { createRequest } from '../core/request.js'; import { matchAllRoutes } from '../core/routing/index.js'; import { isPage, resolveIdToUrl } from '../core/util.js'; -import { createI18nMiddleware } from '../i18n/middleware.js'; +import { createI18nMiddleware, i18nPipelineHook } from '../i18n/middleware.js'; import { getSortedPreloadedMatches } from '../prerender/routing.js'; import { isServerLikeOutput } from '../prerender/utils.js'; import { PAGE_SCRIPT_ID } from '../vite-plugin-scripts/index.js'; @@ -280,8 +280,7 @@ export async function handleRoute({ const i18Middleware = createI18nMiddleware( config.experimental.i18n, config.base, - config.trailingSlash, - manifestData.routes + config.trailingSlash ); if (i18Middleware) { @@ -290,6 +289,7 @@ export async function handleRoute({ } else { pipeline.setMiddlewareFunction(i18Middleware); } + pipeline.onBeforeRenderRoute(i18nPipelineHook); } else if (onRequest) { pipeline.setMiddlewareFunction(onRequest); }