Skip to content

refactor(routing): extract shared compareRoutes comparator to utils#322

Merged
james-elicx merged 1 commit intomainfrom
refactor/shared-route-sort-comparator
Mar 7, 2026
Merged

refactor(routing): extract shared compareRoutes comparator to utils#322
james-elicx merged 1 commit intomainfrom
refactor/shared-route-sort-comparator

Conversation

@james-elicx
Copy link
Copy Markdown
Collaborator

Summary

  • Extracts the duplicated sort comparator into a new compareRoutes<T extends { pattern: string }>() function in routing/utils.ts, which already exports routePrecedence
  • Replaces three identical inline comparators: scanPageRoutes and scanApiRoutes in pages-router.ts, and appRouter in app-router.ts
  • No behaviour change — only deduplication

Testing

pnpm test tests/routing.test.ts tests/route-sorting.test.ts — 75 tests pass.

The sort comparator (routePrecedence diff + localeCompare tiebreaker)
was duplicated in three places — scanPageRoutes, scanApiRoutes, and
appRouter. Extract it as compareRoutes() in routing/utils.ts, which
already exports routePrecedence, and replace all three call sites.
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new Bot commented Mar 7, 2026

Open in StackBlitz

npm i https://pkg.pr.new/vinext@322

commit: 6eefb7b

@james-elicx
Copy link
Copy Markdown
Collaborator Author

/bigbonk review

@james-elicx james-elicx marked this pull request as ready for review March 7, 2026 15:49
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 7, 2026

Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original

@ask-bonk
Copy link
Copy Markdown
Contributor

ask-bonk Bot commented Mar 7, 2026

Good — routePrecedence is only referenced in its own definition, its use inside compareRoutes, and a comment in an E2E test. No remaining direct imports from the routers.


This is a clean, straightforward deduplication. Here's the review:

LGTM. The change is correct and well-scoped.

What was done:

  • Three identical inline comparators (a, b) => { const diff = routePrecedence(a.pattern) - routePrecedence(b.pattern); return diff !== 0 ? diff : a.pattern.localeCompare(b.pattern); } extracted into a single compareRoutes function in routing/utils.ts
  • The generic constraint <T extends { pattern: string }> is the right choice — it works with both AppRoute and Route (and any future route type) without coupling utils.ts to those interfaces
  • The routePrecedence import in app-router.ts and pages-router.ts is correctly replaced with compareRoutes
  • routePrecedence remains exported for direct use (e.g., the E2E test references it in a comment, and it's useful as a standalone scoring function)

No behavioral change, no edge cases to worry about. The 75 passing tests confirm it.

github run

@james-elicx james-elicx enabled auto-merge (squash) March 7, 2026 15:51
@james-elicx james-elicx merged commit ce1d988 into main Mar 7, 2026
23 checks passed
@james-elicx james-elicx deleted the refactor/shared-route-sort-comparator branch March 7, 2026 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant