BUG: injectRoute merge routes in production when entrypoint is already used

goulvenclech commented 6 months ago

Hi everyone 👋

Duplicate my message from #3602 , with two minimal reproductions.

I'm currently making an Astro integration where my injectRoutes works perfectly in dev mode, but where all my paths are weirdly merged in production, even with different pattern. This is caused by using the same .astro entrypoint for two or more injectRoutes.

Astro Info

Astro                    v4.5.12
Node                     v20.10.0
System                   macOS (arm64)
Package Manager          pnpm
Output                   static
Adapter                  none
Integrations             astropi

Simple example with static path

Link to Minimal Reproducible Example -> https://stackblitz.com/edit/github-l3a17a?file=astro.config.mjs

    injectRoute({
      pattern: `/blog`,
      entrypoint: './src/entrypoint.astro',
      prerender: true,
    });
    injectRoute({
      pattern: `/another-blog`,
      entrypoint: './src/entrypoint.astro',
      prerender: true,
    });

^ This will work in dev. But in prod, only /blog/index.html is generated.

Complete example with static & spread paths

To clarify the bug, I have made another (not so) minimal repro -> https://stackblitz.com/edit/github-l3a17a?file=src%2F%5B...slug%5D.astro

This one have spread routes like :

          injectRoute({
            pattern: `/blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });

In dev, everything work as intended, with 5 pages generated with the correct URLs :

/ <- src/pages/index.astro
/blog <- src/blog.astro
/another-blog <- src/blog.astro
/blog/test <- src/[...slug].astro
/another-blog/another-test  <- src/[...slug].astro

Here how it's merged in build :

generating static routes 
12:06:19 ▶ src/blog.astro
12:06:19   └─ /blog/index.html (+24ms)
12:06:19 ▶ src/[...slug].astro
12:06:19   ├─ /blog/test/index.html (+29ms)
12:06:19   └─ /blog/another-test/index.html (+6ms)
12:06:19 ▶ src/pages/index.astro
12:06:19   └─ /index.html (+1ms)
12:06:19 ✓ Completed in 176ms.

We loose one the blog's index. And one article is merged into the wrong URL.

What's the expected result?

Firstly I don't see why the entrypoints influence the generation of routes, when the pattern and the collections are different. Secondly, because the behaviour differs between development and production, so if this issue is closed "as not planed" we should at least limit the behaviour in development with a warning.

Participation

[X] I am working on a pull request for this issue, see #10625

Fryuni commented 6 months ago

Confirming that this issue happens as described.

This is happening due to the page data collection returning a map from the component (the entrypoint for injected routes) to the route using that component here:

https://github.com/withastro/astro/blob/a3df9d83ca92abb5f08f576631019c1604204bd9/packages/astro/src/core/build/page-data.ts#L51-L60

To support multiple routes using the same entrypoint, this would need to be an array with all the routes with the same route.component. Changing this will also require changing the internals.routePerComponent to an array and every place that uses it. This affects quite a few places so it is not a small change, but TS should be able to guide you through it.

But before anyone starts implementing it, I think it is worth discussing, as you pointed out, which path should be taken from here:

The build should work in this case, so a fix is needed.
The build should not work in this case, so the dev server should fail in the same scenario.

goulvenclech commented 6 months ago

@Fryuni I think this should be fixed because :

I don't see why the entrypoints should influence the generation of routes, when the patterns are different
As you pointed out, the same bug problem occurred in page caching and a PR ( #10248 ) have solved it by putting a string key instead of using route.component.
Astro Integration, like mine, may want to generate multiple collections (created by the users) based on a common template. I think my project proves that there is a valid use case. The alternative is to handle everything inside a unique entrypoint like Starlight does, but it's a real gas factory and seems counterintuitive to me.

I just opened a PR #10625 , where I'll try to implement a fix inspired by #10248 .

Fryuni commented 6 months ago

I sent a commit on a branch of mine with the integration test reproducing this issue to validate it. If you haven't got to testing maybe it will help you: https://github.com/withastro/astro/commit/ccc33c3aab6765489887e18e6b9f540c19fe7382

goulvenclech commented 6 months ago

Thanks @Fryuni , I'll use this!

ematipico commented 6 months ago

To support multiple routes using the same entrypoint, this would need to be an array with all the routes with the same route.component. Changing this will also require changing the internals.routePerComponent to an array and every place that uses it. This affects quite a few places so it is not a small change, but TS should be able to guide you through it.

I went down this road before, when I was implementing the fallback system for i18n routing. The logic is exactly the same: to allow a component to generate multiple routes.

I didn't pursue this solution because:

it could be a breaking change for integrations because we pass down such information to them (unless we want to make this backwards compatible, but it requires way more work)
it's not a bug fix, but a new feature
it requires some discussion around expectations
it's more complex than you expect

If you're OK, I am going to remove the bug label for the time being.

Fryuni commented 6 months ago

The minor bug is more for the inconsistency between dev and prod. That I consider a bug.

Blocking it on dev would be fixing the bug, making it work for prod builds would be a new feature.

ematipico commented 6 months ago

I looked at the PR that OP made and they were trying to implement the feature. What's the best course of action?

Fryuni commented 6 months ago

I am personally in favor of this new feature because I'm already thinking of the fun ways I can use it to push the limits, so it's not an unbiased take.

But I think it is better to get consistent behavior first. Reaching a decision regarding this new feature and then reaching a good implementation will take some considerable time.

goulvenclech commented 6 months ago

@ematipico I'm not in favour of removing the "bug" tag—for the same reason as @Fryuni —but of course, this should need some discussion. I would love to have your opinion on the three arguments I raised above -> https://github.com/withastro/astro/issues/10622#issuecomment-2028855306

This week I have a lot of work, that's why I didn't continue this PR, but I'll come back to it soon. Here is my plan in chronological order:

Fix the bug and get the behavior I want, as a POC
Make sure this hasn't broken anything (I don't plan to change the tests apart from the one I added), especially in the public API for integrations
Clean up after myself, including improving naming and documentation.

Since this bug break my integration, I'm pretty motivated to solve it 😛 and it's not a problem if it takes some considerable time.

ematipico commented 6 months ago

So what should be the fix?

goulvenclech commented 6 months ago

@ematipico As I said before here and in the PR, for me, we should never get the pages by their component, since 1. there is no reason why two pages can't have the same entrypoint 2. there is clear use cases where integration could want to have the same entrypoint for multiple pages.

The fix should be either : a - using a key, like you did in the cache part ( #10248 ) b - using the path or the pattern, because I don't see how two pages could use the same path/pattern c - change the record for a Set or an Array

I'm currently doing my PR following the solution a, because it's coherent with your PR (that has been merged before). But I'm in favour of the solution b or c, but keeping in mind that the solution C could break more things along the way.

ematipico commented 6 months ago

Is there a place in the docs where this topic isn't explained very well or lives in some room of interpretation?

I ask this because, as far as I know, it's already possible for a component to emit multiple paths, and that's done using the spread route e.g. [...dynamic]

goulvenclech commented 6 months ago

@ematipico I'm sorry, but I don't think you understood the bug. What can I clarify in my or @Fryuni 's messages to help you?

The problem is not about spread route. Nor emitting multiple paths for a given entrypoint. @Fryuni & myself knows that this feature works, since we use it daily.

The problem is about the integration API & and how we get pages (by components/entrypoints) in the build process, making it impossible for the function injectRoutes() (only in prod) to inject multiples (static or spread) routes with the same entrypoint.

This problem is for both spread routes and static routes (see the test & my use case), is a bug because differs from dev (correct behaviour) and production (merged routes), no error nor warning are raised, and this limitation is not documented.

goulvenclech commented 6 months ago

To clarify the bug, I have made another (not so) minimal repro -> https://stackblitz.com/edit/github-l3a17a?file=src%2F%5B...slug%5D.astro

This one have spread routes like :

          injectRoute({
            pattern: `/blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog`,
            entrypoint: './src/blog.astro',
            prerender: true,
          });
          injectRoute({
            pattern: `/another-blog/[...slug]`,
            entrypoint: './src/[...slug].astro',
            prerender: true,
          });

In dev, everything work as intended, with 5 pages generated with the correct URLs :

/ <- src/pages/index.astro
/blog <- src/blog.astro
/another-blog <- src/blog.astro
/blog/test <- src/[...slug].astro
/another-blog/another-test  <- src/[...slug].astro

Here how it's merged in build :

generating static routes 
12:06:19 ▶ src/blog.astro
12:06:19   └─ /blog/index.html (+24ms)
12:06:19 ▶ src/[...slug].astro
12:06:19   ├─ /blog/test/index.html (+29ms)
12:06:19   └─ /blog/another-test/index.html (+6ms)
12:06:19 ▶ src/pages/index.astro
12:06:19   └─ /index.html (+1ms)
12:06:19 ✓ Completed in 176ms.

We loose one the blog's index. And one article is merged into the wrong URL.

goulvenclech commented 6 months ago

Just updated my initial post with this second example.

ematipico commented 6 months ago

Thank you, @goulvenclech, for clarifying the use case and apologies for juggling too much on this issue.

It would be great if we could also follow up with a docs PR to also show the usage we are going to fix, as proof of user expectations of the API: https://docs.astro.build/en/reference/integrations-reference/#injectroute-option

withastro / astro