feat(typescript): Add `strictKeyChecks` option to enforce checking key existence when `defaultValue` is used #2274

masnormen · 2024-12-18T09:04:24Z

In my experience, I use defaultValue for quick prototyping, but sometimes I forgot to add the actual key to my translation JSON file and it goes unnoticed because it doesn't produce errors.

This PR introduces a new option strictKeyChecks, which is customizable via user-augmented CustomTypeOptions interface. Enabling this option will make it illegal to reference a key that doesn't exist, even when defaultValues are provided.

If accepted, this PR closes #2043

Examples:

// public/locales/en/common.json
{
  "button": {
    "save": "Save"
  }
}

without strictKeyChecks

// component.ts
export default function I18nextTest() {
  const { t } = useTranslation('common');

  t('button.save'); // ✅
  t('button.save', 'Default'); // ✅
  t('button.save', { defaultValue: 'Default' }); // ✅

  t('wrong'); // ❌ ts error
  t('wrong', 'Default'); // ✅ <= still pass typecheck 
  t('wrong', { defaultValue: 'Default' }); // ✅ <= still pass typecheck 
}

with strictKeyChecks = true

// i18next.d.ts
declare module 'i18next' {
  interface CustomTypeOptions {
    strictKeyChecks: true;
    ...
  }
}


// component.ts
export default function I18nextTest() {
  const { t } = useTranslation('common');

  t('button.save'); // ✅
  t('button.save', 'Default'); // ✅
  t('button.save', { defaultValue: 'Default' }); // ✅

  t('wrong'); // ❌ ts error
  t('wrong', 'Default'); // ❌ ts error
  t('wrong', { defaultValue: 'Default' }); // ❌ ts error
}

Checklist

only relevant code is changed (make a diff before you submit the PR)
run tests npm run test
tests are included
commit message and code follows the Developer's Certification of Origin

Checklist (for documentation change)

only relevant documentation part is changed (make a diff before you submit the PR)
motivation/reason is provided
commit message and code follows the Developer's Certification of Origin

typescript/t.d.ts

coveralls · 2024-12-18T09:14:42Z

coverage: 96.018%. remained the same
when pulling 0289699 on masnormen:feat/strict-key-checks
into c51950e on i18next:master.

adrai · 2024-12-18T09:15:31Z

Nice...
Does this come with any performance implication?
As soon as @marcalexiei approves this, we merge it.

masnormen · 2024-12-19T05:50:08Z

fixed and added the test. could you please recheck @marcalexiei?

marcalexiei

This fixes the previous issue,
however now InterpolationMap is no longer considered when type checking t options:

Testcase (this time the additional describe block is good):

describe('interpolation values with strictKeyChecks == true', () => {
  const t = (() => '') as unknown as TFunction<'interpolators'>;

  it('single interpolation value', () => {
    expectTypeOf(t('simple', { val: 'val' })).toMatchTypeOf<string>();

    // @ts-expect-error val interpolation value is missing
    expectTypeOf(t('simple', { notPresent: 'notPresent' })).toMatchTypeOf<string>();
  });

  it('multiple interpolation values', () => {
    type Expected = 'This has {{more}} than {{one}}';
    expectTypeOf(t('simple_multiple_keys', { more: '', one: '' })).toEqualTypeOf<Expected>();

    // @ts-expect-error "more" required key is missing
    expectTypeOf(t('simple_multiple_keys', { less: '', one: '' })).toEqualTypeOf<Expected>();
  });
});

Note

You need to include interpolators namespace in test/typescript/strict-key-checks/i18next.d.ts

import 'i18next';

 import {
   TestNamespaceCustom,
   TestNamespaceCustomAlternate,
   TestNamespaceFallback,
+  TestNamespaceInterpolators,
 } from '../test.namespace.samples';

 declare module 'i18next' {
   interface CustomTypeOptions {
     strictKeyChecks: true;
     defaultNS: 'custom';
     fallbackNS: 'fallback';
     resources: {
       custom: TestNamespaceCustom;
       alternate: TestNamespaceCustomAlternate;
       fallback: TestNamespaceFallback;
+      interpolators: TestNamespaceInterpolators;
     };
   }
 }

marcalexiei · 2024-12-19T05:58:30Z

test/typescript/strict-key-checks/t.test.ts

