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 adding duplicated routes when users were using router.extendRoutes #1281

Draft
wants to merge 2 commits into
base: v7
Choose a base branch
from

Conversation

hacknug
Copy link

@hacknug hacknug commented Sep 10, 2021

This shouldn't be merged without review for obvious reasons. An explanation for the solution provided in this PR can be found in issue #1280.

The idea here is to check if the route already contains routesNameSeparator. In that case it probably means the user already anticipated the lack of support for this and maybe other features and we can (hopefully and) safely skip patching the route object with redundant data.

This was adding the same route a couple times so I added a conditional to check if a route with the same name already exists. If that's the case, we only push the new route when it has a children property because from what I saw, the module only localizes dynamic children routes in those cases.

Works fine on my local enviroment but I'm pretty sure this isn't taking into account all of the cases this module supports. Let me know if you want me to make any changes or add any extra details.

Closes #1280.

@stale
Copy link

stale bot commented Jan 8, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 8, 2022
@hacknug
Copy link
Author

hacknug commented Jan 11, 2022

not stale

@stale stale bot removed the stale label Jan 11, 2022
@rchl
Copy link
Collaborator

rchl commented Jan 11, 2022

This change is preventing prefixes from being added in some (all?) cases. See the unittesting results.

Also I'd like to see at least one new test added to test the case that you are fixing.

@rchl rchl marked this pull request as draft March 30, 2022 21:22
@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 12, 2022
@Nakroma
Copy link

Nakroma commented Jun 13, 2022

Not stale, we have the same issue. Any news on this PR or something others could help with?

@stale stale bot removed the stale label Jun 13, 2022
@hacknug
Copy link
Author

hacknug commented Jun 28, 2022

@Nakroma I'd say adding a failing test to demonstrate how this is broken in the current version of the module would be extremely helpful and get us one step closer to implementing a proper solution for it. Once we have that in place we can re-implement some of the changes in this PR in a way it doesn't affect any of the existing features of the module (this PR currently is making some of the tests fail). An explanation about your use case and how the current implementation is not working for you would also be helpful in case we need to consider a different scenario from the one I tried to explain in #1280.

I've had this in production since I opened the PR and it works fine for my use case. There's some issues with some routes on the sitemap but I haven't been able to put in the hours to research what's really going on and who's the culprit.

@Nakroma
Copy link

Nakroma commented Jun 29, 2022

When trying to replicate our issue for the failtest I took another look at our problem and it turns out it was an issue with nuxt itself that was causing the route duplication, i18n was just obfuscating it, so our problems seems to be a different one from the one mentioned in #1280

@stale
Copy link

stale bot commented Sep 21, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Sep 21, 2022
@hacknug
Copy link
Author

hacknug commented Sep 21, 2022

Not stale

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

Successfully merging this pull request may close these issues.

Not playing nice with router.extendRoutes
3 participants