Astro v3: Recent `injectRoute` changes broke integration `astro-i18n-aut`

jlarmstrongiv commented 1 year ago

Astro info

Astro version            v2.10.14
Package manager          npm
Platform                 darwin
Architecture             x64
Adapter                  Couldn't determine.
Integrations             @astrojs/mdx, astro-i18n-integration, @astrojs/sitemap

What browser are you using?

Chrome

Describe the Bug

Has the behavior of injectRoute changed substantially in the last month or so? It seems to have broken my translation integration https://github.com/jlarmstrongiv/astro-i18n-aut/issues/19

Recent changes @natemoo-re:

It seems that the astro dev server no longer supports using injectRoute() with the same entryPoint multiple times, specifically when the entryPoint has dynamic paths like [slug].astro.

For example, src/pages/blog/[slug].astro is used as the entryPoint for with the default page, but is also injected with a pattern that has a locale prefix like /es/

It works fine when the entryPoint and pattern doesn’t have any dynamic path portions, like src/pages/about.astro

On the first load, whichever pattern is loaded always works. But, trying to use another pattern that uses the same entryPoint results in an error:

03:44:10 PM [getStaticPaths] A `getStaticPaths()` route pattern was matched, but no matching static path was found for requested path `/blog/using-mdx/`.

Possible dynamic routes being matched: src/pages/blog/[slug].astro.
03:44:10 PM [serve]    404                         /blog/using-mdx/
03:44:12 PM [getStaticPaths] A `getStaticPaths()` route pattern was matched, but no matching static path was found for requested path `/fr/blog/using-mdx/`.

Possible dynamic routes being matched: src/pages/blog/[slug].astro.
03:44:12 PM [serve]    404                      /fr/blog/using-mdx/

Would this be a core astro bug? Are there any alternatives to get the same functionality?

Sounds like a bug to me! A GitHub issue and reproduction would be super helpful.

What's the expected result?

The correct page loads instead of returning 404.

Link to Minimal Reproducible Example

https://github.com/jlarmstrongiv/astro-i18n-aut/issues/19

Participation

[ ] I am willing to submit a pull request for this issue.

jlarmstrongiv commented 1 year ago

Broken in the latest v3 as well:

Astro version            v3.0.4
Package manager          npm
Platform                 darwin
Architecture             x64
Adapter                  Couldn't determine.
Integrations             @astrojs/mdx, astro-i18n-integration, @astrojs/sitemap

natemoo-re commented 1 year ago

Yes the changes from those PRs you mentioned are also in v3.

I haven't had a chance to look at this yet with all the v3 launch stuff coming it, but I hope we can get this fixed soon. Generally the injectRoute behavior honors module resolution much better now, which was from the PRs you linked, but that definitely broke your use case.

Will keep this issue updated when I get a chance to dig into it.

jlarmstrongiv commented 1 year ago

Hiya @natemoo-re,

I’m just wondering if you have any updates for this issue. I was hoping to close it as the i18n routing RFC was released as an experiment, but it doesn’t support the use case that I and others using my i18n package are looking for (1, 2, 3, etc.).

With the proposal, we need to include a page.astro for each page in each locale.

.
└── pages/
    ├── about.astro
    ├── index.astro
    ├── es/
    │   ├── about.astro
    │   └── index.astro
    └── fr/
        ├── about.astro
        └── index.astro

In the astro-i18n-aut integration, a single page.astro handle all locales.

.
└── pages/
    ├── about.astro
    └── index.astro

Optionally, the integration can also manage pages by fetching data from content collections or your API of choice.

.
└── pages/
    ├── blog/
    │   ├── index.astro
    │   └── [id].astro
    └── content/
        └── blog/
            ├── en/
            │   ├── post-1.md
            │   └── post-2.md
            ├── es/
            │   ├── post-1.md
            │   └── post-2.md
            └── fr/
                ├── post-1.md
                └── post-2.md

The astro-i18n-aut integration supports a defaultLocale with no prefixes (/en), without duplicating code in multiple Astro pages.

Since the i18n routing RFC does not support this use case, this issue has become more important, as we need it resolved to support it in the Astro ecosystem. Unfortunately, without this bug fix, it is not possible to support the use case with Astro integrations, middleware, or other existing hooks.

zanhk commented 1 year ago

@jlarmstrongiv I agree with you, the way you handled i18n still seem the best way to do it, when I saw the announcement on the blog page it made me confused, I was expecting a behaviours similar to this integration but from what I could read, it looked like it only introdcued an helper to calculate the url path of the locale where you're at, I didn't dig into it and I'm sure it does more than that (it's also experimental), but from the blog article I didn't saw the benefit of that implementation.

I m thinking this is just a transation point for having a full featured integrated i18n solution but would have be nice if they mentioned the long term route in the article.

My ideal native astro i18n would look like this

Configure locales i18n options from astro.config.mjs

i18n: {
      defaultLocale: "en",
      locales: ["es", "en"]
}

add json (or something similar where I can specify the key/values) with locales on the src/locales folder

// en.json
{
      "hello": "Hello",
      "goodbye": "Goodbye"
}

// es.json
{
      "hello": "Hola",
}

