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: check if URL with replaced defaultLocale in notFoundRoutes #478

Conversation

sreetamdas
Copy link
Contributor

@sreetamdas sreetamdas commented Aug 29, 2022

Issue

Suppose there is a route /only-nl, that only exists in a particular locale, nl-NL. Currently, when generating the sitemap, we will see both the following loc in the sitemap:

/only-nl
/nl-NL/only-nl

This^ is wrong, since the route only exists in the nl-NL locale. Here is the expected output:

- /only-nl 
/nl-NL/only-nl

Why

We currently replace the defaultLocale (when i18n is enabled) in the urlSet (with the help of a regex), but URLs in notFoundRoutes are not replaced.

This would cause "notFound" routes on the defaultLocale (like /only-NL) to be missed when filtering.

What I've done

  • Add an additional check if url with defaultLocale replaced is included in notFoundRoutes
  • Update related fixtures and test

Please let me know if there's anything else you'd like me to add here, and feedback if it can be handled in a better way.

Thanks!

We currently replace the defaultLocale (when i18n is enabled) in the
urlSet (with the help of a regex), but URLs in notFoundRoutes are not
replaced.

This would cause notFound routes on the defaultLocale to be missed when
filtering.

Add additional check to check if url with edfaultLocale replacedd is
included in notFoundRoutes
@sreetamdas sreetamdas marked this pull request as ready for review August 29, 2022 10:07
Copy link
Owner

@iamvishnusankar iamvishnusankar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. Approved 🙏

@iamvishnusankar iamvishnusankar merged commit 3338f60 into iamvishnusankar:master Sep 10, 2022
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.

None yet

2 participants