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 6 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.
10 changes: 10 additions & 0 deletions packages/astro/src/@types/astro.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1555,10 +1555,19 @@ 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
* - "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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@delucis delucis Dec 15, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

I like that direction @sarah11918!

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Member Author

Choose a reason for hiding this comment

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

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

*/
export type RoutePriorityOverride = 'above-project' | 'same-as-project' | 'below-project';

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

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

export interface RouteData {
Expand Down
1 change: 1 addition & 0 deletions packages/astro/src/core/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Member

Choose a reason for hiding this comment

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

I believe we should at least provide a default.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Using a transform now!

}),
])
)
Expand Down