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

refactor: integrate localizeRoutes from vue-i18n-routing #2625

Merged
merged 3 commits into from
Dec 21, 2023

Conversation

BobbieGoede
Copy link
Collaborator

🔗 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

This PR moves adds the code and tests for localizeRoutes from vue-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

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

@BobbieGoede BobbieGoede requested a review from kazupon December 16, 2023 21:55
@BobbieGoede BobbieGoede self-assigned this Dec 16, 2023
@kazupon
Copy link
Collaborator

kazupon commented Dec 17, 2023

do I understand correctly from your comment that we want to move all of vue-i18n-routing inside of this repository?

Yes! we should move all to this repository.
vue-i18n-routing has somes code both version supporting for Vue 2 and Vue 3 with vue-demi.
we don’t need to move the codes for Vue 2.

Would we want to add it as a package inside this repository or fully integrate it in the module code?

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.

@BobbieGoede BobbieGoede force-pushed the refactor/port-localize-routes branch from 7c5df33 to 8572ba6 Compare December 19, 2023 23:53
@BobbieGoede
Copy link
Collaborator Author

BobbieGoede commented Dec 20, 2023

I have refactored and simplified the route localization further, now it mostly doesn't rely on imports from vue-i18n-routing (probably does indirectly).

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.

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.

@kazupon
Copy link
Collaborator

kazupon commented Dec 21, 2023

@BobbieGoede

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.

Good job!
Okay, so, Are you still going to make any changes?
If not, I would like to merge them! :)

@BobbieGoede
Copy link
Collaborator Author

@kazupon
Ah I should have switched it to draft when I was still editing 😅, this PR is all done and can be merged

@kazupon
Copy link
Collaborator

kazupon commented Dec 21, 2023

@BobbieGoede
okay,
when you will finish your work, please ping me.

@BobbieGoede
Copy link
Collaborator Author

@BobbieGoede okay, when you will finish your work, please ping me.

It is finished!

Copy link
Collaborator

@kazupon kazupon left a 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!

@kazupon kazupon merged commit ac4496d into nuxt-modules:main Dec 21, 2023
8 checks passed
DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
…dules#2625)

* refactor: integrate `localizeRoutes` from `vue-i18n-routing`

* test: integrate `localizeRoutes` tests

* refactor: integrate localizeRoutes from vue-i18n-routing (nuxt-modules#28)
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.

2 participants