-
-
Notifications
You must be signed in to change notification settings - Fork 488
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
feat!: add useSetI18nParams
composable
#2580
Conversation
f430708
to
f488e82
Compare
f488e82
to
2e35034
Compare
✅ Live Preview ready!
|
useSetI18nParams
composableuseSetI18nParams
composable
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 |
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
useSetI18nParams
which returns a functionsetI18nParams
which can be used to set the translated parameters for the current route.The composable function logic is basically a reimplementation of
useLocaleHead
fromvue-i18n-routing
except 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 usesuseState
to 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 usingdefinePageMeta
is that it's fixed at build time.Workarounds described #1736 bypass this by setting
route.meta.nuxtI18n
directly, 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
definePageMeta
as 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
, asunhead
can 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'sunhead
instance.📝 Checklist