-
-
Notifications
You must be signed in to change notification settings - Fork 108
fix(#688): Fixed non-deterministic sort order of typed-router.d.ts #733
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
Conversation
WalkthroughAdds deterministic, NFC-normalizing path sorting utilities ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Additional Thoughts1. Do sorting for routes also directly in
|
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @lupas. * #733 (comment) The following files were modified: * `src/codegen/generateRouteFileInfoMap.ts` * `src/core/sortDts.ts`
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/sortDts.ts(1 hunks)src/core/tree.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/tree.ts (1)
src/core/sortDts.ts (1)
comparePaths(30-59)
🔇 Additional comments (6)
src/core/tree.ts (1)
170-172: LGTM!Correctly delegates to the deterministic comparePaths function.
src/core/sortDts.ts (5)
1-15: LGTM!Clear documentation of purpose, rules, and normalization strategy.
17-23: LGTM!Collator configuration is appropriate for deterministic, locale-independent sorting with proper numeric handling.
25-27: LGTM!Proper normalization and segmentation logic.
61-62: LGTM!Clean helper that avoids mutation.
49-51: File detection logic OK
Routes directory contains only .vue files, so the current file-vs-folder check is safe. Revisit if other route file types are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/core/pathSorting.spec.ts(1 hunks)src/core/pathSorting.ts(1 hunks)src/core/tree.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/tree.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/pathSorting.spec.ts (1)
src/core/pathSorting.ts (2)
comparePaths(30-59)sortPaths(62-62)
🔇 Additional comments (3)
src/core/pathSorting.ts (2)
17-23: LGTM!The fixed English collator with numeric and case-first options ensures deterministic, locale-independent sorting.
61-62: LGTM!Correctly creates a shallow copy before sorting to avoid mutating the input array.
src/core/pathSorting.spec.ts (1)
1-160: LGTM!Comprehensive test coverage validates all sorting rules: hierarchical ordering, index.vue precedence, file-before-folder logic, numeric sorting, dynamic segments, and Unicode normalization edge cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/codegen/generateRouteFileInfoMap.ts (1)
42-54: Consider explicitly sorting Map entries for robustness.While the Map insertion order may be deterministic if
getChildrenSorted()provides stable ordering (as noted in the AI summary), explicitly sorting entries by file key would guarantee determinism independent of tree traversal order.Apply this diff to sort Map entries by file key:
const code = Array.from(routesInfo.entries()) + .sort(([fileA], [fileB]) => comparePaths(fileA, fileB)) .map(([file, { routes, views }]) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/codegen/generateRouteFileInfoMap.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/codegen/generateRouteFileInfoMap.ts (2)
src/core/pathSorting.ts (1)
comparePaths(30-59)src/utils/index.ts (2)
formatMultilineUnion(51-55)stringToStringType(67-69)
🔇 Additional comments (1)
src/codegen/generateRouteFileInfoMap.ts (1)
4-4: Import looks good.The
comparePathsimport is necessary for deterministic sorting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/codegen/generateRouteFileInfoMap.ts (1)
43-51: Views are not sorted withcomparePaths.Views remain unsorted, undermining deterministic output. This is the same concern from the previous review.
Apply this diff to sort views:
.map(([file, { routes, views }]) => { const routesSorted = [...routes].sort(comparePaths) + const viewsSorted = [...new Set(views)].sort(comparePaths) return ` '${file}': { routes: ${formatMultilineUnion(routesSorted.map(stringToStringType), 6)} views: - ${formatMultilineUnion(views.map(stringToStringType), 6)} + ${formatMultilineUnion(viewsSorted.map(stringToStringType), 6)} }` })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/codegen/generateRouteFileInfoMap.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/codegen/generateRouteFileInfoMap.ts (2)
src/core/pathSorting.ts (1)
comparePaths(30-59)src/utils/index.ts (2)
formatMultilineUnion(51-55)stringToStringType(67-69)
🔇 Additional comments (2)
src/codegen/generateRouteFileInfoMap.ts (2)
4-4: LGTM!Import is correct for the deterministic sorting implementation.
43-48: LGTM!Routes are correctly sorted with
comparePathsfor deterministic output.
|
Thanks but this was actually already fixed at #698 |
|
Even better! <:o) Thanks! |
Closes: #688
This pull request introduces a deterministic and locale-independent path sorting mechanism for route/path ordering.
The main goal is to fix bug #688 that resulted in a randomly sorted d.ts. file after almost every dev server start, that needed to either be committed or discarded.
The MR goes a bit beyond, ensure consistent sorting of routes, files, and views, regardless of locale or Unicode normalization differences. This is achieved by implementing a new
comparePathsfunction and updating relevant parts of the codebase to use it. Comprehensive tests have also been added to verify the new sorting logic, including edge cases involving Unicode and dynamic segments.Path sorting improvements:
comparePathsfunction insrc/core/pathSorting.tsthat sorts paths deterministically using fixed English collation, numeric-aware ordering, and Unicode normalization (NFC). This ensuresindex.vueis always first, files precede folders, and dynamic segments are handled consistently.TreeNode.comparemethod insrc/core/tree.tsto use the newcomparePathsfunction for consistent node ordering.src/codegen/generateRouteFileInfoMap.tsto sort routes usingcomparePaths, ensuring stable output in generated code. [1] [2] [3]Testing and validation:
src/core/pathSorting.spec.tsto verify the new sorting logic, covering REST-like routes, files vs. folders,index.vueprioritization, dynamic segments, numeric ordering, hierarchical grouping, and Unicode normalization.