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 vue-i18n-routing #2686

Merged
merged 15 commits into from
Jan 14, 2024

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

As mentioned here intlify/routing#79 (comment) we want to integrate vue-i18n-routing in this repo.

We probably want the tests from that repo as well, so I still need to work on that.

@kazupon
There is likely still quite some code that can be removed, and maybe the overall directory structure should be changed, let me know what you think.

I also removed the exclusions/splits that were added to support Vue 2, would this cause issues for Nuxt I18n Bridge/7.4? I removed it to lower the build size.

Also could you take a look at https://github.com/BobbieGoede/i18n/blob/refactor/integrate-routing-4/src/runtime/routing/utils.ts, I'm not sure if the checks are still relevant for Nuxt/Vue 3 as I don't fully understand the difference between I18n, VueI18n and Composer πŸ˜…

πŸ“ Checklist

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

@BobbieGoede BobbieGoede changed the title [WIP] refactor: integrate routing 4 [WIP] refactor: integrate routing Jan 10, 2024
@BobbieGoede BobbieGoede changed the title [WIP] refactor: integrate routing [WIP] refactor: integrate vue-i18n-routing Jan 10, 2024
@kazupon
Copy link
Collaborator

kazupon commented Jan 11, 2024

@BobbieGoede

I also removed the exclusions/splits that were added to support Vue 2, would this cause issues for Nuxt I18n Bridge/7.4? I removed it to lower the build size.

You don't have to worry about that. :)
Nuxt I18n Bridge / Nuxt I18n v7.4 can provide the composable used by Nuxt I18n v8 for the existing Nuxt I18n v7.3 code base. You no longer need to worry about vue-i18n-routing. /cc @wattanx

@kazupon
Copy link
Collaborator

kazupon commented Jan 11, 2024

Also could you take a look at https://github.com/BobbieGoede/i18n/blob/refactor/integrate-routing-4/src/runtime/routing/utils.ts, I'm not sure if the checks are still relevant for Nuxt/Vue 3 as I don't fully understand the difference between I18n, VueI18n and Composer πŸ˜…

I think you can remove them if they are not used in Nuxt I18n.

I18n is an instance created by createI18n, and APIs such as t can be used via the global property.
The Composer is the skeleton exported by useI18n and others that provide Vue composition API-based API.
VueI18n is a legacy API that provides an API compatible with vue-i18n v8. It's wrapped with Composer.

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! πŸ‘
LGTM!

src/vue-i18n-routing.d.ts Outdated Show resolved Hide resolved
Copy link

nuxt-studio bot commented Jan 11, 2024

βœ… Live Preview ready!

Name Edit Preview Latest Commit
i18n Edit on Studio β†—οΈŽ View Live Preview c51bb8e

@BobbieGoede
Copy link
Collaborator Author

Thanks for the feedback!

I think you can remove them if they are not used in Nuxt I18n.

Some of it is still being used but I have now removed the unused parts and cleaned/refactored the rest of the utils.

Great! πŸ‘
LGTM!

I think all that's left to do is to move/add the relevant tests from vue-i18n-routing, hopefully I have time to work on that in the next few days. Will let you know when that's done!

@BobbieGoede BobbieGoede marked this pull request as ready for review January 13, 2024 10:19
@BobbieGoede BobbieGoede changed the title [WIP] refactor: integrate vue-i18n-routing refactor: integrate vue-i18n-routing Jan 13, 2024
@BobbieGoede BobbieGoede marked this pull request as draft January 13, 2024 10:47
@BobbieGoede BobbieGoede force-pushed the refactor/integrate-routing-4 branch from 8963658 to 021465a Compare January 13, 2024 12:15
@BobbieGoede BobbieGoede marked this pull request as ready for review January 13, 2024 12:19
@BobbieGoede
Copy link
Collaborator Author

