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

Update types #212

Merged
merged 6 commits into from
Feb 9, 2019
Merged

Update types #212

merged 6 commits into from
Feb 9, 2019

Conversation

iliyaZelenko
Copy link

This code throws a error:

Property 'locales' does not exist on type 'VueI18n & IVueI18n'. Did you mean 'locale'?

get availableLocales () {
  return this.$i18n.locales.filter(i => i.code !== this.$i18n.locale)
}

It helps me.

Maybe this is not the best solution, but it should fix the error.

I think it's worth adding this for the rest of the variables:

https://github.com/nuxt-community/nuxt-i18n/blob/90bcd80e7f0cbf4f00d15353c4eadea31f6ff892/src/plugins/main.js#L53-L59

@paulgv
Copy link
Collaborator

paulgv commented Feb 5, 2019

Thanks @iliyaZelenko !
@kevinmarrec could you have a quick look at this by any chance?

@kevinmarrec
Copy link

kevinmarrec commented Feb 5, 2019

Assuming nuxt-i18n code set app.i18n = new VueI18n(...) and then set things on app.i18n that is not defined in VueI18n itself, we indeed need to extend VueI18n interfaces.

I won't have time in the coming days to help providing these type definitions, but the types should be provided for the rest of variables aswell.

I'll be able to final review if someone take the time to provide the missing definitions.

@paulgv
Copy link
Collaborator

paulgv commented Feb 7, 2019

@iliyaZelenko do you see what's missing according to kevin's comment? Do you think you'd be able to complete the definitions before we merge?

@iliyaZelenko
Copy link
Author

@paulgv well, today I will make types for the remaining properties.

@kevinmarrec
Copy link

@iliyaZelenko Cool, for the locales property, I think it can be better than any type, would need to check source code to know what's the correct type (It's probably an array of object having code and other properties defined in documentation)

@iliyaZelenko
Copy link
Author

@kevinmarrec It looks like I made types for all options, but there wasn’t much point, as these types will not be used in the code of this package and it is unlikely that someone will use them in their project.

The main thing is that I have extended the IVueI18n interface so that the $i18n object has additional properties, I have separated options that fall into $i18n in a separate interface NuxtVueI18n.Options.VueI18nInterface.

I tested these types on my small project, there were no errors.

@kevinmarrec
Copy link

@iliyaZelenko Alright, great job !

I can't really take the time to check if types are all good, but it seems good and anyway it's not something that can break the project, it's "just" types.

So if it turns out that a type is not properly defined, we'll change it.

I trust you about having tested these types so let's stick with that !

@paulgv paulgv merged commit 5f8f4d7 into nuxt-modules:master Feb 9, 2019
@paulgv
Copy link
Collaborator

paulgv commented Feb 9, 2019

Many thanks to both of you!
We now have some errors in the tests: https://circleci.com/gh/nuxt-community/nuxt-i18n/355
I'm not sure what's missing, can you have a look @iliyaZelenko ?
I'll publish these changes as soon as the tests are fixed!

@iliyaZelenko
Copy link
Author

@paulgv this should fix the error: #216

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.

None yet

3 participants