-
-
Notifications
You must be signed in to change notification settings - Fork 518
feat!: add useSetI18nParams composable
#2580
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
feat!: add useSetI18nParams composable
#2580
Conversation
f430708 to
f488e82
Compare
f488e82 to
2e35034
Compare
✅ Live Preview ready!
|
useSetI18nParams composableuseSetI18nParams composable
kazupon
left a comment
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 work!
I've just reviewed your PR!
Please check it!
|
Thanks for the feedback! I agree that it's better to keep the docs and refer to the new composable, and add warnings when using the deprecated functionality. After more testing with this PR I ran into some SSR/hydration issues that I would like to fix as well. It will take some time to figure out the issues and to make the suggested changes so I'm marking this PR as WIP in the meantime! |
useSetI18nParams composableuseSetI18nParams composable
b83232d to
7d5971f
Compare
useSetI18nParams composableuseSetI18nParams composable
|
While trying to debug the hydration issues I found out I overcomplicated the solution a bit by using @kazupon |
kazupon
left a comment
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!
LGTM!
The logging test should be added if possible.
|
Okay add those soon! I think I will also change the arguments passed to |
|
@kazupon
Not sure if this is possible, as far as I can tell it is possible to set the log level, but changing it would affect all logs, not just for this module. I did change the composable parameter as I described. |
I think that we can do test at unit test that work unplugin directly. However, that is really difficult as it requires mock. I've pull the your branch in my local, and I've checked your branch with playground. We should release rc.9, because that PR might have still issues. |
|
Okay good! I'll merge this and release |
|
@BobbieGoede |
* feat: add `useSetI18nParams` composable * test: test `useI18nParams` composable * chore: add `useSetI18nParams` to playground * docs: remove and replace dynamic parameter usage with `useSetI18nParams` * fix: add `useSetI18nParams` to auto imports * test: fix dynamic parameters test * fix: prevent hydration mismatch by not using `useState` * fix: add deprecation warning for `nuxtI18n` usage * fix: revert docs and refer to composable * docs: improve `useSetI18nParams` documentation * test: improve `useSetI18nParams` test * chore: cleanup playground * fix: invert `useSetI18nParams` defaults * fix: playground products page not changing locale * fix: use `useLogger` for meta deprecation bundler plugin * fix: change `useSetI18nParams` parameters
🔗 Linked issue
❓ Type of change
📚 Description
This PR adds a composable
useSetI18nParamswhich returns a functionsetI18nParamswhich can be used to set the translated parameters for the current route.The composable function logic is basically a reimplementation of
useLocaleHeadfromvue-i18n-routingexcept it uses Nuxt's built in unhead instance to manage and render the tags.This PR only works combined with intlify/routing#82, as this allows us to change where these dynamic translated parameters are sourced from.
In this implementation it usesuseStateto set and retrieve these parameters, which seems to work in my testing.There is likely a better solution for this, but I hope to find a way to resolve the linked issues as I think this is/would be blocking users from moving to v8. If this works, we can always improve the functionality or performance later.
Goal
I have removed references to using dynamic parameters with
definePageMeta, the issue with usingdefinePageMetais that it's fixed at build time.Workarounds described #1736 bypass this by setting
route.meta.nuxtI18ndirectly, this works except for updating the SEO tags during SSR, this PR works similar to the workarounds except SEO tags are updated and rendered on the server.The issue described below also applies to those workarounds.
Common issue
In the documentation I have added a warning about a common scenario as there is no reactivity during SSR. For example, setting dynamic parameters from within a page will not be reflected in a layout as it would have already been rendered (see this comment).
This will cause a hydration mismatch if a language switcher is present in a layout or any page or component that renders before setting dynamic parameters.This has been improved to match the workaround, hydration mismatch is avoided as links will be retroactively updated on the client side, this means not all links are rendered correctly on the server-side while the head tags are.Remarks
definePageMetaas that would make this a breaking PR.should this be fully removed and/or trigger a deprecation warning when still in use?I have added deprecation warnings which log which pages are using the deprecated method.vue-i18n-routing, asunheadcan be used there as well. It's not my intention to move code out of that repository, but the current implementation makes use of Nuxt'sunheadinstance.📝 Checklist