+  });
+});
+
+describe('t defaultValue with strictKeyChecks == true and `returnObjects`', () => {
+  const t = (() => '') as unknown as TFunction<'alternate'>;
+
+  it('should work alongside `returnObjects`', () => {
+    expectTypeOf(t('foobar', { returnObjects: true })).toMatchTypeOf<{
+      barfoo: 'barfoo';
+      deep: {
+        deeper: {
+          deeeeeper: 'foobar';
+        };
+      };
+    }>();
+  });
+});


Stylistic issue: please move this test inside the describe block above. No need to have another block.
I hadn't released that there is not need for an additional block.

Suggested change

});

});

describe('t defaultValue with strictKeyChecks == true and `returnObjects`', () => {

const t = (() => '') as unknown as TFunction<'alternate'>;

it('should work alongside `returnObjects`', () => {

expectTypeOf(t('foobar', { returnObjects: true })).toMatchTypeOf<{

barfoo: 'barfoo';

deep: {

deeper: {

deeeeeper: 'foobar';

};

};

}>();

});

});

});

it('should work alongside `returnObjects`', () => {

expectTypeOf(t('foobar', { returnObjects: true })).toMatchTypeOf<{

barfoo: 'barfoo';

deep: {

deeper: {

deeeeeper: 'foobar';

};

};

}>();

});

});

masnormen · 2024-12-19T06:28:44Z

You're right. I think the current solution is too 'hacky' and therefore break other things... Let me think of a cleaner solution for a bit

masnormen · 2024-12-19T10:52:54Z

typescript/t.d.ts

+export type TFunction<
+  Ns extends Namespace = DefaultNamespace,
+  KPrefix = undefined,
+> = _StrictKeyChecks extends true ? TFunctionStrict<Ns, KPrefix> : TFunctionNonStrict<Ns, KPrefix>;
+


Still not sure what are the possible implications of converting this interface into a type (although it will eventually resolve into an interface)

(also the naming)

masnormen · 2024-12-19T11:18:01Z

Some weird effects of this (or maybe just my lack of knowledge 🫠) is we sometimes have to use as unknown when casting the type of strict TFunction for tests. Not sure why TFunctionStrict acts different from TFunctionNonStrict

marcalexiei · 2024-12-19T15:13:41Z

@masnormen you requested my review but you are still pushing commits, request a review when you have finished.

masnormen · 2024-12-19T15:50:00Z

@masnormen you requested my review but you are still pushing commits, request a review when you have finished.

Sorry @marcalexiei, my bad! I'm trying to make sure there are no perf issues and got some problem with vitest. Should be done now.

By the way, I followed the tutorial on CONTRIBUTING.md to test multiple tsconfig within the same scenario. But it turns out that Vitest will do multiple writes into the same tsconfig.vitest-temp.json file when there are multiple tsconfig.*.json, resulting in a broken temp tsconfig JSON and therefore broken test run:

The temp file name also seems to be hardcoded here so can't really do anything atm.

So for now I just copied the many-keys folder and adjusted many-keys-strict to check against the strictKeyChecks: true option.

marcalexiei

Sorry @marcalexiei, my bad!

No problem! I haven't started the PR review yet, but better to avoid push after requesting a review😉

I'm trying to make sure there are no perf issues

Looks like you did it ✨!
Done a few benchmarks on my biggest package in a mono-repo
Here are the results of running tsc --noEmit:

With current types	With your fix
18.479s	18.610s
19.948s	18.420s
19.077s	18.698s
18.479s	18.804s

I experienced similar results in test/typescript/many-keys test scenarios!

By the way, I followed the tutorial on CONTRIBUTING.md to test multiple tsconfig within the same scenario. But it turns out that Vitest will do multiple writes into the same tsconfig.vitest-temp.json file when there are multiple tsconfig.*.json, resulting in a broken temp tsconfig JSON and therefore broken test run:

~~I know, that why I used the singular sentence:~~

better to add a warning to avoid this kind of situation in the future.
Thanks for bringing this up!

I'll fix this issue in a separate PR

So for now I just copied the many-keys folder and adjusted many-keys-strict to check against the strictKeyChecks: true option.

I'm going to create a PR on your fork to avoid namespace duplication ~~and add a warning regarding testing vite with multiple --project flags. After that we can merge this~~.

refactor: dedupe many-keys namespace and PERF_README files

marcalexiei

I created an issue on vitest repository about the tsconfig.vitest-temp.json:
vitest-dev/vitest#7107

Anyway if the CI will be green this can be merged

adrai · 2024-12-20T05:27:30Z

Thanks to both...
it's included in v24.2.0

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. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  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>

masnormen added 2 commits December 18, 2024 15:37

feat: add option strictKeyChecks on TypeOptions

