feat: add dir property and defaultDirection option

[iifawzi]

i've made some changes to manage the case when the seo is enabled, which required changing the name of plugin.seo.js for clarity. Current scenario is: if the dir property is defined it will be used in the html's dir attribute, if not, or the locales form is an array of codes, the dir property will be set to undefined and the attribute will not be added.

discussion: #762

[iifawzi]

Hey @rchl, can you review this if you got a time?

[rchl]

Yes, sorry for the delay. Will do ASAP.

[rchl]

What I really don't like about this is that it uses a global mixin. It causes unnecessary work for both Vue and Vue-meta and is the reason why seo: true is not recommended in the documentation. Even though this logic is not as heavy as SEO logic, it still causes unnecessary processing for both Vue and vue-meta.

I would ask you to investigate https://vue-meta.nuxtjs.org/api/#meta-addapp instead. I was meaning to look into it for the SEO functionality but it would be nice to try it first on a smaller feature like this one. Judging from the documentation it should be pretty easy to register an app with it and then update it whenever the locale changes.

[iifawzi]

What I really don't like about this is that it uses a global mixin. It causes unnecessary work for both Vue and Vue-meta and is the reason why seo: true is not recommended in the documentation. Even though this logic is not as heavy as SEO logic, it still causes unnecessary processing for both Vue and vue-meta.

I would ask you to investigate https://vue-meta.nuxtjs.org/api/#meta-addapp instead. I was meaning to look into it for the SEO functionality but it would be nice to try it first on a smaller feature like this one. Judging from the documentation it should be pretty easy to register an app with it and then update it whenever the locale changes.

make sense. i just implemented it this way to follow the convention used with SEO functionality. I will take a look on the vue-meta later today.

[iifawzi]

as per docs, if we used vue-meta we will have to inject a function as same as the recommended solution for SEO, since $meta() can only be accessed through a component. is it the intended approach?

[rchl]

as per docs, if we used vue-meta we will have to inject a function as same as the recommended solution for SEO, since $meta() can only be accessed through a component. is it the intended approach?

In theory it can be accessed from the plugin's code also with something like:

  const metaApp = Vue.prototype.$meta().addApp('i18n')

but I'm not sure if this is OK conceptually or more like a hack.

@pimlie maybe you could chime in here? Is that an OK way to add a VueMeta App? If not, maybe there should be some official way to access meta $meta from a Nuxt plugin?

[iifawzi]

as per docs, if we used vue-meta we will have to inject a function as same as the recommended solution for SEO, since $meta() can only be accessed through a component. is it the intended approach?

In theory it can be accessed from the plugin's code also with something like:

  const metaApp = Vue.prototype.$meta().addApp('i18n')

but I'm not sure if this is OK conceptually or more like a hack.

@pimlie maybe you could chime in here? Is that an OK way to add a VueMeta App? If not, maybe there should be some official way to access meta $meta from a Nuxt plugin?

it will work, but we will not be able to re-set the meta when locale changes (from client side), or this at least what i noticed.

import Vue from 'vue'
export const setHeadMeta = function (context) {
  const {app} = context;
  const metaApp = Vue.prototype.$meta().addApp("i18n");
  const currentDir = app.i18n.localeProperties.dir || undefined;
  metaApp.set({
    htmlAttrs: {
      dir: currentDir,
    }
  })
}

and get called when locale changes:

    app.i18n.locale = newLocale
    app.i18n.localeProperties = klona(locales.find(l => l[LOCALE_CODE_KEY] === newLocale) || { code: newLocale })
    
    setHeadMeta(context)

it will work only in the initial render, after locale changes it won't, i read a bit in vue-meta docs/code, but couldn't find a way helps with our scenario.
did i miss anything?

[rchl]

It seemed to work for me when I've tested before (in a slightly different way). Did you see where it goes wrong exactly? Does it seem like a bug in vue-meta?

[iifawzi]

i haven't really reached any conclusions, but it seems like Vue.prototype.$meta() is not working when used after the initial render, i used some silly logs in vue-meta methods to figure what really happens, but nothing appears as if it's not even called.

[pimlie]

If not, maybe there should be some official way to access meta $meta from a Nuxt plugin?

@rchl $meta should also be available from the Nuxt Context which you have access to in Nuxt modules and plugins

[rchl]

If not, maybe there should be some official way to access meta $meta from a Nuxt plugin?

@rchl $meta should also be available from the Nuxt Context which you have access to in Nuxt modules and plugins

Tried different variations including:

context.$meta
undefined
context.app.$meta
undefined

and it doesn't seem to be available. It's also not visible in @nuxt/types.

[pimlie]

Hmm, yeah. So if you want to use addApp your best solution would probably be to get the component instance from the current page through vue-router matched components. You might have to wait for that instance to become available. But addApp is centered around Vue instances indeed and just doesnt work statically.

Alternatively you could try to overwrite app.head to inject/merge your own meta stuff, thats what nuxt-jsonld does: https://github.com/ymmooot/nuxt-jsonld/blob/master/src/mergeHead.ts

[rchl]

Alternatively you could try to overwrite app.head to inject/merge your own meta stuff, thats what nuxt-jsonld does: https://github.com/ymmooot/nuxt-jsonld/blob/master/src/mergeHead.ts

That one appears to be just adding a global mixin rather than working on app.head. Something I'd like to avoid.

[iifawzi]

so, what's your decision for this @rchl?

[rchl]

I'm still against a global mixin so the only idea I have left is exposing a function like $nuxtI18nHead() that the user would opt-in to use in the head() in the layout. That would be similar to the $nuxtI18nSeo() function that we already have.

In the future, in a breaking version, I would probably remove the global SEO mixin completely and require the user to use $nuxtI18nHead() function from within the layout which would also return SEO meta tags if seo is enabled. That's why I'd use the $nuxtI18nHead() name for it instead of the more specific $nuxtI18nDir(), for example.

[iifawzi]

this would be much better, i will commit the required changes tomorrow morning.
EDIT: what if someone wanna use $nuxtI18nSeo() and $nuxtI18nHead() in the same time? (before the breaking version).

also, i'm thinking about adding a defaultDirection option in the module options, maybe with default value auto or ltr to be used when the dir property is not specified for any locale, or in case if someone wants to use rtl for multi-lang or dialect and en for specific one, then defaultDirection would help not specifying the dir in each locale, do you agree?

[rchl]

EDIT: what if someone wanna use $nuxtI18nSeo() and $nuxtI18nHead() in the same time? (before the breaking version).

Simple destructuring like:

return {
  ...this.$nuxtI18nSeo(),
  ...this.$nuxtI18nHead(),
}

should work since there are no overlapping properties in those two.

also, i'm thinking about adding a defaultDirection option in the module options,

Sure

[iifawzi]

should work since there are no overlapping properties in those two.

Since we're adding the dir to htmlAtrrs there will be an overlapping. Maybe we can get over it by adding the dir to bodyAttrs (for now at least, until the breaking version).

[rchl]

should work since there are no overlapping properties in those two.

Since we're adding the dir to htmlAtrrs there will be an overlapping. Maybe we can get over it by adding the dir to bodyAttrs (for now at least, until the breaking version).

Why do you think there would be an overlap? The SEO functionality doesn't set anything in htmlAtrrs.

EDIT: No sorry, it does. Was searching for htmlAtrrs with a typo instead of htmlAttrs. Hmm...

[rchl]

New plan:

• intoduce $nuxtI18nHead()
• make it add the dir attribute and SEO attributes but only when seo option is disabled. When seo option is enabled, only add the dir attribute
• in the future, the $nuxtI18nSeo() will be deprecated/removed and the only way to add SEO attributes will be through $nuxtI18nHead() (with the global seo option controlling if it adds the SEO attributes).

EDIT: To be fair, it probably would make no difference if the SEO attributes would be added both through the $nuxtI18nHead() and global SEO mixin at the same time. So we could go a simpler route and not have that check at all.

[iifawzi]

This's the best plan. I was thinking about all possible edge cases, it seems like with this approach we will ONLY not be able then to disable/enable SEO on specific pages, i checked and it's also the current behavior when using $nuxtI18nSeo() , i think it goes under the main issue #984, right?, nothing else is missing i think (tests will prove :d), and all edge cases is covered, gonna work on it after the launch.

[rchl]

Yes, disabling SEO per-page is broken already - #932
Thanks for working on that.

[iifawzi]

Thank you too, couldn't be done without your guidance.

I committed the changes, and here's some points might need to be discussed:

• i added the addDirAttribute argument with default value true to be used when only the SEO meta is needed.

export const nuxtI18nHead = function (addDirAttribute = true) {
  if (addDirAttribute) {
  // add dir attribute
  }

if this needs to be changed to a different approach, we might allow the DefaultDirection option to accept false value.
and then the dir attribute wouldn't be added, but i think it will not be meaningful, for that i went with the argument approach.

• I didn't change the SEO page in docs, since i'm not sure how can we introduce the $nuxtI18nHead(). Should we add it in the more performant section instead of the current way? or keeping both until the next version? It might be much better, if you got a time to introduce it in docs.

• I tried to add a warning that $nuxtI18nSeo() will be deprecated/removed, but since the global mixin is getting applied in a lot of components, it seems to be annoying, therefore i removed it.

[rchl]

I've refactored a bit so that we could remove the seo-head.js file.
Feel free to review my commits.

[iifawzi]

Way much better, i've only fixed the links to head-meta.js
don't we need to add a note for the new way in SEO.md?

[rchl]

Added something to that effect but wouldn't be surprised if I've overlooked something.

[iifawzi]

Awesome, looks good.

[rchl]

Thanks for your great work.

[melnikovdv]

Guys, thanks for everything you're doing. It's really a great tool.

By the way, you have already updated docs, and people like me are looking for seo: false option or nuxtI18nHead function. And only a glimpse into the sources explained what's going on)

[rchl]

Released in https://github.com/nuxt-community/i18n-module/releases/tag/v6.19.0

(Also added version annotations in documentation to indicate from which version the new properties/options were added - 90a1959)