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

Implement priority overrides for injected routes and redirects #9439

Merged
merged 37 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
37 commits
Select commit Hold shift + click to select a range
9f6a7e9
Implement priority overrides for injected routes and redirects
Fryuni Dec 15, 2023
a6671a8
Fix ordering for route specificity
Fryuni Dec 15, 2023
bf8a333
Don't mix rules on tests
Fryuni Dec 15, 2023
31885a9
Detailed collision detection
Fryuni Dec 15, 2023
06bcec2
Add changeset
Fryuni Dec 15, 2023
8a61bb7
Remove TODO
Fryuni Dec 15, 2023
a475688
Merge branch 'main' into feat/injected-route-priority
Fryuni Dec 15, 2023
c211704
Add comments to clarify default values
Fryuni Dec 18, 2023
0c678b2
Merge branch 'main' into feat/injected-route-priority
Fryuni Dec 20, 2023
7379e1d
Update terminology
Fryuni Dec 20, 2023
a064b4e
Revert unrelated changes
Fryuni Dec 21, 2023
cea5439
WIP
Fryuni Jan 13, 2024
9dd4609
Merge branch 'main' into feat/injected-route-priority
Fryuni Jan 13, 2024
addd2d9
Refactor
Fryuni Jan 13, 2024
e3cbbe9
Fix typo and typing
Fryuni Jan 14, 2024
d5e2142
Merge branch 'main' into feat/injected-route-priority
ematipico Jan 15, 2024
322bb4d
chore: default to legacy
ematipico Jan 15, 2024
28df92f
chore: use experimental flag instead of option
ematipico Jan 15, 2024
a368d60
fix: do not throw an error on collisions
ematipico Jan 15, 2024
dc2760a
chore: fix regression
ematipico Jan 15, 2024
7cfbdbd
chore: use `continue` instead of `return`
ematipico Jan 15, 2024
61fc289
chore: fix tests but one
ematipico Jan 15, 2024
eb855b9
chore: Update test
Fryuni Jan 15, 2024
a8686b8
chore: Change remaining new error to warning
Fryuni Jan 15, 2024
e319eb4
chore: Test collision warnings
Fryuni Jan 15, 2024
bd93047
docs: Update docs of new config
Fryuni Jan 16, 2024
de3a8cb
docs: Improve changesets
Fryuni Jan 16, 2024
fdc7c90
chore: rename experimental flag
ematipico Jan 16, 2024
5671f41
chore: update changeset and docs
ematipico Jan 16, 2024
24c9c74
Sarah editing pass
sarah11918 Jan 16, 2024
7fc2482
nit: Align Markdown table
Fryuni Jan 16, 2024
30241b6
defined definitions!
sarah11918 Jan 16, 2024
fb78e03
added logging info to docs for experimental flag
sarah11918 Jan 16, 2024
dcf8ca4
Yan final boss review
sarah11918 Jan 16, 2024
9882de1
chore: Update flag name in tests
Fryuni Jan 16, 2024
a34647e
chore: Update flag name in tests
Fryuni Jan 16, 2024
dd8c15f
Merge remote-tracking branch 'origin/main' into feat/injected-route-p…
ematipico Jan 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions .changeset/smooth-cobras-help.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
'astro': minor
---

Reworks route priority processing to allow for more flexible and intuitive redirects and route injection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


- Priority order for project routes, injected routes and redirects are now all the same.
- Injected routes and redirects can now specify if they should be prioritized above,
with or below project routes. This is done by adding a `priority` property to the route
object in `injectRoute` and in the `redirects` property in `astro.config.mjs`.
- Now more specific routes always take priority over less specific routes.
Example: `/blog/[...slug]` will take priority over `/[...slug]`
- Static redirects now have a lower priority than all project routed, even if the routes are dynamic,
matching the already documented behavior.
Example: `/blog (redirect)` will no longer override a `/[slug]` route by default, this can be re-enabled
using the new `priority` field.
- Collision detection between routes can now detect coliisions between similar dynamic routes
of any kind (project routes, injected routes and redirects).
Example: `/blog/[page]` will now be detected as a collision with `/blog/[slug]`
- Colision detection is now reported as a warning for backward compatibility with all the previous false negatives.
13 changes: 13 additions & 0 deletions packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1555,10 +1555,22 @@ export interface AstroUserConfig {
*/
export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | 'page-ssr';

/**
* IDs for different priorities of injected routes and redirects:
Fryuni marked this conversation as resolved.
Show resolved Hide resolved
* - "override": Override any file-based project route in case of conflict. Conflicts within override
* routes are resolved by the same rules as file-based routes.
* - "merge": Merge with discovered file-based project routes, behaving the same as if the route
* was defined as a file in the project.
* - "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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@Fryuni Fryuni Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


export interface InjectedRoute {
pattern: string;
entrypoint: string;
prerender?: boolean;
priority?: RoutePriorityOverride;
}

export interface ResolvedInjectedRoute extends InjectedRoute {
Expand Down Expand Up @@ -2385,6 +2397,7 @@ type RedirectConfig =
| {
status: ValidRedirectStatus;
destination: string;
priority?: RoutePriorityOverride;
};

export interface RouteData {
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/assets/endpoint/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export function injectImageEndpoint(settings: AstroSettings, mode: 'dev' | 'buil
pattern: '/_image',
entrypoint: endpointEntrypoint,
prerender: false,
priority: 'override',
});

return settings;
Expand Down
7 changes: 7 additions & 0 deletions packages/astro/src/core/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,15 @@ export const AstroConfigSchema = z.object({
z.literal(308),
]),
destination: z.string(),
priority: z.enum(['override', 'normal', 'defer'])
ematipico marked this conversation as resolved.
Show resolved Hide resolved
.default('defer'),
}),
])
.transform(redirect => (
typeof redirect === 'string'
? {destination: redirect, priority: 'defer' as const}
: redirect
))
)
.default(ASTRO_CONFIG_DEFAULTS.redirects),
prefetch: z
Expand Down