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

fix: v-t directive SSR #2946

Closed

Conversation

BobbieGoede
Copy link
Collaborator

@BobbieGoede BobbieGoede commented May 10, 2024

🔗 Linked issue

❓ Type of change

  • 📖 Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 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)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

📚 Description

Resolves kazupon/kazupon#17

I thought adding the directive transformation would have been more involved than this but it seems to be working 😅

I'll see if I can update https://github.com/intlify/vue-i18n-extensions a bit and perhaps experiment with making something like intlify/vue-i18n#1820 possible, though to be honest, I'm not sure if that feature is the right direction but it would be good learning experience.

📝 Checklist

  • I have linked an issue or discussion.
  • I have added tests (if possible).
  • I have updated the documentation accordingly.

@BobbieGoede BobbieGoede self-assigned this May 10, 2024
@BobbieGoede BobbieGoede requested a review from kazupon May 10, 2024 13:12
@BobbieGoede BobbieGoede marked this pull request as ready for review May 10, 2024 13:12
src/module.ts Outdated
*/

// Use `legacy` as global injections are prefixed (`$t`)
const transformVT = transformVTDirective({ mode: 'legacy' })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think mode option should be set from i18n.types

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm misunderstanding, but the injected component properties are always prefixed with $ (such as $t()) right?

When passing mode: 'composition' to the transform it will (in this case incorrectly) transform the code to use t() without prefix, which I think won't work 🤔.

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.

Thank you so much!
I've just comment to your PR!
Please check it!

@BobbieGoede BobbieGoede requested a review from kazupon May 30, 2024 11:45
@kazupon
Copy link
Collaborator

kazupon commented Jun 18, 2024

@BobbieGoede
Sorry to keep you waiting!

I've just released unplugin-vue-i18n v5 beta
I've merged vue-i18n-extensions into unplugin-vue-i18n.
https://github.com/intlify/bundle-tools/releases/tag/unplugin-vue-i18n%405.0.0-beta.2

If you set the optimizeTranslationDirective option to true, unplugin-vue-i18n will optimise v-t with Vue compiler and vite (rollup) transform.

@BobbieGoede BobbieGoede force-pushed the fix/t-directive-ssr branch from 0425f95 to 0347987 Compare June 23, 2024 08:29
@BobbieGoede
Copy link
Collaborator Author

I've just released unplugin-vue-i18n v5 beta I've merged vue-i18n-extensions into unplugin-vue-i18n. intlify/bundle-tools@unplugin-vue-i18n%405.0.0-beta.2 (release)

If you set the optimizeTranslationDirective option to true, unplugin-vue-i18n will optimise v-t with Vue compiler and vite (rollup) transform.

@kazupon
Also sorry for the delay too 😅 I just got back from a short holiday

I tried updating to the unplugin-vue-i18n beta but I'm getting the following error when running tests:

Error: requires 'vue-i18n' or 'petite-vue-i18n' to be present in the dependency tree.

Does updating this require updating other deps to incompatible versions? If so, we may have to merge the PR in its current state (unless this won't work for some configs) and delay the unplugin-vue-i18n update for the next major 🤔

@kazupon
Copy link
Collaborator

kazupon commented Jun 24, 2024

@BobbieGoede

Thanks!

Also sorry for the delay too 😅 I just got back from a short holiday

I tried updating to the unplugin-vue-i18n beta but I'm getting the following error when running tests:

Error: requires 'vue-i18n' or 'petite-vue-i18n' to be present in the dependency tree.

Does updating this require updating other deps to incompatible versions? If so, we may have to merge the PR in its current state (unless this won't work for some configs) and delay the unplugin-vue-i18n update for the next major

Hmm, That looks like a bug in unplugin-vue-i18n. 😅
I have an idea, and I'll try to fix it.

@kazupon
Copy link
Collaborator

kazupon commented Jun 25, 2024

@BobbieGoede
Copy link
Collaborator Author

@kazupon
I've updated to the latest beta, commented out the previous implementation and pushed the changes I made so far. It seems like something is not working, some tests fail and the tests seem to get frozen so I cancelled the workflow.

When running the fixture specs/fixtures/inline_options it will throw an error:

cannot support locale type

For any t or $t function call with a key that also has a translation (the fixture has no translations for blog and my-module-exemple.hello, these don't trigger the error).

Could you take a look again? 😅

@kazupon
Copy link
Collaborator

kazupon commented Jun 26, 2024

@BobbieGoede
Thanks!

When I tried the playground, I reproduced your issue on there.
I faced the errors:

at createCoreError (./node_modules/.pnpm/@[email protected]/node_modules/@intlify/core-base/dist/core-base.mjs:630:32)
at compileToFunction (./node_modules/.pnpm/@[email protected]/node_modules/@intlify/core-base/dist/core-base.mjs:952:11)
at compileMessageFormat (./node_modules/.pnpm/@[email protected]/node_modules/@intlify/core-base/dist/core-base.mjs:1171:15)
at translate (./node_modules/.pnpm/@[email protected]/node_modules/@intlify/core-base/dist/core-base.mjs:1039:45)
at ./node_modules/.pnpm/[email protected][email protected]/node_modules/vue-i18n/dist/vue-i18n.mjs:402:46
at wrapWithDeps (./node_modules/.pnpm/[email protected][email protected]/node_modules/vue-i18n/dist/vue-i18n.mjs:363:13)
at t (./node_modules/.pnpm/[email protected][email protected]/node_modules/vue-i18n/dist/vue-i18n.mjs:402:12)
at setup (./playground/pages/index.js:48:51)
at _sfc_main.setup (./playground/pages/index.js:260:23)

And I noticed that unplugin-vue-i18n depend on message-compiler of vue-i18n v10.
This error appears to occur because nuxt-i18n has been using its message compiler since vue-i18n v9, and the interface is not what is expected.

I’m just starting work for nuxt-i18n v9 and would like to support v-t SSR full support in that version.

@kazupon kazupon mentioned this pull request Jul 4, 2024
9 tasks
@kazupon kazupon closed this in #3014 Jul 4, 2024
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.

hello! kazupon!
2 participants