Implement priority overrides for injected routes and redirects

[Fryuni]

Changes

Refactor route sorting logic to follow the priority order as specified in the docs in order:

• Static routes without path parameters will take precedence over all other routes
• Dynamic routes using named parameters take precedence over rest parameters
• Pre-rendered dynamic routes take precedence over server dynamic routes
• Rest parameters have the lowest priority
• Endpoints always take precedence over pages
• Ties are resolved alphabetically

Injected routes and redirects will now be sorted alongside routes from project files, with an option to switch to the previous "legacy" behavior.

The "legacy" behavior for redirects is to be sorted below any other route and for injected routes is to be sorted above.

Collision detection

The previous collision detection relied on routes being sorted in steps, which is exactly what this PR intends to remove, so this also changes the collision detection. Cases like /[foo] (redirect) and /[bar] (ssr route) are now reported as collisions.

Routes that may result in a collision depending on their returns on getStaticPaths are not reported as colliding. Routes that only partially collide are also not reported since that may be intentional (like /[...slug] and /api/[operation]).

Previous divergence from the docs

Static redirects and dynamic pages did not resolve as documented in the previous code.

Example: /foo (redirect) and /[slug] (page).
According to the docs, the redirect should be shadowed by the page route.
But by the previous code, the redirect would be sorted above the page.

New divergence from the docs

The docs do not specify how to handle scenarios that differ only in specificity, like /[slug] and /foo/[slug]. So this PR includes an extra rule to sort more specific routes higher than less specific routes. So:

• /blog/[...slug] (page) is sorted above /[...slug] (page). This was sorted the same way previously due to how the items were sorted as they were crawled.
• /blog/[...slug] (injected page) is sorted above /[...slug] (injected page). This was previously kept in the order that injectRoute was called (BREAKING CHANGE).

Breaking changes... kinda

• /blog/[...slug] (injected page) is sorted above /[...slug] (injected page). This was previously kept in the order that injectRoute was called.
• /foo (redirect) is now sorted below /[slug] (page) by default, as documented.

Since those changes in behavior were not tested and either not documented or behaving differently from what was documented, I considered them more bug fixes than breaking changes, so I didn't set the changeset to major.

I can change it to major if that is the prevailing decision.

Summary of the changes

• Better collision reporting when the collision is certain
• Injected routes default to be sorted alongside project routes
• Redirects default to be sorted alongside project routes
• Routes sorted using the following rules in order of relevance:
  • More specific routes (with more segments) take precedence over less specific routes
  • Static routes (without any path parameter) take precedence over dynamic routes
  • Dynamic routes using named parameters take precedence over rest parameters
  • Pre-rendered routes take precedence over server routes (not just for dynamic routes)
  • Endpoints take precedence over pages
  • Lastly, routes are sorted alphabetically

Testing

Unit tests were added for all the cases mentioned above. If the team thinks it is necessary, I can also add extra integration tests.

Docs

The changes in behavior are described above.
The docs should be updated to use the rules from the summary above in the "Route Priority Order" section.
The new priority option should be added in the reference documentation for redirects and injectRoute.

I can send the docs PR as soon as the new behavior is approved.

Relations

• Closes Injected SSR routes not following routing priority order #9392

[changeset-bot]

🦋 Changeset detected

Latest commit: dd8c15f

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Fryuni]

I'm not sure about the names of the priorities. They are technically correct but might also be confusing. I also considered the following alternatives:

• override/normal/fallback
• higher/normal/lower

Any other ideas?

[ematipico]

Some preliminary comment

[ematipico]

I don't think using spread operators is the correct choice here. It seems we always create new arrays, which is a hit memory-wise.

I believe we should mutate the ones we have and create only one.

E.g.

const routes = [];
routes.concat(
	injectedRoutes['above-project'].concat(
		redirectRoutes['above-project']
	).sort(comparator)
);

I think this is safe because we aren't going to use injectedRoutes and redirecteRoutes anymore after this point.

[Fryuni]

I don't think it will make much of a difference. Each .concat is a possible allocation while multiple spread operations inside the same [] are always a single allocation with the correct size (the sum of the length of all the arrays being spread).

Safety-wise you are correct, using concat is not a safety problem in this case.

I'll try to run some benchmarks for that

[Fryuni]

I stand corrected. concat takes half the time of the spread operator, unless the function is called enough times for all JIT passes, then it is just 2% faster (but still consistently faster).

Although I just tested this for 3.5k routes, 500 each of (injected above, redirect before, project, inject with project, redirect with project, injected below, redirect below) and it took about 0.5ms even with JIT disabled, so it might not be a worth optimization at the cost of readability.

I think the code with the spreads is more clear about what would be the final ordering than this:

