-
-
Notifications
You must be signed in to change notification settings - Fork 647
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
Wrong or missing key is not highlighted when there is a fallback #2043
Comments
Yes, that's exactly how it should behave. |
It is also tested to behave exactly like that: https://github.com/i18next/i18next/blob/master/test/typescript/custom-types/t.test.ts#L78-L82 |
but then what's the point of having a validation, if i'm not alerted that a key doesn't exist? fallbacks should be more for cases when you can't load the translations due to internet connection perhaps? |
It was a requirement to have TS not complain when defining a defaultValue... |
Yeah I think it would just make sense to have an option to highlight wrong/missing keys even though there is a default value to fall back to. |
Feel free to create a pull request. |
I've spent a few hours on this topic now, and I have to say, I'm not really sure what to do, i'm missing a lot of context of what other functionalities are doing and I wouldn't really feel confident touching this code too much. But while investigating, I've also discovered another problem with the change that was introduced here. I think the change of having "just string" when there's a default value was rushed a bit, would love to have more insights on how to improve this tho :) |
Sorry, but I'm not a TS expert. :-( Maybe @pedrodurek @janpaepke @marcalexiei @hussainahmad @jtbandes @henrikvolmer @JounQin @nicegamer7 can help? |
Hi Sara, If you supply a default value this should be considered valid by i18n-next and thus not report an error. As far as i18n-next is concerned, the code is fine. If you are looking for vars which are missing from your translation files even though you supplied a default value, you might want to have a look at the available parsers (see docs) or maybe there's an eslint plugin which could report a warning. If there's not, you could consider creating one, if your usecase is important enough... |
It's possible to make TS throw an error for extraneous options, I just never opened a PR for this because I wasn't sure if it was useful to anyone. I can open a PR that adds a new option to change the behaviour. Is that preferred, or should we leave this as-is? |
Extraneous options should still be possible. There might be other i18next plugins or postprocessors that needs this. But an option to toggle the TS validation when defaultValue is used, would be a nice addition. |
@janpaepke the default value is not only used when there's no key, but also when translations can't be loaded: instead of showing the translation keys, the default value is being displayed. I think these behaviors should at least coexist, but i wouldn't completely remove the logic to check for the existence of the key and just bulk accept a random string. Also, I'm not sure why we wouldn't want a check for the exact variables to be passed, I would just assume that being the best type safety, but maybe i'm missing something here 😅 Btw, I'm basing this behavior also having seen how flutter is integrated with i18next. |
My point is essentially: If a default is provided, it's not an error if no value exists. That's the whole point of a default value: providing a fallback. Just think of any function where you provide a default value for a parameter. You wouldn't expect typescript to complain if you don't provide one, when you use the function, right? So to your point:
Yes, that's exactly what should happen, because it will show the default. Remember: Key-checking isn't a miracle weapon that prevents you from doing stupid things. It only checks the default translation file. So for example if you only provide a key in the default lang and then forget to add it in other languages, only the closest fallback will be displayed. No error will be shown, even with strict key checking. That being said, I agree that a point can be made to opt into strict key checking, which should be feasible with an option in Wether or not this warrants the extra complexity in the code is for the maintainers to decide and likely depends on how many others also think that this is needed. |
Absolutely agree, but in this case, we're talking about a type safety check, so i agree that we cover the case when the key doesn't exist by showing the default value, but here we're talking about checking if that key exists from a "type" point of view.
that would be great! i think we should cover both cases, just becasue i think it's something that kinda makes sense anyway 😄 i'd love to help adding this, but i'm not feeling confident enough to do it myself right now. would be amazing if i could have some support here |
Optional (or even required) parameters don't exist in vanilla javascript. So the compiler complaining (or not) when providing or missing function parameters is very much a "type safety check" and purely from the "type" point of view. Regarding implementing this as an option I unfortunately lack the time to support. |
Still waiting for someone who wants to try a PR without breaking the existing behaviour. |
i haven't had any time to look into this unfortunately 😓 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@marcalexiei do you like to have a look at this? |
I'll try to wrap things up to see if I have understand the problem and the proposed solution: the idea is to provide an option inside |
Ok for you @SaraRavasio ? |
Just adding, as someone very new to this project, that @SaraRavasio point resonates with me. I would expect to be able to get flagged if a key is missing, even if fallback is provided. To add my pinch of salt, feel free to tell me this is another issue that needs to be filed, but interpolated keys should also fail if the options object is not provided at all (first line of the "wrong"). Interpolation in the default value (the 2&3 lines in the wrong) don't matter to us much, we have a rule to never get fallbacks (to keep TS checks). but if it could be correctly checked, we could get back to provide defaultValue. And while on the subject, it also never consider count to be a missing key in option, even for plural keys, but that's a very nice to have compared to what's discussed here |
addressed in v24.2.0 with the |
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [i18next](https://www.i18next.com) ([source](https://github.com/i18next/i18next)) | dependencies | major | [`23.16.8` -> `24.2.0`](https://renovatebot.com/diffs/npm/i18next/23.16.8/24.2.0) | --- ### Release Notes <details> <summary>i18next/i18next (i18next)</summary> ### [`v24.2.0`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2420) [Compare Source](i18next/i18next@v24.1.2...v24.2.0) - feat(typescript): Add strictKeyChecks option to enforce checking key existence when defaultValue is used [2274](i18next/i18next#2274), fixes [2043](i18next/i18next#2043) ### [`v24.1.2`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2412) [Compare Source](i18next/i18next@v24.1.1...v24.1.2) - optimize fix: Bug Report: Unsafe Behavior in i18n.t Function Leading to Potential Code Execution [2273](i18next/i18next#2273) ### [`v24.1.1`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2411) [Compare Source](i18next/i18next@v24.1.0...v24.1.1) - fix: Bug Report: Unsafe Behavior in i18n.t Function Leading to Potential Code Execution [2273](i18next/i18next#2273) ### [`v24.1.0`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2410) [Compare Source](i18next/i18next@v24.0.5...v24.1.0) - try to address [2270](i18next/i18next#2270) by cloning the store data [2271](i18next/i18next#2271) ### [`v24.0.5`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2405) [Compare Source](i18next/i18next@v24.0.4...v24.0.5) - remove extra log for [2268](i18next/i18next#2268) ### [`v24.0.4`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2404) [Compare Source](i18next/i18next@v24.0.3...v24.0.4) - simplify fix: incorrect locale detected [2268](i18next/i18next#2268) ### [`v24.0.3`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2403) [Compare Source](i18next/i18next@v24.0.2...v24.0.3) - fix: incorrect locale detected [2268](i18next/i18next#2268) - fix: Intl.getCanonicalLocales throws with custom regions [2267](i18next/i18next#2267) ### [`v24.0.2`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2402) [Compare Source](i18next/i18next@v24.0.1...v24.0.2) - if no Intl api, log error and use dummy rule ### [`v24.0.1`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2401) [Compare Source](i18next/i18next@v24.0.0...v24.0.1) - early return and log error, if no Intl api ### [`v24.0.0`](https://github.com/i18next/i18next/blob/HEAD/CHANGELOG.md#2400) [Compare Source](i18next/i18next@v23.16.8...v24.0.0) **This is a major breaking release:** - remove support for older environments - remove old i18next JSON formats - To convert your existing v3 translations to the v4 format, have a look at [i18next-v4-format-converter](https://github.com/i18next/i18next-v4-format-converter) or this [web tool](https://i18next.github.io/i18next-v4-format-converter-web/). - remove support for compatibility to v1 API - Intl API is mandatory now and will not fallback anymore - possible compatibility layer for older formats: `test/compatibility/v4/v4Compatibility.js` - rename `initImmediate` to `initAsync` - fallback to `dev` language if plural rule not found - remove TypeScript v4 support. TypeScript v5 is now an optional peer dependency - addresses - [2244](i18next/i18next#2244) - [2184](i18next/i18next#2184) - [2213](i18next/i18next#2213) - [2206](i18next/i18next#2206) - [2208](i18next/i18next#2208) - [2148](i18next/i18next#2148) - [2254](i18next/i18next#2254) ➡️ check out the [migration guide](https://www.i18next.com/misc/migration-guide#v23.x.x-to-v24.0.0) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS41Ny40IiwidXBkYXRlZEluVmVyIjoiMzkuNzUuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19--> Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/384 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>
🐛 Bug Report
Wrong or missing key is not highlighted when there is a fallback.
To Reproduce
Expected behavior
In both cases I'm referencing a key that doesn't exist, but in the first case I have a fallback, and the wrong key is not highlighted. In the second case, where i don't have a fallback, I get it highlighted.
Your Environment
i18next.d.ts
Resources generated following this step
The text was updated successfully, but these errors were encountered: