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: avoid normalizing the fullPath #2189

Merged
merged 2 commits into from Apr 17, 2024
Merged

fix: avoid normalizing the fullPath #2189

merged 2 commits into from Apr 17, 2024

Conversation

posva
Copy link
Member

@posva posva commented Mar 25, 2024

Close #2187

Copy link

netlify bot commented Mar 25, 2024

Deploy Preview for vue-router canceled.

Name Link
🔨 Latest commit 87b4e49
🔍 Latest deploy log https://app.netlify.com/sites/vue-router/deploys/661f8583cc8e6e0008c781af

@codecov-commenter
Copy link

codecov-commenter commented Mar 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

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

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

Additional details and impacted files
@@           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           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@posva posva merged commit c54fc84 into main Apr 17, 2024
6 checks passed
@posva posva deleted the fix/fullpath-encode branch April 17, 2024 08:20
@dargmuesli
Copy link

dargmuesli commented Apr 17, 2024

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.

Copy link
Member Author

posva commented Apr 17, 2024

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

@BobbieGoede
Copy link

@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 nuxt-modules/i18n#2920

@posva
Copy link
Member Author

posva commented Apr 18, 2024

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
Copy link

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
Copy link

DavyJones21 commented Apr 18, 2024

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
Copy link

I figured out what was causing this to break in the i18n module and have a fix lined up (nuxt-modules/i18n#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 added a commit that referenced this pull request Apr 18, 2024
Copy link
Member Author

posva commented Apr 18, 2024

I reverted this and released a new patch

@DavyJones21
Copy link

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

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.

hash decode not match the later encode
5 participants