router icon indicating copy to clipboard operation
router copied to clipboard

test: earlier routes should have matching priority

Open skirtles-code opened this issue 1 year ago • 3 comments

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:

routes: [
  {
    path: '/user/:id(1\\d*)',
    component: {}
  }, {
    path: '/user/:id(\\d+)',
    component: {}
  }
]

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.

skirtles-code avatar Feb 11 '24 14:02 skirtles-code

Deploy Preview for vue-router canceled.

Name Link
Latest commit 985e1f18ea5d4cc45c9672ac38b144731aff4358
Latest deploy log https://app.netlify.com/sites/vue-router/deploys/65c8d4fbf9bc860008043d92

netlify[bot] avatar Feb 11 '24 14:02 netlify[bot]

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2df32af) 90.85% compared to head (985e1f1) 90.88%. Report is 9 commits behind head on main.

Files Patch % Lines
packages/router/src/navigationGuards.ts 88.88% 0 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2138      +/-   ##
==========================================
+ Coverage   90.85%   90.88%   +0.03%     
==========================================
  Files          24       24              
  Lines        1115     1119       +4     
  Branches      347      347              
==========================================
+ Hits         1013     1017       +4     
  Misses         63       63              
  Partials       39       39              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Feb 11 '24 14:02 codecov-commenter

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

posva avatar Feb 13 '24 15:02 posva