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

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

Closed
1 task done
FugiTech opened this issue Jun 15, 2022 · 8 comments
Closed
1 task done
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: integration Related to any renderer integration (scope)

Comments

@FugiTech
Copy link
Contributor

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

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

delucis commented Jun 15, 2022

CC @thepassle

@thepassle
Copy link
Contributor

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

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).

@natemoo-re natemoo-re added - P3: minor bug An edge case that only affects very specific usage (priority) s1-small pkg: integration Related to any renderer integration (scope) labels Jun 15, 2022
@FredKSchott
Copy link
Member

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.

@FredKSchott FredKSchott added - P2: nice to have Not breaking anything but nice to have (priority) and removed - P3: minor bug An edge case that only affects very specific usage (priority) labels Aug 22, 2022
@matthewp
Copy link
Contributor

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

@goulvenclech
Copy link
Contributor

goulvenclech commented Mar 31, 2024

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.

  • 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 ).
  • I can provide an up-to-date minimal reproduction, if the one given by @FugiTech is obsolete.

@Fryuni
Copy link
Member

Fryuni commented Mar 31, 2024

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

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 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

No branches or pull requests

8 participants