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

Redesign t function types #1911

Merged
merged 3 commits into from
Jun 8, 2023
Merged

Conversation

pedrodurek
Copy link
Member

@pedrodurek pedrodurek commented Feb 11, 2023

Description

The existing implementation of the type for the t function is characterized by being very complex, slow, hard to maintain, and not scalable, which are also hampered by various limitations.

Our redesign endeavours to enhance the approach to parsing and inferring keys for the t function. Instead of performing a recursive examination of each key-value pair in resources associated with specific namespace(s) each time the t function is invoked, we generate a comprehensive set of keys from all namespaces just once. Then, we parse and filter the keys based on the designated namespace and key prefix. The outcome of this redesign is that the types have been greatly simplified, and are faster, smarter, more scalable, and are easier to maintain. This upgrade will allow us to mitigate numerous issues raised by devs.

Features

  • When loading multiple namespaces (react-i18next), t function will infer and accept the keys for the first namespace. So this pattern will be accepted now:

Screenshot 2023-02-12 at 9 40 06 PM

  • t function will now infer and accept the keys for the main namespace (i18next):

Screenshot 2023-02-12 at 9 48 31 PM

  • We're introducing a new type (returnObjects) that will infer fewer keys if set to false, and all keys and sub-keys if set to true. If the option returnObjects from t function is set to true, it'll work the same way:

Screenshot 2023-02-12 at 9 52 07 PM

Screenshot 2023-02-12 at 9 57 43 PM

Screenshot 2023-02-12 at 10 03 12 PM

Screenshot 2023-02-12 at 10 08 18 PM

Fixes

This'll close: i18next/react-i18next#1422, i18next/react-i18next#1571 and #1886.
And it'll probably close: i18next/react-i18next#1600, i18next/react-i18next#1601, i18next/react-i18next#1608, #1901 and #1883.
Many more...

Breaking changes

