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

Wrong or missing key is not highlighted when there is a fallback #2043

Closed
SaraRavasio opened this issue Sep 26, 2023 · 23 comments · Fixed by #2274
Closed

Wrong or missing key is not highlighted when there is a fallback #2043

SaraRavasio opened this issue Sep 26, 2023 · 23 comments · Fixed by #2274

Comments

@SaraRavasio
Copy link

🐛 Bug Report

Wrong or missing key is not highlighted when there is a fallback.

To Reproduce

image

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": "^23.5.1"
  • "react-i18next": "^13.2.2"
  • "i18next-resources-for-ts": "^1.3.3"

i18next.d.ts

import Resources from './resources';

declare module 'i18next' {
  interface CustomTypeOptions {
    defaultNS: 'myfile';
    resources: Resources;
  }
}

Resources generated following this step

@adrai
Copy link
Member

adrai commented Sep 26, 2023

Yes, that's exactly how it should behave.
For the first example: since you defined a default value, it's all ok, because at runtime you will see the default translation value.
For the second example: you did not define a default value, that's not ok, because at runtime you will see the i18n key and no translation

@adrai
Copy link
Member

adrai commented Sep 26, 2023

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

@SaraRavasio
Copy link
Author

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?

@adrai
Copy link
Member

adrai commented Sep 26, 2023

It was a requirement to have TS not complain when defining a defaultValue...
If you want you can suggest a new PR that introduces a new option that would change that behaviour.

@SaraRavasio
Copy link
Author

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.
These default values are used when translations can't be loaded in general, not only if the key is missing, so I really don't see how one thing should exclude the other 😄

@adrai
Copy link
Member

adrai commented Sep 26, 2023

Feel free to create a pull request.

@SaraRavasio
Copy link
Author

SaraRavasio commented Sep 26, 2023

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.

image

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 :)

@adrai
Copy link
Member

adrai commented Sep 26, 2023

Sorry, but I'm not a TS expert. :-(

Maybe @pedrodurek @janpaepke @marcalexiei @hussainahmad @jtbandes @henrikvolmer @JounQin @nicegamer7 can help?

@adrai adrai reopened this Sep 26, 2023
@janpaepke
Copy link

janpaepke commented Sep 26, 2023

Hi Sara,
I think we might have a different understanding as to what the error highlight means. To me it makes sense to report an error if there is no actual meaningful string value that would be displayed.

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...

@nicegamer7
Copy link

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?

@adrai
Copy link
Member

adrai commented Sep 26, 2023

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.

@SaraRavasio
Copy link
Author

@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.
That being said, I don't see the point of having a key validation to show us if a key is missing, if we just don't care about it if we have a default value, the two things are not really connected with each other. You have a very specific workflow in which you start by not having the keys and then adding them, but that's not usually the case.
Also, what if someone by mistake removes a character from the key, because there's a default value i shouldn't be alerted that translations are using a wrong/missing key?

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.

@janpaepke
Copy link

janpaepke commented Oct 2, 2023

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:

Also, what if someone by mistake removes a character from the key, because there's a default value i shouldn't be alerted that translations are using a wrong/missing key?

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 i18next.d.ts, similar to how allowObjectInHTMLChildren works.

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.

@SaraRavasio
Copy link
Author

You wouldn't expect typescript to complain if you don't provide one, when you use the function, right?

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 being said, I agree that a point can be made to opt into strict key checking, which should be feasible with an option in i18next.d.ts, similar to how allowObjectInHTMLChildren works.

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

@janpaepke
Copy link

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.
You might want to have a look at the option(s) I mentioned and how they are implemented. That might give you some pointers. Best of luck!

@adrai
Copy link
Member

adrai commented Nov 28, 2023

Still waiting for someone who wants to try a PR without breaking the existing behaviour.

@SaraRavasio
Copy link
Author

i haven't had any time to look into this unfortunately 😓

Copy link

stale bot commented Dec 15, 2023

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.

@stale stale bot added the stale label Dec 15, 2023
@stale stale bot removed the stale label Feb 12, 2024
@adrai
Copy link
Member

adrai commented Feb 17, 2024

@marcalexiei do you like to have a look at this?

@marcalexiei
Copy link
Member

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 CustomTypeOptions to optionally enable type checking also on TOptions#defaultValue and TFunction 2nd parameter to be sure that the fallback key exists in available Resources?

@adrai
Copy link
Member

adrai commented Feb 18, 2024

Ok for you @SaraRavasio ?

@PookMook
Copy link

PookMook commented Feb 28, 2024

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.
If this is via an option or something I can setup in my project once to get that behavior, this is fine enough if documented properly. I guess you don't want to break existing workflow and I get that, but probably this should be a sane default for newer projects (have it in the documentation or in your examples)

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").
We are implementing i18n + locize currently, and we currently have a rule to always include an object as second parameter, even if empty, to have peace of mind that TS would catch mistakes.

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.
image

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

@adrai
Copy link
Member

adrai commented Dec 20, 2024

addressed in v24.2.0 with the strictKeyChecks option: https://www.i18next.com/overview/typescript#custom-type-options
image

alexandresoro pushed a commit to alexandresoro/ouca that referenced this issue Dec 23, 2024
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants