-
Notifications
You must be signed in to change notification settings - Fork 229
Create tanstack-router-route-matching-tree-rewrite.md #534
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
👷 Deploy request for tanstack pending review.Visit the deploys page to approve it
|
|
Review by Claude You're right, let me give you more critical feedback. I was being too polite - there are definitely areas that need work. Major Issues1. Inconsistent and Confusing Performance ClaimsYour numbers are all over the place and don't add up:
This creates confusion. Pick ONE consistent narrative. Either lead with realistic numbers or clearly frame the 20,000× as a best-case scenario from the start. 2. Poor Code ExamplesSeveral code snippets are incomplete or misleading: // This is useless without showing what matches() does
if (matches(...)) {
stack.push({/*next frame*/})
}// This type definition is confusing and incomplete
type Segment = Uint16Array & {0: SegmentKind, 1: number, 2: number, ... }The 3. Algorithmic Complexity Explanation is WrongYour complexity analysis is misleading:
This is comparing apples to oranges. The old approach complexity should also factor in pathname segments. A more honest comparison would be:
Your 50% elimination example is overly optimistic and doesn't reflect real-world routing patterns. 4. Technical Errors and Unclear ExplanationsIn the bitmasking section: This sentence is grammatically broken and technically unclear. The LRU cache section says it performs "half as well as a regular Object" but doesn't explain why you'd choose it over other caching strategies. 5. Missing Context for Key Decisions
6. Weak Benchmarking MethodologyYou mention testing against "production applications" but provide no details about:
7. Poor Structure in Implementation DetailsThe "Fun Implementation Details" section reads like a random collection of optimizations rather than a coherent explanation. The order doesn't make logical sense - you should explain the core trie structure first, then the traversal algorithm, then micro-optimizations. 8. Misleading Conclusion
But then you spend the entire article talking about performance numbers. If correctness was the main goal, show examples of incorrect matches that were fixed, not just performance charts. Specific Technical IssuesBackwards Stack ProcessingYour explanation of why you iterate in reverse is convoluted. The real reason is simpler: you want to process higher priority routes first, and array.pop() is more efficient than array.shift(). Bitmasking LimitationYou casually mention the 32-segment limit but don't discuss the implications. What happens when someone exceeds this? Does it fail silently? Throw an error? This is a critical design limitation. TypeScript Claims
This is wrong. The intersection type Writing IssuesTone InconsistencyYou switch between casual ("Let's be honest") and technical throughout. Pick one and stick with it. Weak TransitionsMany sections just start abruptly without connecting to the previous content. The flow between "Algorithmic Complexity" and "Fun Implementation Details" is particularly jarring. Unnecessary Hedge Words"pretty big app", "insane numbers", "feels reasonable" - these undermine the technical authority of the post. What's Actually Missing
The post reads more like a performance bragging session than a thorough technical explanation of a significant architectural change. |
LeCarbonator
left a comment
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.
Reads well! I was able to follow along just fine, even though I've never heard of LRU caching before.
I mostly corrected grammar and rephrased some sections. Apply the changes you think are approriate.
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.
This is great! :D
Couples of notes:
- You could explain what is a trie (very quick tldr, not in depth), or at least point to a wikipedia article about it
- You could also add a bit on the exploration of other routers path mapping to see what is usually correct vs not
An alternative title could be : How we accidentally made route matching more performant by aiming for correctness
tannerlinsley
left a comment
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.
I hereby approve this blog post ✅
New article related to this PR: TanStack/router#5722