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

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

Closed
1 task
jlarmstrongiv opened this issue Aug 26, 2023 · 18 comments · Fixed by #10248
Closed
1 task

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

jlarmstrongiv opened this issue Aug 26, 2023 · 18 comments · Fixed by #10248
Assignees
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: integration Related to any renderer integration (scope)

Comments

@jlarmstrongiv
Copy link

jlarmstrongiv commented Aug 26, 2023

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 jlarmstrongiv/astro-i18n-aut#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

jlarmstrongiv/astro-i18n-aut#19

Participation

  • I am willing to submit a pull request for this issue.
@jlarmstrongiv
Copy link
Author

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

@jlarmstrongiv jlarmstrongiv changed the title Recent injectRoute changes broke integration astro-i18n-aut Astro v3: Recent injectRoute changes broke integration astro-i18n-aut Aug 31, 2023
@natemoo-re
Copy link
Member

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.

@natemoo-re natemoo-re added - P4: important Violate documented behavior or significantly impacts performance (priority) pkg: integration Related to any renderer integration (scope) labels Aug 31, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 31, 2023
@jlarmstrongiv
Copy link
Author

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
Copy link

zanhk commented Nov 18, 2023

@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
Copy link
Author

@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
Copy link
Author

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
Copy link
Contributor

castarco commented Dec 19, 2023

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
Copy link
Author

@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
Copy link
Member

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
Copy link
Author

@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
Copy link

rawr51919 commented Feb 17, 2024

I wonder if this is related to zankhq/astro-starter#23 and rawr51919/portfolio-2024#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 ematipico self-assigned this Feb 27, 2024
@ematipico
Copy link
Member

ematipico commented Feb 27, 2024

I'll look into it

@ematipico
Copy link
Member

ematipico commented Feb 27, 2024

@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
Copy link

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

@ematipico
Copy link
Member

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

@rawr51919
Copy link

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

@jlarmstrongiv
Copy link
Author

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

@jlarmstrongiv
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants