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 special characters in route #2495

Closed
wants to merge 2 commits into from
Closed

fix: handle special characters in route #2495

wants to merge 2 commits into from

Conversation

Achneoder
Copy link

πŸ”— Linked issue

❓ Type of change

  • πŸ“– Documentation (updates to the documentation or readme)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Resolves #2382

Encodes the routePath via encodeURI for proper comaprison with route.from.fullPath

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@Achneoder
Copy link
Author

I'm probably missing something but the tests seem flaky. Executing my test locally works just fine, running the whole test suite results in failed tests, sometimes on of mine, sometimes others.

I merged my four individual tests to one with the same result.
I also tried to shard the execution of the specs, which results in sometimes in timeouts or just failures of random tests:
See https://github.com/Achneoder/nuxt-i18n/actions/runs/6545610627/attempts/1 - Attempt 1 was successful, Attempt 2 (just a re-run) failed

@BobbieGoede
Copy link
Collaborator

Thanks for contributing!

You're right the tests are flaky, still trying to figure out the exact reason why... πŸ˜… So far I have had some success in reducing the flakiness by adding waitForURL after navigations, which seems to have done the trick here too!

As for the issue this PR is fixing, as you mention in the comment note, this is most likely an issue in vue-i18n-routing. I tried to look into fixing it there after seeing your solution, but can't quite find the root cause (it uses router.resolve which I believe should encode the route as well?) or proof that the issue happens there in the first place.

I think we could merge this PR so at least the issue is resolved in @nuxtjs/i18n, and open an issue on vue-i18n-routing to track if and when that gets solved, after which we can remove the fix you added as ideally this is fixed upstream. What do you think @kazupon?

@BobbieGoede BobbieGoede added the v8 label Oct 17, 2023
@Achneoder
Copy link
Author

Cool! Thanks for your help!

I also tried to track down the origin of the path encoding issue and ended in vue-router. I hope to have a pull request ready soon there

@BobbieGoede
Copy link
Collaborator

I also tried to track down the origin of the path encoding issue and ended in vue-router. I hope to have a pull request ready soon there

Hmm, I suggest opening an issue there if it doesn't exist yet. If this behaviour stems from vue-router I'm curious whether maybe it is intentionally left unencoded. πŸ€”

@Achneoder
Copy link
Author

Apparently vue-router is working as intended:
vuejs/router#2021

Maybe it's worth to check vue-i18n-routing again and apply the fix there, but then there's the same question: is vue-i18n-rouing responsible to provide properly encoded URLs when calling switchLocalePath (and also probably localePath)

Copy link
Collaborator

kazupon commented Oct 19, 2023

@BobbieGoede

I think we could merge this PR so at least the issue is resolved in @nuxtjs/i18n, and open an issue on vue-i18n-routing to track if and when that gets solved, after which we can remove the fix you added as ideally this is fixed upstream. What do you think @kazupon?

Yeah, this issue should be opened in vue-i18n-routing.
You can open in there.

Copy link
Collaborator

kazupon commented Oct 23, 2023

I've released vue-i18n-routing v1.1.2
https://github.com/intlify/routing/releases/tag/v1.1.2

You can bump nuxt i18n deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unjustified "Error: Avoided redundant navigation to current location"
3 participants