@kazupon
I have added most of the tests from vue-i18n-routing. Some of the features/tests don't quite work the same in @nuxtjs/i18n (https://github.com/nuxt-modules/i18n/pull/2686/files#diff-1e1fee9c39238ac92309cc638084d2fa0d3a898b6ea2f4f98de2d01188447306R105-R108 and https://github.com/nuxt-modules/i18n/pull/2686/files#diff-1e1fee9c39238ac92309cc638084d2fa0d3a898b6ea2f4f98de2d01188447306R120-R124) not sure how and whether those should be tested.

Some of the tests had to be adjusted due to #2581.

I haven't added the extends tests, related to extending vueI18n and router. Not entirely sure how those could be tested accurately within the module.

Moving the tests also revealed an existing bug as seen here https://github.com/nuxt-modules/i18n/pull/2686/files#diff-1e1fee9c39238ac92309cc638084d2fa0d3a898b6ea2f4f98de2d01188447306R46-R56. When using strategy no_prefix the routes are not being escaped, this case was not being tested in vue-i18n-routing. I will create a new issue to track this bug.

Copy link
Collaborator

kazupon commented Jan 14, 2024

@BobbieGoede
Thank you for your efforts!

I have added most of the tests from vue-i18n-routing. Some of the features/tests don't quite work the same in @nuxtjs/i18n (https://github.com/nuxt-modules/i18n/pull/2686/files#diff-1e1fee9c39238ac92309cc638084d2fa0d3a898b6ea2f4f98de2d01188447306R105-R108 and https://github.com/nuxt-modules/i18n/pull/2686/files#diff-1e1fee9c39238ac92309cc638084d2fa0d3a898b6ea2f4f98de2d01188447306R120-R124) not sure how and whether those should be tested.

This may be due to a slight difference in behavior between the vue-router used in vue-i18n-routing and the routing inside nuxt.
nuxt uses vue-router, but internally nuxt wraps it and tweaks routing a bit.
The difference may affect the test.

I haven't added the extends tests, related to extending vueI18n and router. Not entirely sure how those could be tested accurately within the module.

extendI18n does not need to be tested if it works with nuxt i18n specs.
(BTW, I was thinking of providing vue-i18n-routing as a routing library to Vue community that uses only vue + vue-router) πŸ˜…

@BobbieGoede
Copy link
Collaborator Author

extendI18n does not need to be tested if it works with nuxt i18n specs.

It seems to be working πŸ˜…

(BTW, I was thinking of providing vue-i18n-routing as a routing library to Vue community that uses only vue + vue-router) πŸ˜…

What do you mean exactly? If there's any changes planned for vue-i18n-routing I can help πŸ˜„

I have no more changes for this PR, it can be merged if everything looks good!

@kazupon
Copy link
Collaborator

kazupon commented Jan 14, 2024

What do you mean exactly? If there's any changes planned for vue-i18n-routing I can help πŸ˜„

With using vite-plugin-pages, I wanted to give like nuxt i18n routing by using vue-i18n-routing for vue community.
https://github.com/hannoeru/vite-plugin-pages

And I wanted to give i18n routing on SSR with using vike
https://github.com/vikejs

I wanted to give these to the vite community via vue-i18n-routing, but my time is limited. πŸ˜…

@kazupon kazupon merged commit 7a0a9bd into nuxt-modules:main Jan 14, 2024
7 checks passed
DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
* refactor: integrate routing compatibles

* fix: routing integration

* refactor: integrate and replace `vue-i18n-routing`

* refactor: remove vue instance proxy

* refactor: cleanup imports and duplicate code

* refactor: restructure composables and remove unused global registry

* refactor: remove unused type declaration file

* refactor: remove links to `vue-i18n-routing` repo types

* refactor: routing utils

* refactor: cleanup routing utils and extends

* test: add routing compatibles tests

* test: add routing tests

* test: fix routing tests

* test: add `switchLocalePath` test

* test: add test clarification
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.

2 participants