Implement priority overrides for injected routes and redirects #9439

Fryuni · 2023-12-15T03:09:11Z

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 · 2023-12-15T03:09:17Z

🦋 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 · 2023-12-15T03:14:36Z

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 · 2023-12-15T12:15:54Z

packages/astro/src/core/routing/manifest/create.ts

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


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.

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

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 · 2023-12-15T12:19:52Z

packages/astro/src/core/config/schema.ts

@@ -173,6 +173,7 @@ export const AstroConfigSchema = z.object({
 						z.literal(308),
 					]),
 					destination: z.string(),
+					priority: z.enum(['above-project', 'same-as-project', 'below-project']).optional(),


I believe we should at least provide a default.

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.

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?

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

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.

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

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

Using a transform now!

ematipico · 2023-12-15T12:20:39Z

packages/astro/src/@types/astro.ts

+ * 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.


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

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

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.

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.

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

I like that direction @sarah11918!

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.

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?

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!

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

ematipico · 2023-12-15T12:22:27Z

packages/astro/src/core/routing/manifest/create.ts

+			.map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content))
+			.join('/')}`.toLowerCase();
+
+		routes[priority ?? 'above-project'].push({


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

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.

It would be great to leave a comment to explain that

Added comments for both defaults

packages/astro/src/@types/astro.ts

# Conflicts: # packages/astro/src/core/routing/manifest/create.ts

florian-lefebvre · 2023-12-17T12:27:44Z

Do you think it can close #6221 ?

Fryuni · 2023-12-17T18:17:28Z

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 · 2024-01-08T15:13:53Z

packages/astro/src/@types/astro.ts

+ * - "defer": Defer to any file-based project route in case of conflict. Conflicts within defer
+ *   routes are resolved by the same rules as file-based routes.
+ */
+export type RoutePriorityOverride = 'override' | 'normal' | 'defer';


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

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

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

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

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.

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

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.

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

UX with a notFoundRoute: false option:

User creates a 404.astro

They get an error about route collisions (see Subpath: This route collides with: "src/pages/404.astro". starlight#1080)

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

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:

User creates a 404.astro. It works!

sarah11918 · 2024-01-08T15:14:54Z

.changeset/smooth-cobras-help.md

+'astro': minor
+---
+
+Reworks route priority processing to allow for more flexible and intuitive redirects and route injection


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.

packages/astro/src/core/config/schema.ts

packages/astro/src/core/routing/manifest/create.ts

ematipico · 2024-01-10T14:17:42Z

packages/astro/src/core/routing/manifest/create.ts

-	} catch (e) {
-		resolved = fileURLToPath(new URL(entrypoint, config.root));
+	// endpoints take precedence over pages
+	if ((a.type === 'endpoint') !== (b.type === 'endpoint')) {


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

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.

packages/astro/src/core/routing/manifest/create.ts

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!

.changeset/short-keys-bow.md

.changeset/smooth-cobras-help.md

packages/astro/src/@types/astro.ts

.changeset/smooth-cobras-help.md

Co-authored-by: Luiz Ferraz <luiz@lferraz.com>

.changeset/short-keys-bow.md

.changeset/smooth-cobras-help.md

packages/astro/src/@types/astro.ts

yanthomasdev

Found two small nits, overall the docs LGTM.

.changeset/short-keys-bow.md

packages/astro/src/@types/astro.ts

Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>

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?

…riority

Fryuni added 5 commits December 14, 2023 21:23

Implement priority overrides for injected routes and redirects

9f6a7e9

Fix ordering for route specificity

a6671a8

Don't mix rules on tests

bf8a333

Detailed collision detection

31885a9

Add changeset

06bcec2

github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Dec 15, 2023

Remove TODO

8a61bb7

ematipico reviewed Dec 15, 2023

View reviewed changes

Merge branch 'main' into feat/injected-route-priority

a475688

# Conflicts: # packages/astro/src/core/routing/manifest/create.ts

delucis mentioned this pull request Dec 17, 2023

Subpath: This route collides with: "src/pages/404.astro". withastro/starlight#1080

Closed

1 task

Fryuni added 2 commits December 18, 2023 15:35

Add comments to clarify default values

c211704

Merge branch 'main' into feat/injected-route-priority

0c678b2

florian-lefebvre mentioned this pull request Dec 20, 2023

Inconsistent route priority with rest parameters and configured redirects #8129

Closed

1 task

Update terminology

7379e1d

Fryuni requested review from delucis, sarah11918, matthewp and ematipico December 20, 2023 22:41

Revert unrelated changes

a064b4e

sarah11918 reviewed Dec 24, 2023

View reviewed changes

matthewp added the semver: minor Change triggers a `minor` release label Dec 26, 2023

Princesseuh added this to the 4.2.0 milestone Jan 2, 2024

sarah11918 reviewed Jan 8, 2024

View reviewed changes

ematipico requested changes Jan 10, 2024

View reviewed changes

Fryuni added 5 commits January 15, 2024 14:45

chore: Update test

eb855b9

chore: Change remaining new error to warning

a8686b8

chore: Test collision warnings

e319eb4

docs: Update docs of new config

bd93047

docs: Improve changesets

de3a8cb

ematipico approved these changes Jan 16, 2024

View reviewed changes

ematipico added 2 commits January 16, 2024 14:58

chore: rename experimental flag

fdc7c90

chore: update changeset and docs

5671f41

sarah11918 reviewed Jan 16, 2024

View reviewed changes

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved

Sarah editing pass

24c9c74

Fryuni commented Jan 16, 2024

View reviewed changes

.changeset/smooth-cobras-help.md Outdated Show resolved Hide resolved

Fryuni and others added 2 commits January 16, 2024 14:09

nit: Align Markdown table

7fc2482

defined definitions!

30241b6

Co-authored-by: Luiz Ferraz <luiz@lferraz.com>

sarah11918 reviewed Jan 16, 2024

View reviewed changes

.changeset/short-keys-bow.md Outdated Show resolved Hide resolved

sarah11918 reviewed Jan 16, 2024

View reviewed changes

.changeset/smooth-cobras-help.md Show resolved Hide resolved

sarah11918 reviewed Jan 16, 2024

View reviewed changes

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved

added logging info to docs for experimental flag

fb78e03

yanthomasdev reviewed Jan 16, 2024

View reviewed changes

.changeset/short-keys-bow.md Outdated Show resolved Hide resolved

packages/astro/src/@types/astro.ts Outdated Show resolved Hide resolved

Yan final boss review

dcf8ca4

Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com>

sarah11918 approved these changes Jan 16, 2024

View reviewed changes

Fryuni and others added 3 commits January 16, 2024 17:10

chore: Update flag name in tests

9882de1

chore: Update flag name in tests

a34647e

Merge remote-tracking branch 'origin/main' into feat/injected-route-p…

dd8c15f

…riority

ematipico merged commit fd17f4a into withastro:main Jan 17, 2024
13 of 14 checks passed

astrobot-houston mentioned this pull request Jan 17, 2024

[ci] release #9709

Merged

Fryuni mentioned this pull request Jan 24, 2024

Add support for SSR withastro/starlight#1255

Merged

3 tasks

This was referenced Mar 21, 2024

Astro: Redirect routes cause some directories to fail matching on S3 sst/sst#3717

Closed

astro-fix: prevent dead-end nodes from resulting in 404 sst/sst#3720

Merged

Fryuni deleted the feat/injected-route-priority branch April 5, 2024 00:11

Implement priority overrides for injected routes and redirects #9439

Implement priority overrides for injected routes and redirects #9439

Conversation

Fryuni commented Dec 15, 2023 • edited Loading

Changes

Collision detection

Previous divergence from the docs

New divergence from the docs

Breaking changes... kinda

Summary of the changes

Testing

Docs

Relations

changeset-bot bot commented Dec 15, 2023 • edited Loading

🦋 Changeset detected

Fryuni commented Dec 15, 2023

ematipico left a comment

Choose a reason for hiding this comment

ematipico Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Fryuni Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delucis Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florian-lefebvre commented Dec 17, 2023

Fryuni commented Dec 17, 2023

sarah11918 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryuni Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico left a comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

yanthomasdev left a comment

Choose a reason for hiding this comment

sarah11918 left a comment

Choose a reason for hiding this comment

Fryuni commented Dec 15, 2023 •

edited

Loading

changeset-bot bot commented Dec 15, 2023 •

edited

Loading

ematipico Dec 15, 2023 •

edited

Loading

Fryuni Dec 15, 2023 •

edited

Loading

delucis Dec 15, 2023 •

edited

Loading

Fryuni Jan 9, 2024 •

edited

Loading