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: handle undefined path in router resolve #1904

Closed
wants to merge 1 commit into from
Closed

Conversation

kedrzu
Copy link
Contributor

@kedrzu kedrzu commented Jun 30, 2023

Suppose we execute for example:

router.replace({
    path: undefined,
    params: { foo: 'bar' }
);

Error would be thrown during route.resolve as we only check if location has property path but not if the path is defined and non null.

@netlify
Copy link

netlify bot commented Jun 30, 2023

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit 798a679
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/649e94c8c057840009eb7ba7

Copy link
Member

posva commented Jun 30, 2023

It’s preferred not to provide the path property altogether. What use case would this help with?

@kedrzu
Copy link
Contributor Author

kedrzu commented Jul 2, 2023

Yes, but it should not matter whether path is provided as undefined or not provided at all.
Especially, since in TypeScript path?: string property is satisfied by both passing undefined and not passing property value.

I found this bug when fixing the code recently migrated from Vue 2 to Vue 3. Thus, behavior in question is not backwards compatible.

Also there is no readable error thrown. If the intention is really to require passing non-null path property or not passing it at all, it should be clearly stated in some exception being thrown. However, I doubt that's a good option, since adding some smart-defaults is easy, better compliant with TS spec and compatible with previous version of vue-router.

Copy link
Member

posva commented Jul 2, 2023

Yes, TS does allow it but doesn’t allow to check for it with obj.property 🙃
I would like to change the types for that if possible. If not, we will have to go with the double check indeed
Still, what is the code that ends up passing path: undefined?

@kedrzu
Copy link
Contributor Author

kedrzu commented Jul 2, 2023

Piece of code looks like this:

const redirectPath = redirectFromCart ? localePathMain() : undefined;

if (queryChanged || redirectPath) {
  await router.replace({
    path: redirectPath,
    query,
  });
}

Basically, it redirects if path or query has been specified.
With requirement to not pass path at all I would have to replace it with multiple conditional clauses for all 3 cases:

  • query changed and path is not null
  • query changed and path is null
  • query did not change and path is not null

This would look quite cumbersome. What I did to fix this issue, is I wrote an util function to get rid of undefined object props and used it like this:

if (queryChanged || redirectPath) {
    await router.replace(
        removeUndefinedProps({
            path: redirectPath,
            query,
        })
    );
}

Still, looks like a hack..

@kedrzu
Copy link
Contributor Author

kedrzu commented Jul 2, 2023

And because most of the times in JS code undefined is not considered to be a value at all and there is usually no distinction between value being set to undefined or not being set at all.

I believe, that making this distinction for the sake of it does not make a lot of sense.

Also, please take a look at this piece of code, which does exactly the same, what I did by introducing this change:

if ('name' in location && location.name) {

name property is not checked only by its presence in object but also by its value.

@posva
Copy link
Member

posva commented Jul 2, 2023

Sorry, I couldn't push some changes to the PR, I don't know why as the checkbox seems to be checked so I created #1909. Feel free to make a review or even to create a new PR from there!

This could break some types for some people so I want to check a bit more before

@posva posva closed this Jul 2, 2023
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.

2 participants