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 VueI18n creation to own module #387

Merged
merged 1 commit into from
May 6, 2024

Conversation

chrismayer
Copy link
Collaborator

@chrismayer chrismayer commented May 2, 2024

This refactors the creation of the VueI18n instance by encapsulating it in an own ES module. So the i18n instance can be imported to other pure ES modules and will allow internationalization there.

This follows the approach given here: kazupon/vue-i18n#149 (comment)

This would make the introduction of a global $appLanguage variable in #356 obsolete by this much cleaner and more versatile approach. If you approve this PR, I'd happily revert #356 afterwards in a separate PR.

Accessing the current app language in pure ES modules could then be done by the following code:

const { i18n } = await import('../../../src/locales/wgu-i18n.js');
console.log(i18n.locale);

@chrismayer chrismayer marked this pull request as draft May 2, 2024 11:56
This refactors the creation of the VueI18n
instance by encapsulating in an own ES module.
So the i18n instance can be imported to other pure
ES modules and will allow internationalization.
@chrismayer chrismayer marked this pull request as ready for review May 2, 2024 12:07
Copy link
Collaborator

@sronveaux sronveaux left a comment

Choose a reason for hiding this comment

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

Works completely as expected. As said, this one renders #356 obsolete.

The only thing I wanted to comment on is that assignment of $appConfig on Vue.prototype directly inside unit tests is considered bad practice. We should create a local Vue instance and assign to this local instance in place of the global one.
However, as this is also done in other modules (for example LocaleSwitcher.spec.js), I approved as is. Perhaps an issue should be created with this point to be addressed and enhanced later...

@chrismayer
Copy link
Collaborator Author

Thanks for your review @sronveaux and thanks for your suggestion regarding the assignment to Vue.prototype in the tests. I created an issue to improve that as a whole ➡️ #388. Going to merge now...

@chrismayer chrismayer merged commit 35bf846 into wegue-oss:master May 6, 2024
1 check passed
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.

2 participants