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.

Sign up for GitHub

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

Jump to bottom

Changed behaviour for optional params #1893

Open
bodograumann opened this issue Jun 20, 2023 · 5 comments · May be fixed by #2083
Open

Changed behaviour for optional params #1893

bodograumann opened this issue Jun 20, 2023 · 5 comments · May be fixed by #2083
Assignees
@posva
Labels
bug Something isn't working has workaround A workaround has been found to deal with the issue

Comments

@bodograumann
Copy link

Reproduction

https://github.com/vuejs/router/blob/main/packages/router/__tests__/router.spec.ts#L300-L328

Steps to reproduce the bug

  • Route with an optional parameter
  • Set the parameter
  • Unset the parameter with undefined

Expected behavior

I would have expected the parameter to be unset as in versions before 4.2, but I know this might not have been the intended behaviour.

Actual behavior

This issue is about the changes in #1814

It is a bit of a mess, because several things come together.

  1. The documentation says undefined and null are stringified: 0c91292
  2. Before the change they where not stringified, but could actually be used to unset optional params in the current route.
  3. After the change they are actually removed instead of stringified.

Additional information

Whatever the original intented use was, the use-case of unsetting the value of an optional parameter was never properly documented.
In practice using undefined worked and was used by us and others, so the change is causing breakages in real-world usages.

I checked and unsetting an optional parameter with "" did work at least in 4.1.6, but I am not sure if it also did in earlier versions, when we first implemented this code.

Imho using the empty string as "not set" is bad practice and they should be normalized to undefined instead.
@bodograumann bodograumann mentioned this issue Jun 20, 2023
Merged
@posva posva closed this as completed in e6dfcde Jun 20, 2023
@posva Volta.net
Copy link
Member

posva commented Jun 20, 2023

I added a test but that seems to be working. If that's not what you meant a boiled down reproduction or a failing test would help. I updated the docs too

@bodograumann
Copy link
Author

Ok I saw it and am a bit confused how that works. Let me look into it.

Also wanted to note about the docs again, but seems you were faster 😆

@bodograumann
Copy link
Author

The issue occurs when you only want to update the params of the current route, without changing the name:

  it('removes null/undefined params when current location has it', async () => {
    const { router } = await newRouter()

    await router.push({ name: 'optional', params: { p: 'a' } })
    await router.push({ params: { p: null } })
    expect(router.currentRoute.value.params).toEqual({})

    await router.push({ name: 'optional', params: { p: 'a' } })
    await router.push({ params: { p: undefined } })
    expect(router.currentRoute.value.params).toEqual({})
  })

@posva posva self-assigned this Jun 20, 2023
@posva posva reopened this Jun 20, 2023
@posva posva added bug Something isn't working has workaround A workaround has been found to deal with the issue labels Jun 20, 2023 — with Volta.net
@posva Volta.net
Copy link
Member

posva commented Jun 20, 2023

Ah I see, the behavior should be consistent:

  • router.push({ name: 'optional', params: { p: null }})
  • router.push({ params: { p: null }})

No matter the current location.

The actual fix is more complicated than it seems because it requires some refactoring so I recommend you to use the documented approach for the moment of using a param with an empty string to remove it

@bodograumann
Copy link
Author

Ack, we are implementing the workaround right now.
Glad that this is getting fxed.

posva added a commit that referenced this issue Jun 20, 2023
@posva
fix: null should be preserved in relative navigations
67fee5f 
The fix is a bit more complicated that I anticipated, I will come back
to this later on as the currently documented version works perfectly.

- the nullish params are removed before being passed to the matcher
- The encodeParam function transform null into ''
- The applyToParams also works with arrays but it makes no sense to
  allow null in array params

Ideally, I would make the matcher a bit more permissive so the encoding
is kept at the router level. I think the matcher sholud be responsible
for removing the nullish parameters but that also means the encode
function should leave nullish values untouched. We might need an
intermediate Type for this shape of Params, it gets a little bit tedious
in terms of types, so I would like to avoid adding more types.

Close #1893
@posva posva linked a pull request Dec 15, 2023 that will close this issue
Draft
posva added a commit that referenced this issue Dec 15, 2023
@posva
fix: null should be preserved in relative navigations
c93e259 
The fix is a bit more complicated that I anticipated, I will come back
to this later on as the currently documented version works perfectly.

- the nullish params are removed before being passed to the matcher
- The encodeParam function transform null into ''
- The applyToParams also works with arrays but it makes no sense to
  allow null in array params

Ideally, I would make the matcher a bit more permissive so the encoding
is kept at the router level. I think the matcher sholud be responsible
for removing the nullish parameters but that also means the encode
function should leave nullish values untouched. We might need an
intermediate Type for this shape of Params, it gets a little bit tedious
in terms of types, so I would like to avoid adding more types.

Close #1893
@thejoecode thejoecode mentioned this issue Dec 18, 2023
Open
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@posva posva

Labels
bug Something isn't working has workaround A workaround has been found to deal with the issue
Projects
Vue Router Roadmap
Status: Planned
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: null should be preserved in relative navigations vuejs/router
2 participants
@posva @bodograumann