fix: remove nullish params when resolving

[skirtles-code]

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]

✅ 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]

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

[posva]

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

[or2e]

@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]

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]

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