i18n icon indicating copy to clipboard operation
i18n copied to clipboard

Fix adding duplicated routes when users were using `router.extendRoutes`

Open hacknug opened this issue 2 years ago • 9 comments

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.

hacknug avatar Sep 10 '21 14:09 hacknug

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[bot] avatar Jan 08 '22 22:01 stale[bot]

not stale

hacknug avatar Jan 11 '22 12:01 hacknug

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 avatar Jan 11 '22 15:01 rchl

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[bot] avatar Jun 12 '22 21:06 stale[bot]

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

Nakroma avatar Jun 13 '22 11:06 Nakroma

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

hacknug avatar Jun 28 '22 09:06 hacknug

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

Nakroma avatar Jun 29 '22 11:06 Nakroma

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[bot] avatar Sep 21 '22 03:09 stale[bot]

Not stale

hacknug avatar Sep 21 '22 08:09 hacknug