229d9a0

test: tests for strictKeyChecks

19bb0d5

masnormen commented Dec 18, 2024

View reviewed changes

typescript/t.d.ts Outdated Show resolved Hide resolved

masnormen marked this pull request as ready for review December 18, 2024 09:11

adrai requested a review from marcalexiei December 18, 2024 09:13

adrai added the typescript label Dec 18, 2024

This comment was marked as resolved.

Sign in to view

fix: change ActualOptions into TOpt & $Dictionary

f6081de

marcalexiei suggested changes Dec 19, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

masnormen added 6 commits December 19, 2024 14:16

fix: add interpolation tests

2ebf270

fix: working TFunction signature for now

2f77a92

test: add array tests

0658a8f

fix: tidy up code

63ac839

fix: cleanup unnecessary changes

9eb119a

test: some more cleanup

846fd99

masnormen requested a review from marcalexiei December 19, 2024 10:31

masnormen commented Dec 19, 2024

View reviewed changes

masnormen added 2 commits December 19, 2024 20:33

fix: put back noinfer

c725901

test: add some more test for many keys and misc

d84bfd2

marcalexiei removed their request for review December 19, 2024 15:13

test: duplicate many-keys for strict key checks scenario

e6150c6

test: add additional signatures

f94c1a2

masnormen requested a review from marcalexiei December 19, 2024 16:09

marcalexiei reviewed Dec 19, 2024

View reviewed changes

marcalexiei and others added 2 commits December 19, 2024 20:25

refactor: dedupe many-keys namespace and PERF_README files

4bd30d5

Merge pull request #1 from marcalexiei/feat/strict-key-checks

0289699

refactor: dedupe many-keys namespace and PERF_README files

marcalexiei mentioned this pull request Dec 20, 2024

workspace typecheck: tsconfig.vitest-temp.json is overridden if more workspace target the same folder with a different tsconfigs vitest-dev/vitest#7107

Closed

6 tasks

marcalexiei self-requested a review December 20, 2024 03:29

marcalexiei reviewed Dec 20, 2024

View reviewed changes

adrai merged commit 62daae4 into i18next:master Dec 20, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(typescript): Add `strictKeyChecks` option to enforce checking key existence when `defaultValue` is used #2274

feat(typescript): Add `strictKeyChecks` option to enforce checking key existence when `defaultValue` is used #2274

masnormen commented Dec 18, 2024

coveralls commented Dec 18, 2024 •

edited

Loading

adrai commented Dec 18, 2024

This comment was marked as resolved.

masnormen commented Dec 19, 2024

marcalexiei left a comment

marcalexiei Dec 19, 2024

masnormen commented Dec 19, 2024

This comment was marked as outdated.

masnormen Dec 19, 2024 •

edited

Loading

masnormen commented Dec 19, 2024 •

edited

Loading

marcalexiei commented Dec 19, 2024

masnormen commented Dec 19, 2024

marcalexiei left a comment •

edited

Loading

marcalexiei left a comment

adrai commented Dec 20, 2024

feat(typescript): Add strictKeyChecks option to enforce checking key existence when defaultValue is used #2274

feat(typescript): Add strictKeyChecks option to enforce checking key existence when defaultValue is used #2274

Conversation

masnormen commented Dec 18, 2024

without strictKeyChecks

with strictKeyChecks = true

Checklist

Checklist (for documentation change)

coveralls commented Dec 18, 2024 • edited Loading

adrai commented Dec 18, 2024

This comment was marked as resolved.

masnormen commented Dec 19, 2024

marcalexiei left a comment

Choose a reason for hiding this comment

marcalexiei Dec 19, 2024

Choose a reason for hiding this comment

masnormen commented Dec 19, 2024

This comment was marked as outdated.

masnormen Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

masnormen commented Dec 19, 2024 • edited Loading

marcalexiei commented Dec 19, 2024

masnormen commented Dec 19, 2024

marcalexiei left a comment • edited Loading

Choose a reason for hiding this comment

marcalexiei left a comment

Choose a reason for hiding this comment

adrai commented Dec 20, 2024

feat(typescript): Add `strictKeyChecks` option to enforce checking key existence when `defaultValue` is used #2274

feat(typescript): Add `strictKeyChecks` option to enforce checking key existence when `defaultValue` is used #2274

coveralls commented Dec 18, 2024 •

edited

Loading

masnormen Dec 19, 2024 •

edited

Loading

masnormen commented Dec 19, 2024 •

edited

Loading

marcalexiei left a comment •

edited

Loading