vuejs / router

🚦 The official router for Vue.js
https://router.vuejs.org/
MIT License
3.75k stars 1.15k forks source link

fix: avoid normalizing the fullPath #2189

Closed posva closed 2 months ago

posva commented 3 months ago

Close vuejs/router#2187

netlify[bot] commented 3 months ago

Deploy Preview for vue-router canceled.

Name Link
Latest commit 87b4e491aea5b26c893596a02963859b0ef2f225
Latest deploy log https://app.netlify.com/sites/vue-router/deploys/661f8583cc8e6e0008c781af
codecov-commenter commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 91.01%. Comparing base (31c5936) to head (e4805e6).

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

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2189 +/- ## ======================================= Coverage 91.01% 91.01% ======================================= Files 24 24 Lines 1135 1135 Branches 351 351 ======================================= Hits 1033 1033 Misses 63 63 Partials 39 39 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dargmuesli commented 2 months ago

Hmm, I suspect this change to break some routing in an i18n'd nuxt app of mine: dargmuesli/creal/pull/451. I get no error in the application though, so I'm not sure if another library needs to adapt. Just want to point this out as meta info, let's see if someone creates a more specific issue faster than me.

Explanation on the failling pipeline: a playwright test fails which checks for a router navigation to happen when a different language is selected in the website's footer. So if you want to test it yourself, you can checkout that branch and try to change the language in the footer. WIth vue-router v4.3.1 nothing happens, v4.3.0 works.

posva commented 2 months ago

Thanks for the info, if you create a boiled down repro without external deps that would be really helpful

BobbieGoede commented 2 months ago

@posva I can confirm this (or a different change in 4.3.1) breaks route resolution in the Nuxt i18n module.

Here's a reproduction using the Nuxt i18n starter, it shows the routes resolved for each language working with 4.3.0, changing the version in the package.json overrides to 4.3.1 makes these stop working.

Let me know if I can provide more information!

Related https://github.com/nuxt-modules/i18n/issues/2920

posva commented 2 months ago

Yes, a reproduction without i18n and Nuxt would help. The repro with i18n still looks like a black box without any errors so I can't know what is happening

BobbieGoede commented 2 months ago

I understand this isn't particularly helpful for debugging but I figured this would at least demonstrate this release contains breaking changes.

Hopefully I have time to dig into why this is breaking in our case and provide a more minimal reproduction, in the meantime Nuxt i18n users will have to pin vue-router to 4.3.0.

DavyJones21 commented 2 months ago

Hi.

Here's an example: From the api server comes the path, like this: /custom/path (It's a strange logic, but we have what we have).

I pass path in the custom router-link component (I use my own logic without vue-i18n):

<AppRouterLink :to="/custom/path" />

Inside AppRouterLink component:

const localizedTo = computed(() => {
    let { to } = props

    if (typeof props.to === "string") {
        to = router.resolve(props.to)
    }

    return {
        ...to,
        params: {
            ...to?.params,
            locale: locale.value,
        },
    }
})
<RouterLink :to="localizedTo">...</RouterLink>

Instead of /en/custom/path I get /custom/path and when I go to such a link the router does not work and the site crashes. With version 4.3.0 everything works fine.

BobbieGoede commented 2 months ago

I figured out what was causing this to break in the i18n module and have a fix lined up (https://github.com/nuxt-modules/i18n/pull/2922) that should resolve the issue for us and will work with both vue-router 4.3.0 and 4.3.1.

In our case we were resolving localized routes by replacing a resolved route's name, since the resolved route still had the previous fullPath that would be prioritized instead as per this PR.

@DavyJones21 I'm curious to see your own logic for resolving localized routes, since you're experiencing the same issue I'm guessing your localized route resolution is either the same or very similar, you may be able to fix this issue in the same way (https://github.com/nuxt-modules/i18n/pull/2922/files#diff-35b10ac721121ca10ffabe8186f63f1e7abfef49564ec90c7ab23bf24d3d3870)

posva commented 2 months ago

I reverted this and released a new patch

DavyJones21 commented 2 months ago

@BobbieGoede Yes, removing the fullPath from the resolved router object does solve the problem. It is strange that subsequent params changes do not work as they should.

Alternatively, I can add the locale to the path so that the resolving is immediately successful.

Thanks for help.