Skip to content
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

fix: keep optional params coming from a parent record #2031

Merged
merged 6 commits into from
Feb 13, 2024
Merged

Conversation

chuyuandu
Copy link
Contributor

resolve TODO: only keep optional params coming from a parent record

Copy link

netlify bot commented Nov 3, 2023

Deploy Preview for vue-router ready!

Name Link
🔨 Latest commit 8fa3574
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/6548c8868c7ec300088b7137
😎 Deploy Preview https://deploy-preview-2031--vue-router.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov-commenter
Copy link

Codecov Report

Merging #2031 (0d414e1) into main (b6d12ed) will increase coverage by 0.02%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##             main    #2031      +/-   ##
==========================================
+ Coverage   90.77%   90.79%   +0.02%     
==========================================
  Files          24       24              
  Lines        1116     1119       +3     
  Branches      348      349       +1     
==========================================
+ Hits         1013     1016       +3     
  Misses         63       63              
  Partials       40       40              
Files Coverage Δ
packages/router/src/matcher/index.ts 98.78% <100.00%> (+0.02%) ⬆️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

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.

Thanks!

  • It should also go for ancestors, not just direct parents parents have all ancestors
  • It should have tests about how params with matching names but coming from different routes are discarded

But I'm actually unsure if this is the desired behavior. Currently, matching params are silently passed around and people might find that useful. I wonder if making this behavior stricter will be better in the long term

// only keep optional params coming from a parent record
matcher.keys
.filter(k => !k.optional)
.map(k => k.name)
Copy link
Member

Choose a reason for hiding this comment

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

This could be done once at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. A direct parent's keys contains all optional params, includes ancestors
  2. I'll open a new PR to improve the code if the behavior it is necessary, absolutely i think this is better to keep the optional params from parent record.
{
    path: '/:type?/root',
    children: [
    {
        path: 'path_a',
        name: 'a',
    },
    {
        path: 'path_b',
        name: 'b',
    }
  ]
}

// current path:   '/optionType/root/path_a'

resolve({
    name: 'b'
});

I think /optionType/root/path_b is better than /root/path_b in this case

Copy link
Member

Choose a reason for hiding this comment

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

I guess o forgot how I wrote my own code 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest to keep optional params coming from a parent record, that is really useful

@chuyuandu chuyuandu requested a review from posva November 6, 2023 11:07
@chuyuandu
Copy link
Contributor Author

@posva hey, i have improved the code and add unit tests

Copy link
Member

posva commented Nov 7, 2023

thanks! I will check later on this year

@posva posva changed the title keep optional params coming from a parent record fix: keep optional params coming from a parent record Feb 13, 2024
@posva posva merged commit 04b50e5 into vuejs:main Feb 13, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants