Fix context type checking #2182

stefan-schweiger · 2024-05-10T16:06:31Z

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

coveralls · 2024-05-10T16:08:43Z

coverage: 96.313%. remained the same
when pulling 55078b6 on stefan-schweiger:master
into d057e1d on i18next:master.

marcalexiei

Unit test

Unit tests should be added to have this scenario tested properly.
You can add them here:

i18next/test/typescript/custom-types/t.test.ts

Line 143 in 9ce9150

describe('context', () => {

You can update / refine the ones added via #2173.

Note

Be sure to update your branch: I have added a few more tests via #2186

Performance

I performed a benchmark on test/typescript/many-keys test scenario and tsc time went from:

npx tsc --noEmit  4.09s user 0.10s system 247% cpu 1.693 total

to

npx tsc --noEmit  5.86s user 0.23s system 206% cpu 2.942 total

Nearly ~2s up (on a MacBook Air M3).

I haven't tested in a real app.

During this tests no context value has been used, so I think that some optimisation need to be performed, especially due to the issues regarding out of memory and long type-checking time.

typescript/t.d.ts

stefan-schweiger · 2024-05-13T06:59:13Z

Unit test

Unit tests should be added to have this scenario tested properly. You can add them here:

i18next/test/typescript/custom-types/t.test.ts

Line 143 in 9ce9150

describe('context', () => {

You can update / refine the ones added via #2173.

Note

Be sure to update your branch: I have added a few more tests via #2186

Added/extended tests, still all green.

Performance

I performed a benchmark on test/typescript/many-keys test scenario and tsc time went from:
npx tsc --noEmit  4.09s user 0.10s system 247% cpu 1.693 total
to
npx tsc --noEmit  5.86s user 0.23s system 206% cpu 2.942 total
Nearly ~2s up (on a MacBook Air M3).

I haven't tested in a real app.

During this tests no context value has been used, so I think that some optimisation need to be performed, especially due to the issues regarding out of memory and long type-checking time.

I have no real clue about typescript performance optimisations. If you have any concrete suggestions please go ahead.

marcalexiei · 2024-05-13T07:45:41Z

In my real application time goes up from 129.95s to 147.57s.

The performance is affected whether you provide a context to a t function or not.
In my opinion this should not be merged as it is.
Up to @adrai to decide.

I have no real clue about typescript performance optimisations

You can find some information about ts performance here:
https://github.com/microsoft/TypeScript/wiki/Performance#performance-tracing

and you can test them in i18next here:
https://github.com/i18next/i18next/blob/master/test/typescript/many-keys/PERF_README.md

If you have any concrete suggestions please go ahead.

I don't have time to perform an in depth review at this time.
The only suggestion I have, so far, is to try to find a way to compute ContextOfKey only when context key is provided.
This is something similar to what has been done in #2095

adrai · 2024-05-13T07:58:52Z

The performance implication is to heavy... so please @stefan-schweiger try to find a way to way to compute ContextOfKey only when context key is provided, like suggested above.

stefan-schweiger · 2024-05-13T08:07:00Z

@marcalexiei how do you arrive at those performance numbers? running time npx tsc --noEmit in the test/typescript/many-keys takes 5.9 seconds (M1 Pro) for me no matter if my code is there or not. Is there some cache or something I'm not taking into account?

stefan-schweiger · 2024-05-13T08:27:27Z

I've now even cloned a completely fresh master branch from your repo and I still get the same performance time. Are you sure this is an issue caused by my PR?

marcalexiei · 2024-05-13T08:29:38Z

upstream remote is i18next
context-fix is your repository

stefan-schweiger · 2024-05-13T08:39:45Z

Well that's just not what's happening for me. Where do we go from here?

stefan-schweiger · 2024-05-13T08:44:24Z

@adrai can you maybe confirm either result?

marcalexiei · 2024-05-13T08:45:10Z

I'm trying on another machine

marcalexiei · 2024-05-13T08:47:43Z

On M2 Max there is no performance issue...

marcalexiei · 2024-05-13T08:55:10Z

After you added improve readability of typing commit the performance issue is no longer present

marcalexiei · 2024-05-13T09:00:54Z

stefan-schweiger · 2024-05-13T09:09:38Z

I'm not sure I see how moving the code around a bit would make such a difference, but I guess I'll take it. So this can be merged then?

marcalexiei · 2024-05-13T09:25:48Z

I'm not sure I see how moving the code around a bit would make such a difference

Actually it does: you moved the context validation from the ActualOptions type parameter to the function parameters. This likely makes ts compute ContextOfKey only when necessary.

So this can be merged then?

Since the implementation is changed I would like to perform additional checks, including a real application.
I'll review the new implementation later since I no longer have time today.

stefan-schweiger · 2024-05-13T14:38:28Z

test/typescript/custom-types/t.test.ts

+ // TODO: edge case which is not correctly detected currently
+ // expectTypeOf(
+ // // @ts-expect-error no default context so it must give a type error
+ // t('dessert', { context: 'cake' as 'cake' | undefined }),
+ // ).toEqualTypeOf<never>();


@marcalexiei while using this in my actual app I found this edge case which is uncovered. Wasn't able to figure out how to correctly handle this yet, maybe you have an idea. Overall I think it's not a blocker for the PR as it's still an improvement over the current functionality, but if possible to find a solution for this as well it would be great.

also can in theory be covered by writing code like this value ? t('dessert', { context: value }) : t('dessert'). Which is not ideal but helps.

marcalexiei

I discovered a few regressions:

usage with a t function using multiple namespaces
usage with a t function with an explicit namespace inside the key

I created a PR on master with the additional test cases: #2188

stefan-schweiger · 2024-05-14T08:03:26Z

To be honest, this is getting above my knowledge of the type system, I have tried to tinker around a bit but with no real solution in sight. I'm having a hard time to figure out how the whole NS/Prefix detection is meant to work, so if you can provide any input it would be appreciated.

stefan-schweiger · 2024-05-14T08:10:33Z

Ok, I'm not sure I understand the solution, but somehow I got the tests to be green again, please take a look if whatever I did make sense

marcalexiei

@stefan-schweiger request a review from me when you are done with your changes.
I don't want to risk performing tests on outdated code or code that you are working on.

stefan-schweiger · 2024-05-14T12:55:25Z

@stefan-schweiger request a review from me when you are done with your changes. I don't want to risk performing tests on outdated code or code that you are working on.

I'm still trying to figure out a solution for this t('dessert', { context: undefined as 'cake' | undefined }) but so far no luck, for now I guess go ahead with the review, if I find a solution for this I will do it in a separate PR

marcalexiei

Performance

Real app

Current version (23.11.4)	With the fix
~120.61s	~129.01s

I think that the performance increase is within an acceptable limit,
probably it can be improved further later.

Many keys test scenario (no context involved)

Current version (23.11.4)	With the fix
~4.3s	~4.3s

Breaking changes

While testing the patch I had to make some changes in the code.

Note

All the following scenarios were working with version 23.11.4

Empty string and falsy values

Those were accepted by the previous version when the context has a default value,
however now I have to set undefined to remove the type error.

// touch is a boolean
- touch && 'touch',
+ touch  ? touch : undefined,

- typeof activeStepIndex === 'number' ? 'step' : ''
+ typeof activeStepIndex === 'number' ? 'step' : undefined

Default context value

When you access a context with a variable and that context has a default value but the i18n resource hasn't mapped all the value a type error is present.
This makes the context default value not useful in this type of scenarios.

Given the following resource structure:

"theme_setting": {
  "value": "system",
  "value|dark": "dark",
  "value|light": "light"
},

an this code

const currentTheme = 'system' as 'dark' | 'light' | 'system';
t('generic:theme_setting.value', { context: currentTheme })

To remove the error I had to remove the default context value and add the missing value (system):

-   "value": "system",
+   "value|system": "system",

Conclusion

These breaking changes make context type-check much more strict than the previous version.

I don't have a solution to make the new strict validation compatible with the old ts API.

Making a release at this point would be a major bump and due to the breaking changes above it might require a migration guide (from a TS validation standpoint).

stefan-schweiger · 2024-05-15T06:31:52Z

Empty string and falsy values

Those were accepted by the previous version when the context has a default value, however now I have to set undefined to remove the type error.
// touch is a boolean
- touch && 'touch',
+ touch  ? touch : undefined,
- typeof activeStepIndex === 'number' ? 'step' : ''
+ typeof activeStepIndex === 'number' ? 'step' : undefined

I think that should be solvable by simply adding false | '' | 0 to the type definition. Tests are still green if I do that, but I might need to add additional tests.

Default context value

When you access a context with a variable and that context has a default value but the i18n resource hasn't mapped all the value a type error is present. This makes the context default value not useful in this type of scenarios.

Given the following resource structure:
"theme_setting": {
  "value": "system",
  "value|dark": "dark",
  "value|light": "light"
},
an this code
const currentTheme = 'system' as 'dark' | 'light' | 'system';
t('generic:theme_setting.value', { context: currentTheme })
To remove the error I had to remove the default context value and add the missing value (system):
-   "value": "system",
+   "value|system": "system",

This might be a bit harder, but let me try to give it a go as well.

stefan-schweiger · 2024-05-15T06:43:22Z

Btw is was recommended to me by members of your organisation before that contexts are for known values and should not use defaults. So for the second one you could maybe even argue it's a feature... but let me know if I still should try to fix the typing for it.

i18next/react-i18next#1730 (comment)

marcalexiei · 2024-05-15T07:44:46Z

I personally don't have any problem changing the code to provide only valid context value,
so support falsy values commit can be reverted also since your last comment:

Btw is was recommended to me by members of your organisation before that contexts are for known values and should not use defaults. So for the second one you could maybe even argue it's a feature... but let me know if I still should try to fix the typing for it.

Probably someone will occur in these errors while upgrading. Up to @adrai to decide.
FWIW this can be merged for me.

stefan-schweiger · 2024-05-15T07:51:42Z

To be honest, I never 100% agreed with the assessment that context is only for known values and you even mention the fallback case in your documentation. In the end whatever you decide is fine by me, but it should be reflected in the documentation as well.

adrai · 2024-05-15T07:56:24Z

Btw is was recommended to me by members of your organisation before that contexts are for known values and should not use defaults. So for the second one you could maybe even argue it's a feature... but let me know if I still should try to fix the typing for it.

i18next/react-i18next#1730 (comment)

I think that was a misunderstanding...
Jan said that "context is more for a known set of option", but that does not mean it can't be used without a known value... at least at runtime this for sure is not a problem.
So this is/was just a TypeScript "problem".

So @marcalexiei and @stefan-schweiger should we merge this PR and release a new version? (patch/minor/major)?

stefan-schweiger · 2024-05-15T08:01:13Z

@adrai in this case there is still the gap mentioned by @marcalexiei

Default context value

When you access a context with a variable and that context has a default value but the i18n resource hasn't mapped all the value a type error is present. This makes the context default value not useful in this type of scenarios.

Given the following resource structure:
"theme_setting": {
  "value": "system",
  "value|dark": "dark",
  "value|light": "light"
},
an this code
const currentTheme = 'system' as 'dark' | 'light' | 'system';
t('generic:theme_setting.value', { context: currentTheme })
To remove the error I had to remove the default context value and add the missing value (system):
-   "value": "system",
+   "value|system": "system",

For t function parameters this is relatively easy to solve, but I'm currently pulling my hair out about the TFunctionReturn type because ActualKey is always defined to include the context which might not exist in the resources. So any help from your team would be appreciated.

marcalexiei · 2024-05-15T10:37:03Z

So @marcalexiei and @stefan-schweiger should we merge this PR and release a new version? (patch/minor/major)?

My main concern that is holding me back from approval is that now the types make context type-checking much more stricter than the JS API.

I understand the need to have a stricter validation on all possible values of a context, but is really necessary if I had a default value for the context?
The more I think about this, the more I think that strict validation should be performed only when there isn't a default value for the context.

Like I told earlier I'm too busy to perform an in depth review at this time.
I can find some time to review PRs if required, like I did here, but not much more.
At least not for the next weeks.

stefan-schweiger · 2024-05-16T13:20:50Z

I also won't be available for the next two weeks starting tomorrow, so let's see were we stand once I'm back..

marcalexiei · 2024-05-16T17:00:30Z

Added a new test for #2182 (review) ("Default context value" section) via #2192

Stefan Schweiger added 2 commits May 10, 2024 18:02

Fix context typing

fa6420d

Remove unneeded type change

a080730

adrai requested a review from marcalexiei May 10, 2024 16:07

adrai added the typescript label May 10, 2024

marcalexiei suggested changes May 12, 2024

View reviewed changes

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

stefan-schweiger and others added 2 commits May 13, 2024 08:53

Merge branch 'i18next:master' into master

9f4f9ca

Add tests for context type checking

8b225ea

improve readability of typing

f82a259

marcalexiei self-requested a review May 13, 2024 09:26

Stefan Schweiger added 2 commits May 13, 2024 16:06

add additional test cases

bb47175

add additional test cases

11d1472

stefan-schweiger commented May 13, 2024

View reviewed changes

marcalexiei suggested changes May 14, 2024

View reviewed changes

Merge branch 'i18next:master' into master

cbbbe81

fix for multiple namespaces

6787fa0

fix typo in test

1abbcf8

marcalexiei self-requested a review May 14, 2024 11:44

Reduce nesting of ContextOfKey

55078b6

marcalexiei suggested changes May 14, 2024

View reviewed changes

stefan-schweiger requested a review from marcalexiei May 14, 2024 12:55

marcalexiei reviewed May 14, 2024

View reviewed changes

Stefan Schweiger added 2 commits May 15, 2024 08:36

support falsy values

c2ba23a

fix error due to formatting

1788111

marcalexiei mentioned this pull request May 16, 2024

types: add test for context with default value #2192

Merged

4 tasks

Fix context type checking #2182

Are you sure you want to change the base?

Fix context type checking #2182

Conversation

stefan-schweiger commented May 10, 2024 • edited

Checklist

Checklist (for documentation change)

coveralls commented May 10, 2024 • edited

marcalexiei left a comment

Choose a reason for hiding this comment

Unit test

Performance

stefan-schweiger commented May 13, 2024

Unit test

Performance

marcalexiei commented May 13, 2024

adrai commented May 13, 2024

stefan-schweiger commented May 13, 2024

stefan-schweiger commented May 13, 2024

marcalexiei commented May 13, 2024 • edited

stefan-schweiger commented May 13, 2024 • edited

stefan-schweiger commented May 13, 2024

marcalexiei commented May 13, 2024

marcalexiei commented May 13, 2024

marcalexiei commented May 13, 2024

marcalexiei commented May 13, 2024

stefan-schweiger commented May 13, 2024

marcalexiei commented May 13, 2024

stefan-schweiger May 13, 2024

Choose a reason for hiding this comment

stefan-schweiger May 13, 2024

Choose a reason for hiding this comment

marcalexiei left a comment • edited

Choose a reason for hiding this comment

stefan-schweiger commented May 14, 2024

stefan-schweiger commented May 14, 2024

marcalexiei left a comment

Choose a reason for hiding this comment

stefan-schweiger commented May 14, 2024 • edited

marcalexiei left a comment • edited

Choose a reason for hiding this comment

Performance

Real app

Many keys test scenario (no context involved)

Breaking changes

Empty string and falsy values

Default context value

Conclusion

stefan-schweiger commented May 15, 2024

Empty string and falsy values

Default context value

stefan-schweiger commented May 15, 2024

marcalexiei commented May 15, 2024

stefan-schweiger commented May 15, 2024

adrai commented May 15, 2024

stefan-schweiger commented May 15, 2024

Default context value

marcalexiei commented May 15, 2024

stefan-schweiger commented May 16, 2024

marcalexiei commented May 16, 2024

stefan-schweiger commented May 10, 2024 •

edited

coveralls commented May 10, 2024 •

edited

marcalexiei commented May 13, 2024 •

edited

stefan-schweiger commented May 13, 2024 •

edited

marcalexiei left a comment •

edited

stefan-schweiger commented May 14, 2024 •

edited

marcalexiei left a comment •

edited