-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
refactor: integrate localizeRoutes
from vue-i18n-routing
#2625
refactor: integrate localizeRoutes
from vue-i18n-routing
#2625
Conversation
Yes! we should move all to this repository.
I would prefer to move it as an integrated module code, if it is moved as a separate package in monorepo, it would be very cumbersome to manage and require npm publish of the package. Even if we were to manage it in the pnpm workspace as a simple priavte pacakge without using npm publish, it would still be cumbersome to manage because the code would have to be inlined and included in nuxt i18n. |
7c5df33
to
8572ba6
Compare
I have refactored and simplified the route localization further, now it mostly doesn't rely on imports from
Sounds good! It will definitely make it easier to change/fix some things (such as #2628), will slowly work to refactor and integrate the code over multiple PRs. Also I notice the tests introduced by #2580 are flaky, I'll have to look into that sometimes. |
Good job! |
@kazupon |
@BobbieGoede |
It is finished! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great works!
your are perfect! 💯
LGTM!
…dules#2625) * refactor: integrate `localizeRoutes` from `vue-i18n-routing` * test: integrate `localizeRoutes` tests * refactor: integrate localizeRoutes from vue-i18n-routing (nuxt-modules#28)
🔗 Linked issue
❓ Type of change
📚 Description
This PR moves adds the code and tests for
localizeRoutes
fromvue-i18n-routing
.@kazupon do I understand correctly from your comment that we want to move all of
vue-i18n-routing
inside of this repository? Would we want to add it as a package inside this repository or fully integrate it in the module code?Besides the above reasoning, the main motivation behind wanting to add this code is to work on fixing #2351. I figured I would add it first without making any changes. Will be refactoring and possibly fixing the issue in a separate PR.
📝 Checklist