All breaking changes described below are minor ones:

  1. Projects with the option returnObjects set as true by default will also have to set the same option in the CustomTypeOptions type. Otherwise, only complete keys will be allowed (key1.key2.key3...).
 // i18next.d.ts
 import 'i18next';
 declare module 'i18next' {
    interface CustomTypeOptions {
     returnObjects: true
      ...
  1. Renaming StringMap to $Dictionary, and we'll no longer export it. $Dictionary is an internal helper, and there is no reason to expose it. If needed, you can create a copy and reuse it in your project.
  2. ns property from InterpolationOptions type will be constrained to Namespace rather than string or readonly string[].
  3. Renaming KeysWithSeparator type to JoinKeys, and it will no longer be exposed.
  4. Renaming TFuncKey type to ParseKeys, and it will no longer be exposed.
  5. Removing NormalizeByTypeOptions type.
  6. Renaming DefaultTFuncReturnWithObject type to DefaultTReturn. It will accept TOptions as generic constraint and will no longer be exposed.
  7. Removing DefaultTFuncReturn type in favour of DefaultTReturn.
  8. Removing NormalizeReturn type.

Hopefully I didn't forget anything 😅 .

Benchmark results

Num keys: 11,429
Num namespaces: 55
Compilation time with the types: ~6.27s
Compilation time without the types: ~1.49s
VSCode autocomplete keys (without cache): ~2.477s

Limitations

  • Some features won't work appropriately in typescript versions 4.1 or lower.
  • If an array of namespaces is passed to the ns option from t function, all namespaces passed are considered default.

Screenshot 2023-02-13 at 12 49 01 PM

We'll be able to tackle this issue in typescript 5 with the new const Type Parameters. Alternatively, you can convert the array to literal values with as const:

const result = t('...', {ns: ['ns1', 'ns2'] as const});

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

@pedrodurek
Copy link
Member Author

pedrodurek commented Feb 13, 2023

There are a few remaining tasks that I'll be working on in the next few days before merging this PR, that includes:

  • Open a PR for react-i18next repo supporting the new types.
  • Open a PR for next-i18next repo supporting the new types.
  • Create new tests and update current ones.
  • Update i18next documentation.
  • Include benchmark results here.

After merging this PR:

  • Update examples.

@kkuegler
Copy link
Member

(this comment is somewhat tangential to this PR, but I think being aware of this can help shape this new typing approach)

Hi @pedrodurek,

earlier this year, I tried to come up with some type fixes for getFixedT() in #1898.

Maybe we can incorporate some of those ideas into your new typing approach. Also, that PR had some new test cases that might make sense to incorporate here/add later.

getFixedT()

To me, it seems the types for getFixedT() should work somewhat differently to the useTranslation() ones:

Adapting your first example from the PR description, getFixedT(null, ['ns1', 'ns2']) should allow keys from both namespaces both without a ns prefix and with a prefix, i.e. 'key1' | 'key2' | 'key3' | 'key4' | 'ns1:key1' | 'ns1:key2' | 'ns2:key3' | 'ns2:key4'.

getFixedT(null, ['ns1']) should allow 'key1' | 'key2' | 'ns1:key1' | 'ns1:key2'. This seems to work fine with this PR.

getFixedT(null, 'ns1') (with a string instead of an array) should allow the same keys, but with this PR only allows 'key1' | 'key2'.
To change this, one could change the ParseKeysByNamespaces type to this:

type $Flatten<A> = A extends ReadonlyArray<infer E> ? E : A;
type ParseKeysByNamespaces<
  Ns extends Namespace,
  Keys,
  UnionNsps = $Flatten<Ns>,
> = UnionNsps extends keyof Keys ? AppendNamespace<UnionNsps, Keys[UnionNsps]> : never;

As outlined in #1889 (the bug corresponding to the PR mentioned above), getFixedT() - like i18next.t() - should allow the keys of all other namespaces available in i18next with the respective namespace prefix - even if they are not mentioned via the namespace parameter to getFixedT().

So in fact getFixedT(null, ['ns1']) and getFixedT(null, 'ns1') should allow 'key1' | 'key2' | 'ns1:key1' | 'ns1:key2' | 'ns2:key3' | 'ns2:key4'. This is quite different to how useTranslation() seems to work.

Lots of TS key suggestions

These are the same keys that are valid for your i18next.t() example. In the real world, there are much more translation keys. Also, the non-namespaced keys don't usually happen to be alphabetically before the namespaced keys like in the example. Due to this, TS will suggest a lot of keys and will happily mix ns-prefixed and unprefixed keys. I think this can be quite a mess to look at, making the key suggestions less helpful.

To cope with that I came up with the (hack-ish) type DisableSuggestion (see https://github.com/kkuegler/i18next/blob/18c6274451d133f8df98c91b53b8b98ba4db15d5/index.d.ts#L10). This can allow some keys, but keep them out of the TS suggestion list. I hoped to find more "official" ways to do this, but couldn't find any.

The end :)

I'm not sure if you can incorporate some of these aspects into this PR or not, but in the long run I think we should address these, so the runtime behavior and the types for getFixedT() match.

@jamuhl
Copy link
Member

jamuhl commented Feb 19, 2023

Perhaps we should make clear what useTranslation is...it's only reason to exist is to assert the namespaces provided are loaded (nothing more) before rendering (and is only needed in combination with using backends).

the getFixedT is just a helper to get the t function, but not bound to the default namespace and bound to namespace(s) passed in (bound means just not needing the namespace prefix ns1:key).

t function itself does not care about translations being loaded or not. It just looks up translations based on given namespace and key (and does this by using default namespace or given namespace as prefix to the key)

Under the hood useTranslation calls the getFixedT function and returns the provided bound t function (beside other stuff). But the important thing is calling loadNamespaces and throwing the promise if not yet loaded - and triggering Suspense (or returning the ready flag with value false)

@VictorGlindasPaf
Copy link

VictorGlindasPaf commented Mar 2, 2023

This looks like a fantastic improvement.
We're using these updated types at work, and have some feedback. I know it's a W.I.P, but could be useful nonetheless.

  • The types are faster, but still very slow when we have many keys. We have about 1500 keys spread out in ~60 namespaces in our project, and strictly typing them makes the tsc compilation time go from ~120s to ~250s.

  • Autocomplete of the interpolated values is cool, but seems to break when using multiple namespaces and accessing the key without the namespace prefix. It could be very useful be able to enforce that you can't use interpolated values that are not defined in the types.

  • Typescript doesn't seem to be able to keep track of the string references, so we can't "go to definition", and "go to references". It would be fantastic if there was some way to accomplish this, as it would make finding unused translations very easy.

@diegodorado
Copy link

Thanks for the thourough description of what the PR does @pedrodurek ! I am eager to see this merged, and I am specially interested in typed interpolation params.

Until this is merged, I have a quick question on how useTranslation would be better used:

With i18next@22.4.11 and react-i18next@12.2.0, and having resources defined as

const ns1 = {
  key1: '',
}

const ns2 = {
  key2: '',
}

const resources = {
  en: {
    ns1,
    ns2,
  },
}

Depending if I call useTranslation(nsArray) or useTranslation<NSType>() I get different allowed keys for t.

With the first one:

  const { t } = useTranslation(['ns1', 'ns2'])
  t('key1') // no error
  t('ns1:key1') // no error
  t('key2') // TSC Error: No overload matches
  t('ns2:key2') // no error

While the later:

  const { t } = useTranslation<'ns1'|'ns2'>()
  t('key1') // no error
  t('ns1:key1') // TSC Error: No overload matches
  t('key2') // no error
  t('ns2:key2') // TSC Error: No overload matches

As I understand, using one or the other might be subject to preference or use cases, but curious if there is any reason to use one or the other or if the PR proposal is to deprecated one of these.

Thanks!

@stale
Copy link

stale bot commented Mar 25, 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 Mar 25, 2023
@pedrodurek pedrodurek removed the stale label Mar 25, 2023
@falsandtru
Copy link

Can this resolve #1894?

@pedrodurek
Copy link
Member Author

pedrodurek commented Apr 15, 2023

Benchmark results in Mac M1:

Num keys: 11,429
Num namespaces: 55
Compilation time with the types: ~6.27s
Compilation time without the types: ~1.49s
VSCode autocomplete keys (without cache): ~2.477s

I won’t share the results of the current types because it doesn’t handle these many keys/namespaces. 😅

I’ll share the project that I used for the benchmark once we merge this PR.

I do believe that I still can improve the compilation in 30-40%, but I'll work on it in another PR.

@pedrodurek
Copy link
Member Author

Hey @falsandtru, I'm not sure, but we can work something out once it's merged

@falsandtru
Copy link

I see

@VictorGlindasPaf
Copy link

VictorGlindasPaf commented Apr 18, 2023

We found another issue in the new types.
When the namespace can be one of several ones, it doesn't complain if we access a key that only exists on one of them, but not the other.

Screenshot 2023-04-18 at 10 31 58

In the above example, choose-amount.heading is available only on deposit, but we get no error.

@stale
Copy link

stale bot commented Apr 25, 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 Apr 25, 2023
@adrai adrai removed the stale label Apr 26, 2023
@pedrodurek pedrodurek force-pushed the redesign-t-function-types branch 2 times, most recently from 48b0f7b to ee546e6 Compare April 27, 2023 03:39
@adrai
Copy link
Member

adrai commented Apr 29, 2023

fyi: ts tests are failing: https://app.circleci.com/pipelines/github/i18next/i18next/1104/workflows/55564641-6a5f-462f-af39-139d8e209ba8/jobs/908/parallel-runs/0/steps/0-105

@pedrodurek
Copy link
Member Author

Hey @adrai, it's safe to merge it, I'll update the other PRs once this one is merged.

Do you also want to update the typescript dependency?

Done!

Can you also adapt the docs afterwards, if necessary?

Do you mean this?

@pedrodurek does this example also need to be updated?

I'll update the examples with the correct i18n version once the PR is merged.

@pedrodurek pedrodurek requested a review from adrai June 8, 2023 06:57
@adrai adrai merged commit 3c5308e into i18next:master Jun 8, 2023
4 of 5 checks passed
@adrai
Copy link
Member

adrai commented Jun 8, 2023

fyi: goal is to release a new major version with #1885 and #1945 and also addressing some other stuff: #1884
but first we need to address:
#1885 (comment)
#1911 (review)

@adrai
Copy link
Member

adrai commented Jun 8, 2023

Do you mean this?

@pedrodurek I mean because of this

@adrai
Copy link
Member

adrai commented Jun 9, 2023

@pedrodurek now all but this is ready to release the major version...
Can you check the example and describe how the new typesafe translation approach should be set up in a project?

@pedrodurek does this example also need to be updated? https://github.com/i18next/i18next/tree/master/examples/typescript it seems to not work with the new index.d.ts
image

@adrai
Copy link
Member

adrai commented Jun 10, 2023

seems the compilerOptions needs to be strict: b7075f2#diff-603812bc45f8ec4c682a4be24c9423d325e92c7ea7f10afe1bd84b6c46077164R10

@adrai
Copy link
Member

adrai commented Jun 12, 2023

stuff to check before releasing this:

@adrai
Copy link
Member

adrai commented Jun 12, 2023

seems because typeof here is just using string type for the translation values, so there is no info of the interpolation anymore...

@pedrodurek a possible workaround:

// the resources.d.ts file is generated with npm run i18next-resources-for-ts

image

@adrai
Copy link
Member

adrai commented Jun 13, 2023

"strictNullChecks": true is sufficient

@janpaepke
Copy link

janpaepke commented Jun 29, 2023

Hi guys. This update reintroduced an issue I helped fix in the previous version:
For details see here: #1867

Essentially, when the t function receives a default value (either as a string, or as part of the defaultValue prop in the options), typescript should not enforce "known keys only".

That's the whole point of the default value option. If we're forcing people to always have a value for every key in their main namespace, then this entire option is completely useless, as it will always be overwritten anyway.

The same is true by the way for the Trans component of react-i18next, but this is probably less of your concern.

@adrai
Copy link
Member

adrai commented Jun 29, 2023

Hi guys. This update reintroduced an Issue I had fixed in the previous version: For details see here: #1867

Essentially, when the t function receives a default value (either as a string, or as part of the defaultValue prop in the options), typescript should not enforce "known keys only".

That's the whole point of the default value option. If we're forcing people to always have in their main namespace, then this entire option is completely useless, as it will always be overwritten anyway.

The same is true by the way for the Trans component of react-i18next, but this is probably less of your concern.

@pedrodurek what do you think?

@janpaepke
Copy link

janpaepke commented Jun 29, 2023

addendum

My gut instinct would be to solve this via overloads:

/**************************
 * T function declaration *
 **************************/
export interface TFunction<Ns extends Namespace = _DefaultNamespace, KPrefix = undefined> {
  // known keys
  <
    Key extends ParseKeys<Ns, TOpt, KPrefix> | TemplateStringsArray,
    TOpt extends TOptions,
    Ret extends TFunctionReturn<Ns, AppendKeyPrefix<Key, KPrefix>, TOpt>,
  >(
    ...args:
      | [key: Key | Key[], options?: TOpt & InterpolationMap<Ret>]
      | [key: Key | Key[], defaultValue: string, options?: TOpt & InterpolationMap<Ret>]
  ): TFunctionReturnOptionalDetails<Ret, TOpt>;
  // unknown keys
  <
    TOpt extends TOptions,
    Ret extends TFunctionReturn<Ns, AppendKeyPrefix<Key, KPrefix>, TOpt>,
  >(
    ...args:
      | [key: string | string[], options?: TOpt & InterpolationMap<Ret> & { defaultValue: string }]
      | [key: string | string[], defaultValue: string, options?: TOpt & InterpolationMap<Ret>]
  ): TFunctionReturnOptionalDetails<Ret, TOpt>;
}

But you likely had a reason for eliminating them (probably speed?).
I haven't tested or benchmarked the above, but it might help as a starting point.

@adrai
Copy link
Member

adrai commented Jun 29, 2023

@janpaepke can you try with v23.2.6 ?

@janpaepke
Copy link

I did and it works. 👍
Thanks for your quick reaction!

It let's me set any string as the key, when a default value is provided and complains if there's not.
Is it possible to implement a similar solution for the Trans component? (default value is children in that case).
Or should I post this in a different repo?

There's another thing I noticed, but I am not entirely sure if it's related to my environment (IDE/ts version).

Key-Autocompletion for the t function doesn't seem to work for me any more.
Interestingly it does still work for the Trans Component.
And yes, this was also the case without this latest change.

Should I create a separate issue for that?

@adrai
Copy link
Member

adrai commented Jul 3, 2023

@janpaepke intellisense is tracked here: microsoft/TypeScript#54708 (probably fixed in next ts version and/or ide version)
Regarding Trans component, yes open a new issue in the react-i18next repo. btw: alternatively, just omit the i18nKey for the Trans component.

@janpaepke
Copy link

@adrai thanks a lot!
Trans Component -> will add an issue.
re your workaround: How would leaving out the key work? It couldn't relate the respective translation vars, if available, right?
Plus i18next-parser can't extract it anymore... Sorry, if off topic.

@bencerf
Copy link

bencerf commented Jul 13, 2023

In a React Component, when I'm loading a namespace foo (not configured to be the default ns !), I got an TS error by not specifying the namespace in the t function :

const { t } = useTranslation('foo');
...
<div>
  // TS error
  {t('key')}
  // No TS error
  {t('foo:key')}
</div>

The TS error says Argument of type '["key"]' is not assignable to parameter of type '[key: TemplateStringsArray |...... I thought it was possible, not to specify the namespace but seems, it's requiered with v23.

Should you update this documentation of loading namespace in react-i18next ?

Thanks in advance for your answer

@adrai
Copy link
Member

adrai commented Jul 13, 2023

In a React Component, when I'm loading a namespace foo (not configured to be the default ns !), I got an TS error by not specifying the namespace in the t function :

const { t } = useTranslation('foo');
...
<div>
  // TS error
  {t('key')}
  // No TS error
  {t('foo:key')}
</div>

The TS error says Argument of type '["key"]' is not assignable to parameter of type '[key: TemplateStringsArray |...... I thought it was possible, not to specify the namespace but seems, it's requiered with v23.

Should you update this documentation of loading namespace in react-i18next ?

Thanks in advance for your answer

@MiryksV please provide a complete but minimal reproducible example repository or similar...

@bencerf
Copy link

bencerf commented Jul 13, 2023

I couldn't reproduce on my sandbox, I'll take a deeper look in my project. Sry to bothering you for nothing.
Thanks for your answer.

@reckter
Copy link

reckter commented Aug 6, 2023

I just adopted this, and the typescript compiler needs about 8.5GB ram now in order to type check our code.
It takes about 1.7min (with a few errors left to fix).

We use the interface generation, because we have our translations in .json files.
The interface file is 8888 lines long (I swear, I didn't make this up xD), with 26 namespaces.

Without the generated interfaces included, typescript needs about ~3s to run.

If you have any ideas on why it might take so long, I would really appreciate it!

Edit: after getting rid of all TS errors, it went down to 3s runtime, and about 400Mb ram.

@reckter
Copy link

reckter commented Aug 8, 2023

Seems like, once I had a no-error run the tsc cache really speeds up the compiler.
Every time I bust the cache (manual deletion, or different branches) it will take up to ~2.5 minutes and over 8GB of ram.
After a successful run it will be in the low seconds again.

@semenov-ol
Copy link

Hey, before i can use generic type to specify which type would return
i18n.t<string, string[]>('i18nkey', { returnObjects: true })

How can i do the same with new version, when passing third param ts complain that 'Type  string[]  does not satisfy the constraint  string'?
i18n.t<string, any, string[]>('i18nkey', { returnObjects: true })

@felixmeziere
Copy link
Contributor

I have the same issue as @reckter with very long tsc run times because of this, any way to solve the issue ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet