-
-
Notifications
You must be signed in to change notification settings - Fork 335
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
feat!: type-safe messages #1649
base: master
Are you sure you want to change the base?
feat!: type-safe messages #1649
Conversation
TODO: Allow for non-existent messages to go through. If done correctly, could prevent a breaking change. Also yes, I will move the type to /types/, this is not final yet as I'm very early into the development of this feature. |
@kazupon should this change VueMessageType or should this be added as an overload of t? Curious to hear your opinion on how to proceed with this. Both options have their advantages, and I would imagine backwards compatibility is preferred. I was also thinking of making a "lazy loaded" version of this that would essentially only display autocompletions for the keys directly under the current scope, which would basically emulate object property intellisense in VSCode while being in a string. Would love to hear your thoughts on this as well! |
Thank you for your contrivution! First we would to confirm, I don't know from this PR what is the issue. |
@kazupon sure thing! Also, just to clarify, it's in no way done, that's why it's still a draft and not even ready for review yet. There's still a lot for me to do to make it stable and then figure out how to generate tests for it. |
@GalacticHypernova
I am glad of your PR because I wanted to achieve fully type-safe message!
You can add type test to |
Of course!
I mean, this would require unit testing of type inference, rather than regular types. It partially relies directly on the typescript engine. Does vitest have something to deal with that?
Could you please tell me where the messages themselves are stored (as in, the messages object from which you're reading the translations)? I'm seeing multiple potential places, but thought I'd ask you. Is it the |
Yeah, vitest has type testing. vue-i18n has its own type check utils for type testing at
The messages themselves are stored in Compose's readonly messages, which is typed with the |
Thank you! |
This is an awesome feature. But may I ask, Is this useful for JSON files? |
@sagsagg I am planning to make it JSON compatible, yes. I'd have to see if json files get treated differently in terms of message parsing and adjust accordingly, but it is planned. |
@kazupon does this make sense? From the tests that are already present the functionality appears to stay the same, still getting all the right messages. Would appreciate a review from you so that in case something went wrong I won't need to rewrite everything. If I get the green light from you I could start changing all the translation functions. EDIT: Getting some weird behavior, investigating. It appears as though the tests fail with this one because it doesn't recognize the Locale when defining messages with locales in the composer (Locale is probably an empty string), leading to no autocomplete and defaulting to string. Either I'm missing something or I'm missing something. Does the locale not separate the messages in the composer? EDIT 2: Upon further inspection, I realized the TranslationComposer has a Locales property. When changing it to Messages[Locales] the autocomplete works but TS errors saying Locales can't be used to index Messages. Can we allow it to error or should Locale be fixed? Perhaps we can extend ComposerTranslation from Composer or ComposerOptions? Although I don't quite know how that would affect messages from files.. This still needs a bit of experimentation. Waiting to consult with you before proceeding on how we should integrate this. |
@GalacticHypernova
Thank you for your contribution. The value of If |
@kazupon |
@kazupon |
Is this pr still going on? Look forward to |
Yea, it is still in progress. Sorry, was quite busy lately with personal life. This will likely take a little longer as there seems to be a limitation with a strictly typed solution which would mean a refactor is in order, and testing that will take a little bit. I'm just testing it locally and haven't pushed it yet. |
Looking forward to this feature 😄 |
Thank you for your contribution! |
@kazupon Was thinking about this a little more. Obviously reading the messages from the files with a typed solution won't be possible, considering types can't access runtime objects (unless we can do somehow |
Closes #1124
Closes nuxt-modules/i18n#1753
This PR adds type safety for message translations when used with helpers like t().
The reason it's a breaking change is because it's more strict, which could bring some edge case bugs with non-existent messages.
It is a draft for now because there is much work I need to do to get it to work.
Benefits:
This allows for much safer and convenient locale message translation by providing autocomplete suggestions to make sure no typos are present. Essentially a very useful DX update.
Related: #1648
(Had to redo a certain part of the PR so decided to re-open it)
Proof-of-concept:
Smaller object:
Bigger object
Performance implications:
None. As with the proof of concept, we have tested (with @BobbieGoede) the performance on extremely large objects. (Tested with core.js package-lock.json file). The results were pretty much instant and no performance downgrade has been detected, view video in the attached folder below:
Video.zip (Video was too big and I couldn't get it to save a trimmed version, while testing with Bobbie I had experienced the same issue and had to send the same zip file, apologies in advance!)
Important note
I do not know how to generate unit tests for it, I'm still looking into it.