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: remove nullish params when resolving #1814

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

skirtles-code
Copy link
Contributor

Introduction

This line appears in router.ts:

const targetParams = assign({}, rawLocation.params)

The lines that follow update the contents of targetParams to remove nullish values, but that object is never actually used anywhere.

Background

That code was introduced in change ebca15a. From what I can tell, targetParams was supposed to be used to build matcherLocation on the lines that follow, but instead it is using the original rawLocation.params.

The accompanying test also contains a problem. It is using toMatchObject() to verify that the params object is empty. Unfortunately, toMatchObject() only checks properties that are present in the expectation, it won't fail for any extra properties that are present. So even though it appears to be verifying that the params is empty, that isn't really what it's doing. The params object still contains the null and undefined property values.

Proposed Solution

I've assumed the intention is for the null and undefined values to be removed, so I've adjusted the code accordingly.

However, if the current behaviour is deemed correct then the alternative 'fix' would be to remove this section of the code, as it currently isn't doing anything.

@netlify
Copy link

netlify bot commented Apr 24, 2023

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit d3d7552
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/64461a4b32e143000924a882

@codecov-commenter
Copy link

Codecov Report

Merging #1814 (d3d7552) into main (4b08a12) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1814      +/-   ##
==========================================
- Coverage   90.86%   90.77%   -0.09%     
==========================================
  Files          24       24              
  Lines        1116     1117       +1     
  Branches      347      348       +1     
==========================================
  Hits         1014     1014              
  Misses         63       63              
- Partials       39       40       +1     
Impacted Files Coverage Δ
packages/router/src/RouterView.ts 93.42% <100.00%> (+0.08%) ⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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 a lot, it's indeed much better to write the tests without matchObject(). Also, thanks for the detailed search made beforehand 🔍 , it really helps and it's a joy to review a PR like this! 😍

While testing this a bit, it made me wonder if it shouldn't cast the params to empty strings instead of removing them. This would it make consistent with router.resolve({ name: 'optional', params: { p: '' }}). The other way around is not possible because a param can be intentionally empty (arguably not something people should use though). However, I feel like it's safer to keep the difference in this case. In practice, the user should test with !route.optionalParam to check if an optional param is absent. I will have to make sure this is mentioned in the changelog

@posva posva changed the title fix: remove nullish params fix: remove nullish params when resolving Apr 24, 2023
@posva posva merged commit 15e20cb into vuejs:main Apr 24, 2023
@or2e
Copy link

or2e commented May 17, 2023

@posva

Hi ')
I wouldn't classify this edit as a bugfix because the expected behavior has changed.
Now, to omit an optional parameter, instead of undefined you have to explicitly set the empty string ''

<4.2

 router.push({ params: { page: undefined }}); 

4.2

 router.push({ params: { page: '' }});

@posva
Copy link
Member

posva commented May 18, 2023

This is a bugfix. You can check the expected behavior in the test, among them, null and undefined remove the optional param. As mentioned in the comments above, check for empty/missing params with !route.params.optionalParam. Cheers.

@bodograumann
Copy link

This also broke navigation for us.
I created an issue to clarify at least the documentation, @or2e: #1893

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.

5 participants