In every page or component import t or getLocaleUrl

---
import { t, getLocaleUrlPrefix } from "astro-i18n-aut";
---

<span>{t.hello}</span>

then astro figure out all by itself giving you full intellisense support for the locales and fallback for the locales that have not been specified

Probably this solution does not accommodate everyone, but would be nice to have something similar as an option, until then the @jlarmstrongiv 's solution looks like the best way to go for me and this bug it's pretty annoying as it make harder to test changes on local environment.

jlarmstrongiv commented 1 year ago

@zanhk and others, if you would like to see Astro support multiple i18n locales in a single Astro file in core, I encourage you to leave constructive comments, suggestions, and 👍’s in the i18n routing RFC—they are currently gathering feedback and it’d be great for you to share yours. Let’s leave this issue specifically for the injectRoute bug.

Still leave 👍’s here for visibility too! Thanks for you consideration and support

jlarmstrongiv commented 1 year ago

One thing I will add is that some adapters, like astro-sst don’t support the astro preview command, making this bug especially painful and testing i18n extremely difficult.

castarco commented 11 months ago

Regarding the idea of choosing the locale dynamically for a single document, I think it makes sense in some cases (applications), but in others it would be a hindrance.

I speak many languages and I've lived in many different countries, and the experience of seeing how the website forces a language while not letting you choose explicitly is f%% horrible.

As a user, for "content" sites, I very much prefer an automatic redirect from / to /en (and then letting me jump to something else (which will be easier to see because the language is already encoded in the URL), like /es, or /ca) than to be in a position where the language has been forced onto me because I live here or there.

jlarmstrongiv commented 11 months ago

@castarco feel free to leave your feedback about your preferred i18n routing experience in the i18n routing RFC

This issue is focused on injectRoute breaking when using the same entryPoint for multiple routes. I created an updated v4 demonstration of the bug in https://github.com/jlarmstrongiv/astro-routing-error

ematipico commented 11 months ago

I wish the reproduction would be minimal, meaning it shouldn't use another library to replicate the issue, because I would like to see the injectRoute API being used, so I could debug it.

I'm willing to help and triage the issue, but I'd need to understand if this is allowed, or if we fixed a bug you were leveraging.

jlarmstrongiv commented 11 months ago

@ematipico please see this minimal reproduction https://github.com/jlarmstrongiv/astro-routing-error please ensure you are on the latest commit for the simplified integration

    {
      name: "astro-routing-bug",
      hooks: {
        "astro:config:setup": async ({ injectRoute }) => {
          injectRoute({
            entrypoint: path.join(process.cwd(), "src/pages/blog/[post].astro"),
            pattern: "/es/blog/[post]",
          });
          injectRoute({
            entrypoint: path.join(process.cwd(), "src/pages/blog/[post].astro"),
            pattern: "/fr/blog/[post]",
          });
        },
      },
    },

The bug is about injectRoute not accepting the same dynamic (e.g. [post].astro) entryPoint file for multiple routes.

It worked great for many versions, but broke unexpectedly in a patch sometime between 2.8.0 and 2.10.14

If you are certain that this is not allowed, please let me know and I will archive astro-i18n-aut. There are no other ways with current Astro apis (even with experimental features) to support i18n with a default locale, without duplicating Astro files for every route. Regardless of whether the solution is in userland or core, it’s important to many users (myself included) to support this use case.

I personally think this should be categorized as a bug, since it broke between Astro minor versions and not allowing the same entryPoint file for multiple routes is an odd limitation that cannot be easily or efficiently worked around. Though, if a new feature is added that makes astro-i18n-aut work or become obsolete, I will still be happy if this issue is closed as wontfix.

rawr51919 commented 9 months ago

I wonder if this is related to https://github.com/zankhq/astro-starter/issues/23 and https://github.com/rawr51919/portfolio-2024/issues/11

There I'm having a very similar issue where if I click on the language I want to display in and then back to the default, it uses the language's prefix instead of none in the URL (in the case of english, /en/ instead of /).

the only fix at the moment is hardcoding every /en/ route as a redirect, wildcard redirects don't work at all

ematipico commented 9 months ago

I'll look into it

ematipico commented 9 months ago

@jlarmstrongiv

Here's some updates:

the issue is caused by our internal caching, once we match a route, we cache its component against the result of getStaticPaths, where they are keyed against the path
it's a complex caching, and it seems that it doesn't allow to have their keyed values to be updated
the second pass matches the same route, but the same component doesn't match the emitted routes, hence the 404

Although, I can see a possible fix on your side while I figure out if there's an efficient caching strategy. pattern can be a regex, so you could create a regex that matches routes that contain multiple locales.

rawr51919 commented 9 months ago

thanks, i wonder if the bugfix also applies to my own astro site

ematipico commented 9 months ago

@rawr51919 I don't know, I fixed the issue based on the minimal repository that was shared here.

rawr51919 commented 9 months ago

might be vercel's end, i'll take a look in a lil

jlarmstrongiv commented 9 months ago

Thank you @ematipico 👍 I will give it a try in the latest release

jlarmstrongiv commented 9 months ago

Just to confirm, this cache fix is applied to both dev and build commands?

withastro / astro