Skip to content
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.

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

feat(i18n): expand fallback system #11677

Merged
merged 14 commits into from
Aug 28, 2024
22 changes: 22 additions & 0 deletions .changeset/blue-pens-divide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
---
'astro': patch
---

Fixes a bug in the logic of `Astro.rewrite()` which led to the value for `base`, if configured, being automatically prepended to the rewrite URL passed. This was unintended behavior and has been corrected, and Astro now processes the URLs exactly as passed.

If you use the `rewrite()` function on a project that has `base` configured, you must now prepend the base to your existing rewrite URL:

```js
// astro.config.mjs
export default defineConfig({
base: '/blog'
})
```

```diff
// src/middleware.js
export function onRequest(ctx, next) {
- return ctx.rewrite("/about")
+ return ctx.rewrite("/blog/about")
}
```
32 changes: 32 additions & 0 deletions .changeset/lazy-feet-join.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
'astro': minor
---

Adds a new option `fallbackType` to `i18n.routing` configuration that allows you to control how fallback pages are handled.

When `i18n.fallback` is configured, this new routing option controls whether to [redirect](https://docs.astro.build/en/guides/routing/#redirects) to the fallback page, or to [rewrite](https://docs.astro.build/en/guides/routing/#rewrites) the fallback page's content in place.

The `"redirect"` option is the default value and matches the current behavior of the existing fallback system.

The option `"rewrite"` uses the new [rewriting system](https://docs.astro.build/en/guides/routing/#rewrites) to create fallback pages that render content on the original, requested URL without a browser refresh.

For example, the following configuration will generate a page `/fr/index.html` that will contain the same HTML rendered by the page `/en/index.html` when `src/pages/fr/index.astro` does not exist.

```js
// astro.config.mjs
export default defineConfig({
i18n: {
locals: ["en", "fr"],
defaultLocale: "en",
routing: {
prefixDefaultLocale: true,
fallbackType: "rewrite"
},
fallback: {
fr: "en"
},
}
})
```


37 changes: 37 additions & 0 deletions packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1668,6 +1668,43 @@ export interface AstroUserConfig {
* */
redirectToDefaultLocale?: boolean;

/**
* @docs
Copy link
Member

Choose a reason for hiding this comment

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

I have some questions first here @ematipico

Is this top-level i18n config? Is this entry in the correct place? Right now, it would be showing up between two i18n.routing.* entries.

Shouldn't it also be near i18n.fallback? And, this doesn't replace it, but is a separate, same-level configuration? (that's up around line 1567).

I think we should also see an example of both this and fallback configured together in one code sample, since I'm assuming they'd need to work together. Without any i18n.fallback configured, you default to showing 404s for pages that don't exist, so you can't have a strategy?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this top-level i18n config? Is this entry in the correct place? Right now, it would be showing up between two i18n.routing.* entries.

Shouldn't it also be near i18n.fallback? And, this doesn't replace it, but is a separate, same-level configuration? (that's up around line 1567).

It should be i18n.routing.fallbackType. I will update the code, because it's incorrect.

I think we should also see an example of both this and fallback configured together in one code sample, since I'm assuming they'd need to work together. Without any i18n.fallback configured, you default to showing 404s for pages that don't exist, so you can't have a strategy?

I updated the docs with an example here 2447fb9 (#11677)

* @name i18n.routing.fallbackType
ematipico marked this conversation as resolved.
Show resolved Hide resolved
* @kind h4
* @type {"redirect" | "rewrite"}
* @default `"redirect"`
* @version 4.15.0
* @description
*
* When [`i18n.fallback`](#i18nfallback) is configured to avoid showing a 404 page for missing page routes, this option controls whether to [redirect](https://docs.astro.build/en/guides/routing/#redirects) to the fallback page, or to [rewrite](https://docs.astro.build/en/guides/routing/#rewrites) the fallback page's content in place.
*
* By default, Astro's i18n routing creates pages that redirect your visitors to a new destination based on your fallback configuration. The browser will refresh and show the destination address in the URL bar.
*
* When `i18n.routing.fallback: "rewrite"` is configured, Astro will create pages that render the contents of the fallback page on the original, requested URL.
*
* With the following configuration, if you have the file `src/pages/en/about.astro` but not `src/pages/fr/about.astro`, the `astro build` command will generate `dist/fr/about.html` with the same content as the `dist/en/index.html` page.
* Your site visitor will see the English version of the page at `https://example.com/fr/about/` and will not be redirected.
*
* ```js
* //astro.config.mjs
* export default defineConfig({
* i18n: {
* defaultLocale: "en",
* locales: ["en", "fr"],
* routing: {
* prefixDefaultLocale: false,
* fallbackType: "rewrite",
* },
* fallback: {
* fr: "en",
* }
* },
* })
* ```
*/
fallbackType: "redirect" | "rewrite"

/**
* @name i18n.routing.strategy
* @type {"pathname"}
Expand Down
10 changes: 5 additions & 5 deletions packages/astro/src/container/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type {
SSRElement,
SSRResult,
} from '../@types/astro.js';
import { type HeadElements, Pipeline } from '../core/base-pipeline.js';
import {type HeadElements, Pipeline, type TryRewriteResult} from '../core/base-pipeline.js';
import type { SinglePageBuiltModule } from '../core/build/types.js';
import {
createModuleScriptElement,
Expand Down Expand Up @@ -71,8 +71,8 @@ export class ContainerPipeline extends Pipeline {
async tryRewrite(
payload: RewritePayload,
request: Request,
): Promise<[RouteData, ComponentInstance, URL]> {
const [foundRoute, finalUrl] = findRouteToRewrite({
): Promise<TryRewriteResult> {
const {newUrl,pathname,routeData} = findRouteToRewrite({
payload,
request,
routes: this.manifest?.routes.map((r) => r.routeData),
Expand All @@ -81,8 +81,8 @@ export class ContainerPipeline extends Pipeline {
base: this.manifest.base,
});

const componentInstance = await this.getComponentByRoute(foundRoute);
return [foundRoute, componentInstance, finalUrl];
const componentInstance = await this.getComponentByRoute(routeData);
return {componentInstance, routeData, newUrl, pathname};
}

insertRoute(route: RouteData, componentInstance: ComponentInstance): void {
Expand Down
10 changes: 5 additions & 5 deletions packages/astro/src/core/app/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
SSRElement,
SSRResult,
} from '../../@types/astro.js';
import { Pipeline } from '../base-pipeline.js';
import {Pipeline, type TryRewriteResult} from '../base-pipeline.js';
import type { SinglePageBuiltModule } from '../build/types.js';
import { RedirectSinglePageBuiltModule } from '../redirects/component.js';
import { createModuleScriptElement, createStylesheetElementSet } from '../render/ssr-element.js';
Expand Down Expand Up @@ -94,8 +94,8 @@ export class AppPipeline extends Pipeline {
payload: RewritePayload,
request: Request,
_sourceRoute: RouteData,
): Promise<[RouteData, ComponentInstance, URL]> {
const [foundRoute, finalUrl] = findRouteToRewrite({
): Promise<TryRewriteResult> {
const { newUrl,pathname,routeData} = findRouteToRewrite({
payload,
request,
routes: this.manifest?.routes.map((r) => r.routeData),
Expand All @@ -104,8 +104,8 @@ export class AppPipeline extends Pipeline {
base: this.manifest.base,
});

const componentInstance = await this.getComponentByRoute(foundRoute);
return [foundRoute, componentInstance, finalUrl];
const componentInstance = await this.getComponentByRoute(routeData);
return {newUrl, pathname, componentInstance, routeData};
}

async getModuleForRoute(route: RouteData): Promise<SinglePageBuiltModule> {
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/app/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ export type SSRManifest = {

export type SSRManifestI18n = {
fallback: Record<string, string> | undefined;
fallbackType: "redirect" | "rewrite";
strategy: RoutingStrategies;
locales: Locales;
defaultLocale: string;
Expand Down
9 changes: 8 additions & 1 deletion packages/astro/src/core/base-pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ export abstract class Pipeline {
rewritePayload: RewritePayload,
request: Request,
sourceRoute: RouteData,
): Promise<[RouteData, ComponentInstance, URL]>;
): Promise<TryRewriteResult>;

/**
* Tells the pipeline how to retrieve a component give a `RouteData`
Expand All @@ -106,3 +106,10 @@ export abstract class Pipeline {

// eslint-disable-next-line @typescript-eslint/no-empty-object-type
export interface HeadElements extends Pick<SSRResult, 'scripts' | 'styles' | 'links'> {}

export interface TryRewriteResult {
routeData: RouteData,
componentInstance: ComponentInstance,
newUrl: URL,
pathname: string
}
3 changes: 2 additions & 1 deletion packages/astro/src/core/build/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
removeLeadingForwardSlash,
removeTrailingForwardSlash,
} from '../../core/path.js';
import { toRoutingStrategy } from '../../i18n/utils.js';
import {toFallbackType, toRoutingStrategy} from '../../i18n/utils.js';
import { runHookBuildGenerated } from '../../integrations/hooks.js';
import { getOutputDirectory } from '../../prerender/utils.js';
import type { SSRManifestI18n } from '../app/types.js';
Expand Down Expand Up @@ -528,6 +528,7 @@ function createBuildManifest(
if (settings.config.i18n) {
i18nManifest = {
fallback: settings.config.i18n.fallback,
fallbackType: toFallbackType(settings.config.i18n.routing),
strategy: toRoutingStrategy(settings.config.i18n.routing, settings.config.i18n.domains),
defaultLocale: settings.config.i18n.defaultLocale,
locales: settings.config.i18n.locales,
Expand Down
9 changes: 5 additions & 4 deletions packages/astro/src/core/build/pipeline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { RESOLVED_SPLIT_MODULE_ID } from './plugins/plugin-ssr.js';
import { getPagesFromVirtualModulePageName, getVirtualModulePageName } from './plugins/util.js';
import type { PageBuildData, SinglePageBuiltModule, StaticBuildOptions } from './types.js';
import { i18nHasFallback } from './util.js';
import type {TryRewriteResult} from "../base-pipeline.js";

/**
* The build pipeline is responsible to gather the files emitted by the SSR build and generate the pages by executing these files.
Expand Down Expand Up @@ -289,8 +290,8 @@ export class BuildPipeline extends Pipeline {
payload: RewritePayload,
request: Request,
_sourceRoute: RouteData,
): Promise<[RouteData, ComponentInstance, URL]> {
const [foundRoute, finalUrl] = findRouteToRewrite({
): Promise<TryRewriteResult> {
const { routeData, pathname, newUrl} = findRouteToRewrite({
payload,
request,
routes: this.options.manifest.routes,
Expand All @@ -299,8 +300,8 @@ export class BuildPipeline extends Pipeline {
base: this.config.base,
});

const componentInstance = await this.getComponentByRoute(foundRoute);
return [foundRoute, componentInstance, finalUrl];
const componentInstance = await this.getComponentByRoute(routeData);
return { routeData, componentInstance, newUrl, pathname };
}

async retrieveSsrEntry(route: RouteData, filePath: string): Promise<SinglePageBuiltModule> {
Expand Down
3 changes: 2 additions & 1 deletion packages/astro/src/core/build/plugins/plugin-manifest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import type { OutputChunk } from 'rollup';
import type { Plugin as VitePlugin } from 'vite';
import { getAssetsPrefix } from '../../../assets/utils/getAssetsPrefix.js';
import { normalizeTheLocale } from '../../../i18n/index.js';
import { toRoutingStrategy } from '../../../i18n/utils.js';
import {toFallbackType, toRoutingStrategy} from '../../../i18n/utils.js';
import { runHookBuildSsr } from '../../../integrations/hooks.js';
import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../../vite-plugin-scripts/index.js';
import type {
Expand Down Expand Up @@ -254,6 +254,7 @@ function buildManifest(
if (settings.config.i18n) {
i18nManifest = {
fallback: settings.config.i18n.fallback,
fallbackType: toFallbackType(settings.config.i18n.routing),
strategy: toRoutingStrategy(settings.config.i18n.routing, settings.config.i18n.domains),
locales: settings.config.i18n.locales,
defaultLocale: settings.config.i18n.defaultLocale,
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,7 @@ export const AstroConfigSchema = z.object({
.object({
prefixDefaultLocale: z.boolean().optional().default(false),
redirectToDefaultLocale: z.boolean().optional().default(true),
fallbackType: z.enum(["redirect", "rewrite"]).optional().default("redirect"),
})
.refine(
({ prefixDefaultLocale, redirectToDefaultLocale }) => {
Expand Down
14 changes: 7 additions & 7 deletions packages/astro/src/core/render-context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,13 @@ export class RenderContext {
if (payload) {
pipeline.logger.debug('router', 'Called rewriting to:', payload);
// we intentionally let the error bubble up
const [routeData, component] = await pipeline.tryRewrite(
const { routeData, componentInstance: newComponent} = await pipeline.tryRewrite(
payload,
this.request,
this.originalRoute,
);
this.routeData = routeData;
componentInstance = component;
componentInstance = newComponent;
this.isRewriting = true;
this.status = 200;
}
Expand Down Expand Up @@ -234,7 +234,7 @@ export class RenderContext {

async #executeRewrite(reroutePayload: RewritePayload) {
this.pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload);
const [routeData, component, newURL] = await this.pipeline.tryRewrite(
const { routeData, componentInstance, newUrl, pathname } = await this.pipeline.tryRewrite(
reroutePayload,
this.request,
this.originalRoute,
Expand All @@ -243,16 +243,16 @@ export class RenderContext {
if (reroutePayload instanceof Request) {
this.request = reroutePayload;
} else {
this.request = this.#copyRequest(newURL, this.request);
this.request = this.#copyRequest(newUrl, this.request);
}
this.url = new URL(this.request.url);
this.cookies = new AstroCookies(this.request);
this.params = getParams(routeData, this.url.pathname);
this.pathname = this.url.pathname;
this.params = getParams(routeData, pathname);
this.pathname = pathname;
this.isRewriting = true;
// we found a route and a component, we can change the status code to 200
this.status = 200;
return await this.render(component);
return await this.render(componentInstance);
}

createActionAPIContext(): ActionAPIContext {
Expand Down
43 changes: 30 additions & 13 deletions packages/astro/src/core/routing/rewrite.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,44 +12,61 @@ export type FindRouteToRewrite = {
base: AstroConfig['base'];
};

export interface FindRouteToRewriteResult {
routeData: RouteData;
newUrl: URL;
pathname: string;
}

/**
* Shared logic to retrieve the rewritten route. It returns a tuple that represents:
* 1. The new `Request` object. It contains `base`
* 2.
*/
export function findRouteToRewrite({
payload,
routes,
request,
trailingSlash,
buildFormat,
base,
}: FindRouteToRewrite): [RouteData, URL] {
let finalUrl: URL | undefined = undefined;
}: FindRouteToRewrite): FindRouteToRewriteResult {
let newUrl: URL | undefined = undefined;
if (payload instanceof URL) {
finalUrl = payload;
newUrl = payload;
} else if (payload instanceof Request) {
finalUrl = new URL(payload.url);
newUrl = new URL(payload.url);
} else {
finalUrl = new URL(payload, new URL(request.url).origin);
newUrl = new URL(payload, new URL(request.url).origin);
}
let pathname = newUrl.pathname;
if (base !== '/' && newUrl.pathname.startsWith(base)) {
pathname = shouldAppendForwardSlash(trailingSlash, buildFormat)
? appendForwardSlash(newUrl.pathname)
: removeTrailingForwardSlash(newUrl.pathname);
pathname = pathname.slice(base.length);
}

let foundRoute;
for (const route of routes) {
const pathname = shouldAppendForwardSlash(trailingSlash, buildFormat)
? appendForwardSlash(finalUrl.pathname)
: base !== '/'
? removeTrailingForwardSlash(finalUrl.pathname)
: finalUrl.pathname;
if (route.pattern.test(decodeURI(pathname))) {
foundRoute = route;
break;
}
}

if (foundRoute) {
return [foundRoute, finalUrl];
return {
routeData: foundRoute,
newUrl,
pathname
};
} else {
const custom404 = routes.find((route) => route.route === '/404');
if (custom404) {
return [custom404, finalUrl];
return { routeData: custom404, newUrl, pathname };
} else {
return [DEFAULT_404_ROUTE, finalUrl];
return { routeData: DEFAULT_404_ROUTE, newUrl, pathname };
}
}
}
Loading
Loading