-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Poc router stack backtracking #1791
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1791 +/- ##
==========================================
- Coverage 89.74% 89.54% -0.21%
==========================================
Files 32 32
Lines 2672 2640 -32
==========================================
- Hits 2398 2364 -34
- Misses 175 178 +3
+ Partials 99 98 -1
Continue to review full report at Codecov.
|
8b07552
to
3093bab
Compare
@lammel could you review this one when you have time (poking with comment to trigger notification email) |
Sorry, forgot to respond. I'm busy this week. Tests are added and green, so we are not far from merging. |
router.go
Outdated
for { | ||
if search == "" { | ||
break | ||
// backtracking happens when we reach dead end when matching nodes in the router tree. To backtrack we set |
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.
Suggestion for comment. Not sure I got it right, take what makes sense to you:
// Backtracking is needed when a dead end (leaf node) is reached in the router tree.
// To backtrack the current node will be changed to the parent node and the next kind for the
// router logic will be returned based on fromKind or kind of the dead end node (static > param > any).
// For example if there is no static node match we should check parent next sibling by kind (param).
// Backtracking itself does not check if there is a next sibling, this is done by the router logic.
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.
changed
router.go
Outdated
cn = previous.parent | ||
valid = cn != nil | ||
|
||
// next node type by priority |
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.
Same here.
// Next node type by priority
// NOTE: With the current implementation we never backtrack from an `any` route, so `previous.kind` is
// always `static` or `any`
// If this is changed then for any route next kind would be `static` and this statement should be changed
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.
changed
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 think we should merge to have a clean base for further router improvements.
Approved!
Creating new PR for #1770 as @stffabi seems to be currently missing in action and I can not push to (edit) his PR ('was rejected by remote').
I would like to add PR after this is merged with: