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: exclude unprefixed routes for strategy prefix #2538

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented Nov 6, 2023

πŸ”— 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

@kazupon
What is the intended behavior for projects with redirectOn: 'root' when accessing an unprefixed (non existant in this case after this change) route like /about, should it return a 404? Right now this still redirects to the default language route, relevant issue: #2524.

Let me know if there are specific cases are missing tests for this change!

πŸ“ Checklist

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

Copy link
Collaborator

kazupon commented Nov 7, 2023

What is the intended behavior for projects with redirectOn: 'root' when accessing an unprefixed (non existant in this case after this change) route like /about, should it return a 404? Right now this still redirects to the default language route

as your mention,redirects to the default language route. this befavior is from nuxt-i18n v7.
we should be fixed.

@divine
Copy link

divine commented Nov 8, 2023

Hello,

Does it handle correct redirection from /about to /fr/about with Accept Language or Cookies already set?

Thanks!

Copy link
Collaborator

kazupon commented Nov 8, 2023

Does it handle correct redirection from /about to /fr/about with Accept Language or Cookies already set?

Thanks feedback!

@BobbieGoede
From this feedback, we need a test with a pattern of cases for cookies and navigator.languages.

@BobbieGoede
Copy link
Collaborator Author

@divine

Does it handle correct redirection from /about to /fr/about with Accept Language or Cookies already set?

You need to set detectBrowserLanguage.redirectOn to 'all' for this to work, this should already work work without this PR. Relevant #2524 (comment).

@kazupon

From this feedback, we need a test with a pattern of cases for cookies and navigator.languages.

As described above this seems to already work with the correct configuration.

as your mention,redirects to the default language route. this befavior is from nuxt-i18n v7.

After giving it more thought, I suppose it makes sense to redirect to a localized catch all route rather than responding with a plain 404 response. Hmm πŸ€” not entirely sure what would be the best approach. I believe this PR should at least resolve #2131.

@divine
Copy link

divine commented Nov 8, 2023

You need to set detectBrowserLanguage.redirectOn to 'all' for this to work, this should already work work without this PR. Relevant #2524 (comment).

I'll check but just make sure this won't create a regression later.

After giving it more thought, I suppose it makes sense to redirect to a localized catch all route rather than responding with a plain 404 response.

Returning 404 doesn't make sense, it's like returning 404 error code on the root path / then redirect to /en/.

You might be surprised but the old version had this problem and a lot of pages were duplicated in search engines. The reason behind this is that i18n module for Nuxt didn't handle correctly SEO. So redirecting to the correct page according to the language header or fallback locale it's what should resolve the duplicate problem.

Only in 2020 SEO feature was integrated correctly, I'm not sure why my contribution silently lost but there are PRs with the reasoning and explanation behind it so I'd expect at least this should work like the previous version which logic was fixed.

Thanks!

@rudolfbyker
Copy link

rudolfbyker commented Nov 22, 2023

I believe this PR should at least resolve #2131.

I tested this, and the unprefixed routes are still available. In fact, I can't see any difference after applying the changes from this PR. EDIT: The unprefixed routes are gone, but for some reason I still get redirected from /foo to /en/foo instead of the expected 404 on /foo (since foo is not a language).

@BobbieGoede
Copy link
Collaborator Author

@rudolfbyker
You probably have a catch all route set up, I think removing that would result in 404 response on these routes. Maybe as a workaround you could detect redirects to the catchall page, and ensure a 404 response if this is from a nonexistent page?

@rudolfbyker
Copy link

I double checked, and I definitely do not have a catch-all route. See my updated example with explanation at #2524 (comment) . Thanks for your time.

@rudolfbyker
Copy link

This PR is good. It removes the unprefixed routes as intended. #2524 seems to be a separate issue.

@kazupon kazupon merged commit 6ad7664 into nuxt-modules:main Dec 4, 2023
7 checks passed
DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
* fix: exclude unprefixed routes for strategy `prefix`

* test: add redirect test for strategy `prefix`
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.

None yet

4 participants