🐛 BUG: injectRoute overwrites existing routes if entrypoint is already used

FugiTech commented 2 years ago

What version of `astro` are you using?

v1.0.0-beta.46

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

yarn

What operating system are you using?

Windows (WSL2 w/ Ubuntu)

Describe the Bug

Calling injectRoute with an entrypoint already used by an existing page in src/pages or by another call to injectRoute will cause the previous route to be overwritten. This happens regardless of the value of pattern, it does not matter if the pattern is static or dynamic. For simplicity, the minimal reproducible example uses a static pattern.

From my brief investigation, this seems to be related to collectPagesData which iterates over all registered routes and builds a map of entrypoint -> PageBuildData. Since it assigns rather than merging, it causes prior valid PageBuildData to be lost.

I'd be willing to file a PR but would first want to confirm that changing the result to an Array and having entries with duplicate entrypoints is acceptable.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-cwhwmd?file=astro.config.mjs

Participation

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

delucis commented 2 years ago

CC @thepassle

thepassle commented 2 years ago

Thats interesting, we do have a check to see if there is a route collision: https://github.com/withastro/astro/blob/main/packages/astro/src/core/routing/manifest/create.ts#L327-L332 That should throw an error if there are route collisions. Could you try debugging that and seeing why it doesnt throw the error?

FugiTech commented 2 years ago

That check detects if there is a collision in the pattern, it does not check if there is a collision in the entrypoint (also referred to in other parts of the code as componentPath).

If I have src/pages/index.astro, Astro's normal build process will give that a route of /. If I then write a plugin that calls

injectRoute({
  pattern: '/ja/',
  entryPoint: 'src/pages/index.astro',
})

then the collision check will compare / to /ja/ and see no collision (which I believe is correct), and then collectPagesData will see two routes using src/pages/index.astro and will only use the latter (which I believe is incorrect).

FredKSchott commented 2 years ago

Interesting@ This was not the intended use-case of the integration API. It was meant to be so that you could create routes using files outside of src/pages directory. I agree that this is a bug, but I would recommend avoiding this pattern if you can, since I am not sure how well supported this will be.

matthewp commented 1 year ago

Closing as we're not going to change this behavior. Injected routes come last and override any routes that might also match.

goulvenclech commented 6 months ago

Hi everyone 👋

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. This is caused by using the same .astro entrypoint for two or more injectRoutes.

Injected routes come last and override any routes that might also match.

@matthewp Not the problem, in my case the injects match different patterns, which work very well locally :

  archetypes.forEach((archetype) => {
    // Inject a route for each archetype index
    injectRoute({
      pattern: `/${archetype.path}`,
      entrypoint: getEntryPoint(archetype),
      prerender: true,
    })
  })
  archetypes.forEach((archetype) => {
    // Inject a route for each archetype dynamic route
    injectRoute({
      pattern: `/${archetype.path}/[...slug]`,
      entrypoint: getDynamicEntryPoint(archetype),
      prerender: true,
    })
  })

The only explanation is that I use the same entrypoint several times, for two "archetypes" of the same type, to allow users to generate several blog collections for example.

function getEntryPoint(archetype: Archetype): string {
  switch (archetype.type) {
    case "blog-content":
      return "@goulvenclech/astropi/pages/blog.astro"
    case "docs-content":
      return "@goulvenclech/astropi/pages/docs-content.astro"
    case "docs-openapi":
      return "@goulvenclech/astropi/pages/docs-openapi.astro"
    default:
      return "@goulvenclech/astropi/pages/index.astro"
  }
}

I think this issue should be re-opened. Firstly because 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.

[x] I am willing to submit a pull request for this issue, if I'm a little oriented on where to look (I started discussing this with @Princesseuh ).
[x] I can provide an up-to-date minimal reproduction, if the one given by @FugiTech is obsolete.

Fryuni commented 6 months ago

Hi @goulvenclech, can you double-check whether you are on the latest version of Astro?

There was a PR recently (#10248) that was supposed to fix the cache collision causing routes with the same entrypoints to misbehave. The example case there was fixed, if there is another or if there was a regression since then you can open a new issue with a reproduction :)

goulvenclech commented 6 months ago

Thanks @Fryuni for your response, but this is not a cache bug, rather in the prod build. I'm running on the latest Astro version ^4.5.12.

I'm gonna create a new issue !

withastro / astro