astro icon indicating copy to clipboard operation
astro copied to clipboard

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

Open FugiTech opened this issue 2 years ago • 3 comments

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.

FugiTech avatar Jun 15 '22 09:06 FugiTech

CC @thepassle

delucis avatar Jun 15 '22 10:06 delucis

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?

thepassle avatar Jun 15 '22 10:06 thepassle

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

FugiTech avatar Jun 15 '22 11:06 FugiTech

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 avatar Aug 22 '22 04:08 FredKSchott