const routes = injectedRoutes['above-project']
  .concat(redirectRoutes['above-project'])
  .sort(comparator)
  .concat(
    projectRoutes
      .concat(injectedRoutes['same-as-project'], redirectRoutes['same-as-project'])
      .sort(),
    injectedRoutes['below-project']
      .concat(redirectRoutes['below-project'])
      .sort()
  );

[ematipico]

I believe we should at least provide a default.

[Fryuni]

The default is added where it is used, but I can also add it here.

I didn't do it because then there would be a risk of the defaults becoming out of sync. If the redirect uses just the destination string instead of an object, there is no priority in the configuration, so the default has to be where it is used.

[ematipico]

so the default has to be where it is used.

From this phrase, I understand "where it is used" and this configuration aren't the same. Did I understand correctly?

[Fryuni]

Yes, I mean the code on create.ts that uses the value from the configuration to do something.

[delucis]

Is the configuration ever not passed through this schema?

I think what @ematipico is suggesting is this:

Suggested change

	priority: z.enum(['above-project', 'same-as-project', 'below-project']).optional(),
	priority: z
	.enum(['above-project', 'same-as-project', 'below-project'])
	.default('above-project'),

This ensures priority is always defined and no checks need to be made later.

[Fryuni]

Redirects can be just a string:

{
  redirects: {
    '/foo': '/bar'
  }
}

So the code code on create.ts would have to duplicate the default anyway. We transform it here... I'll send an option for that

[delucis]

You could use a Zod transform to do that upfront too, iiuc!

[Fryuni]

Using a transform now!

[ematipico]

@withastro/maintainers-docs You should focus here. Naming is very important here

[matthewp]

I think the naming should involve file-based routing somehow, rather than project. Took me a second to realize that's what project meant.

[delucis]

I also wonder if “above”/“below” is ideal. I understand why it was chosen, but I associate high/low more with priority.

Maybe something like this could work? @sarah11918 is also a good person to bounce these off. I don’t love “file-system” here — not sure it’s super clear that this means “file-based route” or if there’s something clearer.

Could there ever be other kinds of priority? If not, it could be redundant to mention that in the enum values and the options could just be high/default/low or something? With the relationship explained in the documentation.

Suggested change

	* IDs for different priorities of injected routes and redirects:
	* - "above-project": Override any project route in case of conflict.
	* - "same-as-project": Behave the same as if the route was defined in the project, following the same priority rules.
	* - "below-project": Only match if no project route matches.
	* IDs for different priorities of injected routes and redirects:
	* - "higher-than-file-system": Override any project route in case of conflict.
	* - "same-as-file-system": Behave the same as if the route was defined in the project, following the same priority rules.
	* - "lower-than-file-system": Only match if no project route matches.

[Fryuni]

Another option would be to use a number, with all project routes having priority zero, injected routes defaulting to a priority of -1 and redirects defaulting to a priority of 1.

Then the other rules apply for when this number is the same.

This may be easier to document and even more flexible. Maybe too flexible and would make things confusing for integration developers.

[delucis]

Yes, I’d caution against that I think. Even if internally the enum maps to -1 | 0 | 1, allowing people to potentially try to outbid each other with ever higher numbers seems really risky. (And doesn’t actually help with clarity high or higher-than-file-system tell you something quite explicit, where 1 is pretty meaningless on its own.)

[delucis]

I like that direction @sarah11918!

[Fryuni]

I also liked @sarah11918's suggestion. I'll send a commit with those and change from just "project routes" to "project file-based routes" to make it clear what they are relative to.

[Fryuni]

Just sent a commit with a new suggestion. I first used the terms by @sarah11918, but it felt weird to have a priority named "default" that is not the default, so I went with "merge".

Also changed the "project routes" to clarify that those are the routes inferred from files.

What do you all think? Better, worse, still confusing?

[sarah11918]

Right, I agree not to use default as an option that is not the default. 😀

merge could work. Synonyms would be like integrate or incorporate or maybe even include.

If you wanted the "conform" (follow the rules) angle, then other synonyms could be follow, obey, observe, match, comply.

I'm not sure I have a strong preference. Just highlighting some options to see if one immediately jumps out at you!

[Fryuni]

Out of those, I think I still prefer merge, with incorporate as a close second.

[ematipico]

Why is the default here above-project and at line 441 is below-project?

[Fryuni]

Injected routes default to be above the project, and redirects default to be below the project.

Redirects defaulting to being below is what was documented and previously implemented.

Injected routes defaulting to being above were not documented, but they were already implemented like that, and from my quick test, it seems some integrations rely on that behavior, so this is to minimize breaking changes to just the edge cases described in the PR.

[ematipico]

It would be great to leave a comment to explain that

