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

Injected SSR routes not following routing priority order #9392

Closed
1 task done
Fryuni opened this issue Dec 10, 2023 · 4 comments · Fixed by #9439
Closed
1 task done

Injected SSR routes not following routing priority order #9392

Fryuni opened this issue Dec 10, 2023 · 4 comments · Fixed by #9439
Labels
- P2: nice to have Not breaking anything but nice to have (priority)

Comments

@Fryuni
Copy link
Member

Fryuni commented Dec 10, 2023

Astro Info

$ astro info
Astro                    v4.0.3
Node                     v18.18.0
System                   Linux (x64)
Package Manager          yarn
Output                   hybrid
Adapter                  @astrojs/node
Integrations             test-integration

If this issue only occurs in one browser, which browser is a problem?

Any browser

Describe the Bug

Non-prerendered routes added by integrations override any existing matching route.

What's the expected result?

The injected route should follow the same route priority order as if it were part of the project.

Link to Minimal Reproducible Example

With project route, working fine: https://stackblitz.com/edit/github-lukzfz-qbzgtv

With injected route, not working: https://stackblitz.com/edit/github-lukzfz-wradsh

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Dec 10, 2023
@Fryuni
Copy link
Member Author

Fryuni commented Dec 10, 2023

This might be related to #7721 since both are about problems with injectRoute and pre-rendering.

I found both bugs while working on the same block of code for Starlight SSR support.

@ematipico ematipico added - P2: nice to have Not breaking anything but nice to have (priority) and removed needs triage Issue needs to be triaged labels Dec 11, 2023
@ematipico
Copy link
Member

This isn't something that Astro supports at the moment, although we plan to address it in the coming weeks.

@Fryuni
Copy link
Member Author

Fryuni commented Dec 11, 2023

It looks like a simple fix. The routes are being sorted separately:

The routes discovered in the project are sorted before being added to the list of routes:

items = items.sort(comparator);

Then the injected routes are sorted between themselves:

?.sort((a, b) =>
// sort injected routes in the same way as user-defined routes
comparator(injectedRouteToItem({ config, cwd }, a), injectedRouteToItem({ config, cwd }, b))
)

Then the injected routes are added to the front of the list (explicitly making them overwrite any project route, but this is not documented on injectRoute):

// the routes array was already sorted by priority,
// pushing to the front of the list ensure that injected routes
// are given priority over all user-provided routes
routes.unshift({
type,
route,
pattern,
segments,
params,
component,
generate,
pathname: pathname || void 0,
prerender: prerenderInjected ?? prerender,
fallbackRoutes: [],
});

Personally, I think injected routes should follow the same priority order, so the sorting should be done after both sets of routes are added to the list.

We could have an extra option in the injectRoute, something like projectConflicts?: 'override' | 'fallback' | 'dynamicRules'. The default for now could be override for backwards compatibility.

As for the priority, this makes Starlight override any and every custom route when running astro dev, even though those routes will work after running astro build since then it will comply with the getStaticPaths, so I don't think this is a "nice to have". At least not the difference in behavior between astro dev and astro build.

I'm currently patching Astro to have Starlight work properly on the sites I maintain, here is one public (my blog):
https://gitlab.com/Fryuni/blog/-/blob/225856156bbec77b286c926b7a880a42636b4d4d/patches/astro+4.0.3.patch

I can send a PR for this if the proposed design (with the option for controlling) is agreeable.

@matthewp
Copy link
Contributor

We've been wanting to tackle this for a while but haven't had the time to do so. I'm not sure it's a simple fix, however, but closer to a refactor of that module. Currently the file-based routes are sorted during the crawl, not altogether afterwards, which would make it easier to fix.

If you want to give this a try though, I think we could figure out a way to do it safely. In addition to injected routes there is also redirects which are similarly sorted separately, after the rest. I would want those to also be sorted together with the other routes.

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)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants