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

Fixes regression on routing priority for multi-layer index pages #10096

Merged
merged 9 commits into from
Feb 14, 2024

Conversation

Fryuni
Copy link
Member

@Fryuni Fryuni commented Feb 12, 2024

Changes

The sorting algorithm positions more specific routes before less specific routes, and considers index pages to be more specific than a dynamic route with a rest parameter inside of it.
This means that /blog is considered more specific than /blog/[...slug].

But this special case was being applied incorrectly to indexes, which could cause a problem in scenarios like the following:

  • /
  • /blog
  • /blog/[...slug]

The algorithm would make the following comparisons:

  • / is more specific than /blog (incorrect)
  • /blog/[...slug] is more specific than / (correct)
  • /blog is more specific than /blog/[...slug] (correct)

Although the incorrect first comparison is not a problem by itself, it could cause the algorithm to make the wrong decision.
Depending on the other routes in the project, the sorting could perform just the last two comparisons and by transitivity infer the inverse of the third (/blog/[...slug > / > /blog), which is incorrect.

Now the algorithm doesn't have a special case for index pages and instead does the comparison soleley for rest parameter segments and their immediate parents, which is consistent with the transitivity property.

Testing

  • Added the reproduction scenario from Issue in Astro 4.2.4+ with dynamic routes ordering #10071
  • Added an extra case in an existing test
  • Changed some of the tests to verify the expected relation invariants instead of the full order of the routes to make mistakes easier to spot and to understand in error reports
  • Changes the order of one test case that was inconsistent with our rules

Docs

No docs changes expected :)

Copy link

changeset-bot bot commented Feb 12, 2024

🦋 Changeset detected

Latest commit: bc61b1b

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Feb 12, 2024
@ematipico ematipico merged commit 227cd83 into withastro:main Feb 14, 2024
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Feb 14, 2024
@Fryuni Fryuni deleted the fix/route-priority-edge-case branch February 14, 2024 11:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue in Astro 4.2.4+ with dynamic routes ordering
3 participants