[Fryuni]

Added comments for both defaults

[florian-lefebvre]

Do you think it can close #6221 ?

[Fryuni]

Do you think it can close #6221 ?

Seems like the same problem to me.

It would require change on the integration registering the catch-all route to use a lower priority than the default since this keeps the current behavior by default

[sarah11918]

Just adding to remove the request for review since I've given my feedback below

[sarah11918]

Is the plan for this to be user-configurable @Fryuni ? If so, where/how?

If in astro.config.mjs then this file will also need a corresponding @docs entry.

Also reminder that you will also need an accompanying PR to update: https://docs.astro.build/en/core-concepts/routing/#route-priority-order

(FYI It's OK if this update doesn't entirely duplicate the config option, but instead just links to it, if you're also adding it to config reference. It might be most helpful if this sections lists out the default, then links to the configurable option for "fine-tuning")

[Fryuni]

On redirects this is user-configurable, on routes this is configurable for integrations.

[matthewp]

I don't think injected routes should have the ability to decide how they should be prioritized. I think the only options should be "normal" (ie, the same as if they were file-based routes) and maybe something called "legacy" (the old behavior, only for backwards compatibility, to be removed in next major).

[Fryuni]

I think the customization is positive for integrations.

For example, Starlight injects a 404 route. With this change it could be injected with priority: 'defer' allowing projects using Starlight to define a custom 404 page. As it is now this is impossible, Starlight will always override the 404 page.

Also, if we change to have a "normal" and "legacy", I'd expect the "normal" to be the default. But that would be a breaking change. I'm not sure how much of an impact this would be on integrations in the wild, but IMHO it would require a major version by itself.

It wasn't something documents though, so I'm not too strongly of that opinion. That was the implemented behavior but not necessarily intended. So maybe it is fine to break integrations that rely on it. I don't know

[ematipico]

I think the customization is positive for integrations.

That's a fact, and I agree with your point. However, we risk not providing predictability to the end users, who need to learn what integrations do. While it's OK to change. In the prioritization logic, we also make sure that the user is protected from what integrations do.

I believe we can progressively provide integrations more control, although we should first fix the predictability of the prioritization.

[matthewp]

@Fryuni It should be possible for Starlight to define 404 via middleware. I think a lot of what Starlight does might be better via middleware. Currently middleware is a little limited and can't do something like 404, we are in the process of fixing that though.

[Fryuni]

Rethinking about this, if an integration wants to provide a route that can be overridden by the project it is on, it could just allow disabling the route. If Starlight wants to allow overriding the 404 page it could have a notFoundRoute: false and not even call injectRoute in that case.

Considering that I don't see much advantage of allowing these different priorities.

[delucis]

I was hoping to avoid exactly that kind of option with the priority system.

UX with a notFoundRoute: false option:

1. User creates a 404.astro
2. They get an error about route collisions (see Subpath: This route collides with: "src/pages/404.astro". starlight#1080)
3. They need to work out Starlight’s option exists — likely not something I can add helpful logging for in Starlight because the error is Astro’s
4. Maybe they set notFoundRoute: false if they find it, maybe they assume this isn’t supported.

UX with a lower route priority when injecting Starlight’s 404:

1. User creates a 404.astro. It works!

[sarah11918]

Let me know when this is considered finalized behaviour/naming etc, and please /ptal me for a review at that time!

[ematipico]

I finally had some time to look into this PR. Thank you for waiting.

As hinted by Matthew, we would like to reduce the goal of the PR to predictable results. While giving the ablity to integration developers to change the priority of routes is tempting, it doens't make that predictable, especially when multiple integrations start injecting their routes, which eventually could collide.

What we could do, instead, is to error in case there are multiple routes that collide, and explain to the user why they collide.

I believe we can tag a route with the integration name, so we know who injected the routes , and tell it to the user. In case this occurs, there isn't much we can do.

[ematipico]

If the endpoints take precedence, wouldn't make more sense to move this check at the beginning of the function?

[Fryuni]

They take precedence after the previous rules. So if both an endpoint and a page are static/dynamic/rest and both have the same number of segments, the endpoint will go first.

[ematipico]

Great work @Fryuni! Thank you very much for the time you spent on this one, and thank you for your immense patience, considering the scope change from the initial implementation.

Now that we ship this as an experimental feature, we can slowly add new APIs to allow better control of routes from integrations.

[sarah11918]

OK, assuming that I have the content right, here's my suggestion for both changesets and docs!

[yanthomasdev]

Found two small nits, overall the docs LGTM.

[sarah11918]

Giving my final, official approval, while still pointing out that since this is a minor release anyway, maybe minor makes more sense than patch if we think this could change how some people's projects behave?