Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: combining 'end' and 'strict' #2154

Merged
merged 2 commits into from
Oct 29, 2024
Merged

Conversation

skirtles-code
Copy link
Contributor

@skirtles-code skirtles-code commented Mar 1, 2024

This Playground illustrates the problem:

The bottom row is the one that isn't working correctly. This combination of options:

{
  path: '/user/',
  strict: true,
  end: false,
  component: ...
}

should match the location path /user/1, but it doesn't.

The reason why is the RegExp is checking for the / twice:

^/user/(?:/|$)

While I've added a lot of extra testing for end: false, only two of those assertions would have failed with the previous code. Specifically, this one on line 666:

matchParams('/home/', '/home/other', {}, options)

and this one on line 673:

matchParams('/home/:p/', '/home/a/b', { p: 'a' }, options)

In both cases they would previously have failed to resolve the route.

Copy link

netlify bot commented Mar 1, 2024

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit efc026e
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/65e99c58b9ed150008af83fe

@codecov-commenter
Copy link

codecov-commenter commented Mar 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.01%. Comparing base (13303bd) to head (efc026e).
Report is 119 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2154      +/-   ##
==========================================
+ Coverage   90.90%   91.01%   +0.11%     
==========================================
  Files          24       24              
  Lines        1121     1135      +14     
  Branches      347      351       +4     
==========================================
+ Hits         1019     1033      +14     
  Misses         63       63              
  Partials       39       39              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@posva posva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still trying to figure out this one. I have never been a fan of the end option. I may not have included enough tests on how this should work. I will have to check them again

@posva posva merged commit ab62098 into vuejs:main Oct 29, 2024
5 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants