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

feat!: add useSetI18nParams composable #2580

Merged

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented Nov 29, 2023

🔗 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 adds a composable useSetI18nParams which returns a function setI18nParams which can be used to set the translated parameters for the current route.

// `products/[slug].vue` (current page='/en/products/red-mug')
 
const setI18nParams = useSetI18nParams();
setI18nParams({
  en: {
    slug: 'red-mug'
  },
  nl: {
    slug: 'rode-mok'
  }
})

The composable function logic is basically a reimplementation of useLocaleHead from vue-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 uses useState 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 using definePageMeta 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

  • I haven't removed the logic of dynamic parameters using 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.
  • In the future I would like to improve and implement this in vue-i18n-routing, as unhead 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's unhead instance.

📝 Checklist

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

Copy link

nuxt-studio bot commented Nov 29, 2023

Live Preview ready!

Name Edit Preview Latest Commit
i18n Edit on Studio ↗︎ View Live Preview 33933c6

@BobbieGoede BobbieGoede self-assigned this Nov 29, 2023
@BobbieGoede BobbieGoede added ssr SEO Related to SEO features such as head tags labels Nov 29, 2023
@kazupon kazupon changed the title feat: add useSetI18nParams composable feat!: add useSetI18nParams composable Dec 4, 2023
@BobbieGoede BobbieGoede mentioned this pull request Dec 4, 2023
4 tasks
@BobbieGoede BobbieGoede marked this pull request as draft December 4, 2023 23:49
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 work!
I've just reviewed your PR!
Please check it!

docs/content/3.options/2.routing.md Outdated Show resolved Hide resolved
specs/fixtures/issues/2020/nuxt.config.ts Show resolved Hide resolved
@BobbieGoede
Copy link
Collaborator Author

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!

@BobbieGoede BobbieGoede changed the title feat!: add useSetI18nParams composable WIP: feat!: add useSetI18nParams composable Dec 6, 2023
@BobbieGoede BobbieGoede closed this Dec 7, 2023
@BobbieGoede BobbieGoede force-pushed the feature/use-set-i18n-params-composable branch from b83232d to 7d5971f Compare December 7, 2023 23:34
@BobbieGoede BobbieGoede reopened this Dec 7, 2023
@BobbieGoede BobbieGoede marked this pull request as ready for review December 8, 2023 09:00
@BobbieGoede BobbieGoede changed the title WIP: feat!: add useSetI18nParams composable feat!: add useSetI18nParams composable Dec 8, 2023
@BobbieGoede BobbieGoede marked this pull request as draft December 8, 2023 09:10
@BobbieGoede
Copy link
Collaborator Author

While trying to debug the hydration issues I found out I overcomplicated the solution a bit by using useState. This feature now internally uses the nuxtI18n approach, but improves its usage and ensures the meta tags are updated. I think there are a few ways to improve on the current way we handle head tags, but that will be something I can focus on aftwards.

@kazupon
I have added the deprecation warnings in the documentation and also added the warning logs when usage is detected, should I add tests to confirm these logs are working as expected? And should it be possible to disable this logs?

@BobbieGoede BobbieGoede marked this pull request as ready for review December 8, 2023 09:26
@BobbieGoede BobbieGoede requested a review from kazupon December 8, 2023 10:51
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!
LGTM!

@kazupon
Copy link
Collaborator

kazupon commented Dec 9, 2023

@BobbieGoede

should I add tests to confirm these logs are working as expected? And should it be possible to disable this logs?

The logging test should be added if possible.
As far as I know, warning can be disabled by specifying it in the useLogger options.
https://nuxt.com/docs/api/kit/logging

useLogger uses consola, I think it may be possible to disable it by setting an ENV or log level.
https://github.com/unjs/consola

@BobbieGoede
Copy link
Collaborator Author

@kazupon

The logging test should be added if possible.

Okay add those soon!

I think I will also change the arguments passed to useSetI18nParams, so that instead of { addDirAttribute?: boolean, addSeoAttribute?: boolean | SeoAttributesOptions } it will accept SeoAttributesOptions | undefined. Reasoning behind it is that this function shouldn't change the dir attribute (will remove that), and right now if addSeoAttribute isn't passed the function won't do anything.

@BobbieGoede
Copy link
Collaborator Author

@kazupon
Actually I don't know how to test those logs as they happen at build time 😅

useLogger uses consola, I think it may be possible to disable it by setting an ENV or log level.
unjs/consola

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.

@kazupon
Copy link
Collaborator

kazupon commented Dec 10, 2023

@BobbieGoede

Actually I don't know how to test those logs as they happen at build time 😅

I think that we can do test at unit test that work unplugin directly. However, that is really difficult as it requires mock.
we can test the dynamicRouteParams and route.meta.nuxtI18n warning logs at playground.

I've pull the your branch in my local, and I've checked your branch with playground.
I saw the warning logs.
I can merge your PR, so it looks good for me. :)

We should release rc.9, because that PR might have still issues.
If there is the issue, just fix it!

@BobbieGoede
Copy link
Collaborator Author

Okay good! I'll merge this and release rc.9 in a moment 💪

@kazupon
Copy link
Collaborator

kazupon commented Dec 10, 2023

@BobbieGoede
I'll merge your PR!

@kazupon kazupon merged commit 898d32a into nuxt-modules:main Dec 10, 2023
7 checks passed
DarthGigi pushed a commit to DarthGigi/i18n that referenced this pull request Apr 16, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
custom route path SEO Related to SEO features such as head tags ssr v8
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants