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

Context app in types #30

Closed
orblazer opened this issue Apr 23, 2019 · 13 comments
Closed

Context app in types #30

orblazer opened this issue Apr 23, 2019 · 13 comments
Labels
transfered Issue transfered from nuxt main repo

Comments

@orblazer
Copy link

What problem does this feature solve?

The problem is, when we use context.app we could not use (in nuxt-i18n) the context.app.i18n, or context.app.$t.

This resolve the problem of types detection when use context.app.

What does the proposed changes look like?

We can probably use interface for that with name like AppContext.

This feature request is available on Nuxt community (#c9094)
@kevinmarrec
Copy link
Contributor

@orblazer Did you add vue-i18n in your types of tsconfig.json ?

@orblazer
Copy link
Author

orblazer commented May 3, 2019

@kevinmarrec Yes but this is beacause nuxt-i18n add i18 with app.i18n and the context reference is Vue instance.

@kevinmarrec
Copy link
Contributor

@orblazer Alright but in which case do you need app.i18n over app.$i18n ?
And app.$t should arleady work.

@orblazer
Copy link
Author

orblazer commented May 3, 2019

That is needed if you want use the i18n in middleware (like for get locale or some variables)

@kevinmarrec
Copy link
Contributor

Isn't context.app.$i18n.locale enough ? Can you provide your middleware or a part of it to see what you're trying to achieve ?

@orblazer
Copy link
Author

orblazer commented May 3, 2019

@kevinmarrec i don't have the code under my hand but this is doesn't work beacause the app is not fully initialized. try to log context.app and you can see only "i18n" and not "$i18n".

@rchl
Copy link
Collaborator

rchl commented May 5, 2019

I know a bit of nuxt-i18n code so can chime in. nuxt-i18n indeed exposes i18n property (and some others) on context.app. The $i18n (with $ prefix) property is added to Vue instance by the i18n plugin itself (so not by the nuxt-i18n module) and so that one is not exposed on context or context.app. So to summarise, it uses either i18n or $i18n depending on the context. Both point to the same object though.

It's IMO a mess and if it would be for me, I would create new major version with this stuff cleaned up -- probably by only exposing $i18n property using inject function.

But in any case, in current state of things at least, is it not fixable in nuxt-i18n module itself rather than here?

Some more info here nuxt-modules/i18n#282

@rchl
Copy link
Collaborator

rchl commented May 5, 2019

OK, I think I understand. context.app is defined as Vue so it's not possible to extend app without also extending Vue instance inside of components. And with messy way things are done in nuxt-i18n, those don't match.

So I guess defining app in nuxt as type that extends Vue would probably help with this issue.

@orblazer
Copy link
Author

orblazer commented May 6, 2019

Yes exactly @rchl

@kevinmarrec
Copy link
Contributor

@rchl @orblazer app from context already has Vue type : https://github.com/nuxt/nuxt.js/blob/dev/packages/vue-app/types/index.d.ts#L17

Are you talking about having a dedicated interface extending Vue so that this interface can be altered by modules like nuxt-i18n ?

I'm pretty lost with all this stuff, I'm not mastering how vue-i18n is built, there is indeed confusion between i18n & $i18n.

$i18n refers to vue-i18n plugin, meanwhile i18n refers to the Nuxt module configuration, if I'm right.

I'm not really sure if adding properties to app is good practices, maybe some clean up with major version could be helpful, as suggested @rchl .

But anyway, this thread should be IMO on dedicated nuxt-i18n module.

/cc @paulgv

@orblazer
Copy link
Author

orblazer commented May 6, 2019

@kevinmarrec this is not only for nuxt-i18n this is for all module whant add context app methods.
Only an interface extends of Vue is probably required.

@rchl
Copy link
Collaborator

rchl commented May 6, 2019

@kevinmarrec

Are you talking about having a dedicated interface extending Vue so that this interface can be altered by modules like nuxt-i18n ?

Yes. The pros are:

  • simple change
  • backwards compatible (hopefully)
  • makes it easy to fix types in current version of nuxt-i18n

I'm not really sure if adding properties to app is good practices

Nothing really wrong with it. Official documentation has many examples with extending app and inject function also does that automatically. https://nuxtjs.org/guide/plugins#inject-in-root-amp-context

$i18n refers to vue-i18n plugin, meanwhile i18n refers to the Nuxt module configuration, if I'm right.

Both refer to vue-i18n plugin instance (extended with nuxt-i18n properties) AFAIK but former is only exposed as a mixin and latter is only exposed on context.app. (Or something close to that ;))

But anyway, this thread should be IMO on dedicated nuxt-i18n module.

That's true if decision is not to do dedicated interface in Nuxt. Fixing nuxt-i18n mess and just exposing $i18n universally (using nuxt's inject function) has pro of having nicer API in nuxt-i18n but:

  • is not backwards compatible
  • is quite a bit of work to clean up everything
  • not sure if @paulgv wants to take that leap

@rchl
Copy link
Collaborator

rchl commented May 6, 2019

I did some investigation on what context.app really is, and came up with conclusion that it's not really a Vue instance. Created #26 for that.

@pi0 pi0 transferred this issue from nuxt/nuxt Aug 10, 2019
@kevinmarrec kevinmarrec added the transfered Issue transfered from nuxt main repo label Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
transfered Issue transfered from nuxt main repo
Projects
None yet
Development

No branches or pull requests

3 participants