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

refactor!: read vue-i18n options from config path #1948

Closed
wants to merge 6 commits into from

Conversation

ineshbose
Copy link
Collaborator

@ineshbose ineshbose commented Mar 22, 2023

Note: Possibly being chased/continued in #1973

❓ 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

Options would need to be passed from a separate file that will be imported by the plugin. This idea is inspired from looking at other modules that require runtime configuration. Please feel free to discuss over Discord about this change.

WORK IN PROGRESS

πŸ“ Checklist

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

@ineshbose
Copy link
Collaborator Author

@ kazupon, please feel free to share thoughts at your convenience. I expect the change to be slightly big in certain areas but I'll only finalise things after the previously proposed PRs have been tackled.

Btw, should the default config file be named i18n.config you think? (it follows *.config convention, is shorter than vue-i18n.{config,options} and it's also what I'm using in my personal Nuxt project) It would be customisable regardless - your call πŸ™‚

@ineshbose ineshbose self-assigned this Mar 22, 2023
Copy link
Collaborator

kazupon commented Mar 22, 2023

should the default config file be named i18n.config you think?

Yeah! your idea sounds good.

vueI18n?: I18nOptions | string
vueI18n?: {
configFile?: string
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I hope the i18n options keep I18nOptions | string because it would break compatibility with v7.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would need to be string for this change. Sorry if I'm misunderstanding but why is compatibility with v7 important (since this is another major version)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed, v8 is a major version, so breaking changes are inevitable.
Upgrading to a major version for a large application that has already been built is a huge effort.

Some frameworks (e.g. larave, rails, and react) have been released so that major versions do not cause too much work for the user's application. They are releasing features supported in previous versions in working order, just as Vue3 supports the Options API.

I have once again thought about how much breaking change would detract from the migration experience for the user.

In this PR, it is a change in the configuration interface, so the affected code point is local.
Changing the vueI18n option would not be much of a problem.
Of course, we need to write a migration guide in the documentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely - it would be great to have as less breaking changes as possible.

Similar to #1919 also moving some fragile configuration options to other solutions that Nuxt provides for stability, I'm aiming to do the same here. This may have been a small weak-point in v7, so having this solved in v8 would be great. The migration would indeed be documented.

@ineshbose
Copy link
Collaborator Author

PR would also possibly tackle this-

i18n/TODO.md

Line 9 in 7943ca1

- [ ] resolve i18n options of nuxt.config with `useRuntimeConfig`, not `i18n.options.mjs`

@kazupon
Copy link
Collaborator

kazupon commented Mar 23, 2023

Could you prioritize this PR over other tasks? πŸ™
I hope to release an RC version soon, and I want this to be the last opportunity involving breaking changes before that release.

@ineshbose ineshbose added the ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf label Mar 23, 2023
}
}
})
```

The `vueI18n` option is the same as `createI18n` function option of Vue I18n. `vueI18n` option is passed to the `createI18n` function via the nuxt plugin of this module internally.
The `defineI18nConfig` function is the same as `createI18n` function option of Vue I18n. The configuration is passed to the `createI18n` function via the nuxt plugin (runtime) of this module internally.
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 we should name defineIVue18nConfig, not defineI18nConfig, because that is coniguration forvueI18n option. And some people might misunderstand it as a nuxt i18n setting.

'@nuxtjs/i18n'
],
i18n: {
vueI18n: { configPath: './nuxt-i18n.js' } // custom path example
Copy link
Collaborator

@kazupon kazupon Mar 24, 2023

Choose a reason for hiding this comment

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

The vueI18n option may no longer be optional. It's recommended to autoload i18n.config.ts if we have a config that conforms to the convention. If we have a nonconventional config file name, we can specify it in the vueI18n option!

Copy link
Collaborator

@kazupon kazupon Mar 24, 2023

Choose a reason for hiding this comment

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

ah, sorry, Never mind this comment.
I was misreading. πŸ˜…

@kazupon
Copy link
Collaborator

kazupon commented Mar 24, 2023

In additions, we also need to fix the basic usage docs.
https://v8.i18n.nuxtjs.org/getting-started/basic-usage#translate-with-vue-i18n

@ineshbose
Copy link
Collaborator Author

Hi @kazupon

I wanted to formalise the context to this PR with complete transparency so that everyone is on the same page!

This module provides enabling vue-i18n with Nuxt 3 and SSR. On further deep-diving into the code of this repository and intlify packages, I realised that there is a lot of code-generation work happening (from JavaScript to JavaScript) which has a reputation of not being the most stable or reliable, so I'm looking for ways around it. Since I'm not an expert in i18n solutions, I cannot comment on the requirement for this. Nuxt 3 also involves a level of code generation through addTemplate, etc, and I have been using these functionalities for a while now to gain a better understanding; I've learnt that code generation should be minimum but also functions shouldn't be serialized (hence #1919 was also made). vue-i18n is excellent for internationalization and it provides modifiers that are passed as functions. From previous versions of this module, vueI18n is the configuration option for it, and then it gets serialized using code generation - but that has the limitations of manual code generation including understandability of code, efficiency and stability.

Based on Nuxt's official guide on a project's contexts - "they are not supposed to share state, code, or context" (also why nuxt.config.ts and app.config.ts are different and declared differently, why plugins and modules are different and declared differently, why app hooks and nuxt hooks are different and declared differently) - since vue-i18n is on the runtime context as a plugin, the configuration shouldn't be in nuxt.config.ts (build context) so that would greatly help with ensuring that developers are aware of the different contexts (as Nuxt aims to help with) and more importantly - code-generation since we don't have to take the build-time configuration to build/serialize it into runtime configuration as discouraged by the guide. Instead, since the configuration is now different, in a different file, it could just be imported in runtime. A great example of this is the FormKit module that only takes in configuration path to create the runtime plugin (docs) - so that is a point of inspiration.

With all that said, this is a massive change that will need to be tested intensively, and for me, as an amateur, to carry this out since I only have such little understanding of how the module and code generation is working, I will need some great help and great guidance. I can't say what steps to take forward, but I'm looking to hear from the lead developer (aka you πŸ˜„) if have some plans or feedback on how this can be facilitated or just cancelled (which I'm open to, but I believe it would continue with the code debt). Let me know - and we can continue a close discussion over Discord if required. πŸ™‚

Copy link
Collaborator

kazupon commented Mar 25, 2023

Hi! @ineshbose
Thank you for your detailed and clear explanation!!

I was understanding the current problems with the vueI18n option and that it does not follow the Nuxt development conventions πŸ˜…
vueI18n option is not code generated by gen.ts, but by using addPluginTemplate and import in that template.
It's better to use addPluginTemplate, as it will be safely handled by the Nuxt plugin.

The implementation using addPluginTemplate needs huge refactoring in runtime/i18n.ts and gen.ts.
I think that this refactoring would delay the release.

So, I have considered whether there is any compromise for the RC release.

Fortunately, vueI18n option, when a path is specified as a string, the code generated in i18n.options.mjs reads the file specified in the path with dynamic import.

The loaded file is processed by the bundler (vite), so that even if functions such as modifiers are set, they can be safely processed.

i18n.config can be solved with module.ts, so all you have to do is specify the path, and the impact cost is low.

If vueI18n option is set with Object, it will output a warning to set it with i18n.config, when nuxi dev.

In the RC release, we can accept configuration by Object, but in the v8 official release or RC patch version release, we will discontinue that support and vueI18n option will only accept path configuration.

That way, I believe we can make changes to vueI18n option progressive without forcing the community to wait for RC release.
We can refactor for vueI18n option using addPluginTemplate until v8 official release.

What do you think?

@ineshbose ineshbose added stale and removed ❗ p4-important Priority 4: bugs that violate documented behavior, or significantly impact perf labels Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants