withastro / roadmap

Ideas, suggestions, and formal RFC proposals for the Astro project.
320 stars 30 forks source link

Rewriting #901

Closed ematipico closed 4 months ago

ematipico commented 7 months ago

Summary

Links

matthewp commented 7 months ago

We should figure out what happens if there are cycles; for example if /foo reroutes to /bar which reroutes back to /foo on an endless loop. Should we do anything to try and prevent that?

ematipico commented 7 months ago

We should figure out what happens if there are cycles; for example if /foo reroutes to /bar which reroutes back to /foo on an endless loop. Should we do anything to try and prevent that?

In my early prototype, I was able to detect loops. I added a paragraph for that here c8f69ac (#901)

matthewp commented 7 months ago

Let's say you had a middleware stack like this:

i18n -> custom -> auth -> page

Given that reroute() returns a response, I think this means that if you call reroute in the custom middleware function, then auth will not be called. Is that right? Or it the function aware of where the current point is in the middleware chain and continues to call the proceeding middleware functions?

ematipico commented 7 months ago

Let's say you had a middleware stack like this:

i18n -> custom -> auth -> page

Given that reroute() returns a response, I think this means that if you call reroute in the custom middleware function, then auth will not be called. Is that right? Or it the function aware of where the current point is in the middleware chain and continues to call the proceeding middleware functions?

That's right, I updated the proposal to better explain the API and how next("/new") vs ctx.reroute("/new") differ

ematipico commented 6 months ago

The commit f9ec599 (#901) changes "reroute" to "rewrite". We assessed that the majority of frameworks and services out there use the term "rewrite".

vchirikov commented 5 months ago

Hi, I want to discuss about the limitations of the current implementation:

  1. I think it will be useful to give an opportunity to middleware to change not only URL, but the properties of context. e.g. locale or locals.

  2. It will be useful to rewrite to /404 / /500, e.g. I want to use a custom i18n middleware which extract the url part from pathname or use the default and rewrite (reroute) to the same page (no additional files for /en, /de, etc). For example /de/about -> about.astro, /en/about -> about.astro, /about -> about.astro. To do this I write a middleware to provide a locale, because I can't change Astro.currentLocale (point 1) I provide a locale via Astro.locals something like this:

import type { MiddlewareHandler } from 'astro';
import type { AstroError } from 'astro/errors';

export function isAstroError(value: unknown): value is AstroError {
  return !!value && typeof value === 'object' && 'type' in value && value.type === 'AstroError';
}

import { defaultLocale, extractLocale } from '@/lib/i18n';
import { removeTrailingSlash } from '@/lib/utils/url';

const frontendUrl = removeTrailingSlash(process.env.FRONTEND_URL);
const frontendUrlLen = frontendUrl.length;

export const onRequest: MiddlewareHandler = async (ctx, next) => {
  // can't set ctx.currentLocale, and can't rewrite with new currentLocale
  if (ctx.request.url.startsWith(frontendUrl)) {
    const urlWithoutBase = ctx.request.url.slice(frontendUrlLen);
    const { locale, url } = extractLocale(urlWithoutBase);
    ctx.locals.locale = locale ?? defaultLocale;
    try {
      await next(url);
      return;
    }
    catch (error: unknown) {
      // now in case of 404 or rendering error astro will throw RewriteEncounteredAnError
      if (isAstroError(error) && error.name === 'RewriteEncounteredAnError') {
        next('/404'); // can't do this, so I can't localize 404.astro properly
        // or next('/500'); in case or rendering error
      }
    }
  }
  await next();
};

But I can't rewrite to /404 for now.

ematipico commented 5 months ago

Hi, I want to discuss about the limitations of the current implementation:

  1. I think it will be useful to give an opportunity to middleware to change not only URL, but the properties of context. e.g. locale or locals.

Can you provide more specifics here? As long your code runs in the middleware, you can always change locals, and I fail to see the connection with the rewriting, that's why I ask more specifics.

  1. It will be useful to rewrite to /404 / /500, e.g. I want to use a custom i18n middleware which extract the url part from pathname or use the default and rewrite (reroute) to the same page (no additional files for /en, /de, etc). For example /de/about -> about.astro, /en/about -> about.astro, /about -> about.astro. To do this I write a middleware to provide a locale, because I can't change Astro.currentLocale (point 1) I provide a locale via Astro.locals something like this:
import type { MiddlewareHandler } from 'astro';
import type { AstroError } from 'astro/errors';

export function isAstroError(value: unknown): value is AstroError {
  return !!value && typeof value === 'object' && 'type' in value && value.type === 'AstroError';
}

import { defaultLocale, extractLocale } from '@/lib/i18n';
import { removeTrailingSlash } from '@/lib/utils/url';

const frontendUrl = removeTrailingSlash(process.env.FRONTEND_URL);
const frontendUrlLen = frontendUrl.length;

export const onRequest: MiddlewareHandler = async (ctx, next) => {
  // can't set ctx.currentLocale, and can't rewrite with new currentLocale
  if (ctx.request.url.startsWith(frontendUrl)) {
    const urlWithoutBase = ctx.request.url.slice(frontendUrlLen);
    const { locale, url } = extractLocale(urlWithoutBase);
    ctx.locals.locale = locale ?? defaultLocale;
    try {
      await next(url);
      return;
    }
    catch (error: unknown) {
      // now in case of 404 or rendering error astro will throw RewriteEncounteredAnError
      if (isAstroError(error) && error.name === 'RewriteEncounteredAnError') {
        next('/404'); // can't do this, so I can't localize 404.astro properly
        // or next('/500'); in case or rendering error
      }
    }
  }
  await next();
};

But I can't rewrite to /404 for now.

404 and 500 would require some attention. Instead of talking code, can you provide expectations base on use cases?

vchirikov commented 5 months ago
  1. about changing context:

I want to use same .astro files to show different localized pages. Translation of pages should be done with i18next (for example). The desired project structure and urls should be:

├───pages
│       404.astro        // same page should be usable with different locales (e.g. en,de,ru)
│       about.astro      // urls: /about, /en/about, /de/about, /ru/about
│       index.astro      // urls: /, /en, /de, /ru

To do this I can implement custom i18n middleware which allow to re-use same .astro pages with different locale (via rewriting from /en/about to /about), but I can't set the ctx.currentLocale (it has no setter) with current implementation of rewriting. The proof:

image

So instead of Astro-integrated i18n features I have to use Astro.locals to provide locale to the pages (because I can't change the context's Astro.currentLocale via rewriting)

My point is it seems useful to give a user an extension point to define/edit readonly context fields from the middleware. Especially redefine Astro.currentLocale.

p.s. thanks a lot for the rewriting feature :) p.p.s I tried to be as clear as possible, but if something else remains unclear - feel free to ask :)

ematipico commented 4 months ago

Thank you, @vchirikov; I will take your feedback into account. However, this is something unrelated to rewriting. These are features related to i18n.

ematipico commented 4 months ago

This is a call for consensus, and we aim to close the RFC in the next three days. Let us know if you have any important concerns.