-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
test: earlier routes should have matching priority #2138
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 Preview for vue-router canceled.
|
Codecov Report
All modified and coverable lines are covered by tests ✅
Project coverage is 90.88%. Comparing base (
66a6f54) to head (985e1f1).
Report is 169 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@ ## main #2138 +/- ## ========================================== + Coverage 90.77% 90.88% +0.11% ========================================== Files 24 24 Lines 1116 1119 +3 Branches 348 347 -1 ========================================== + Hits 1013 1017 +4 Misses 63 63 + Partials 40 39 -1
☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.
posva
commented
Feb 13, 2024
I'm not clear whether this is documented anywhere, but I think it's a reasonable assumption and something that people are likely to be relying upon, even though they may not realise it.
It's reasonable, but I also think one should never have this kind of routes (conflicting routes) precisely because the behavior can be unpredictable. So, I intentionally never documented it. The current sorting order is indeed stable, but I would say one should not rely on this so I feel reluctant to add such test cases.
I think there are safer paths to not change the current matcher behavior. I should post these thoughts on the other PRs
My initial attempt at #2137 passed all the existing unit tests, but with hindsight it fails to take account of various important cases.
This PR attempts to add test cases for one of those problem cases. e.g. Consider the following code:
The path ranking gives these routes the same score. For a path of
/user/1234, both would be eligible to match, but we would want the first route to match. The ordering matters, the earlier route needs to be given priority.I'm not clear whether this is documented anywhere, but I think it's a reasonable assumption and something that people are likely to be relying upon, even though they may not realise it.
My attempt at using a binary search in #2137 to sort the routes broke this assumption, leading to routes with equal scores being in an essentially random order. I believe that PR is fixable, but these extra tests can be merged independently.