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

types(Trans): add typechecking on context prop #1732

Merged
merged 1 commit into from Mar 7, 2024

Conversation

marcalexiei
Copy link
Member

fixes #1730.

This is technically a breaking change since it change Trans and TransProps type parameters, however I don't think usually people explicit them when using the component... but who knows 🤷.


I tried to replicate a type parameter structure similar to the TFunction,
however, looks like that using a single TOpt type parameter doesn't pass the property values (like context) down to TransProps properly so ParseKeys is not aware of the context value.

The only alternative is to add a new type parameter and pass it down the chain, which is basically what @stefan-schweiger proposed in the above linked issue (Thanks 🙏).


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

@coveralls
Copy link

Coverage Status

coverage: 97.017%. remained the same
when pulling 29f0690 on marcalexiei:feature/trans-types
into dc9b7bb on i18next:master.

@adrai
Copy link
Member

adrai commented Mar 7, 2024

@stefan-schweiger can you review this and check if it's ok for you?

@stefan-schweiger
Copy link

Already did a short look when I saw the PR and from my side every thing seems in order

@adrai
Copy link
Member

adrai commented Mar 7, 2024

ok, will merge and release in the next hour(s)

@adrai adrai merged commit f5e94c5 into i18next:master Mar 7, 2024
9 checks passed
@adrai
Copy link
Member

adrai commented Mar 7, 2024

thank you both... it's included in v14.1.0

KPrefix = undefined,
TContext extends string | undefined = undefined,
TOpt extends TOptions & { context?: TContext } = { context: TContext },
Copy link

@natoehv natoehv Mar 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was closed, but if we remove the { context: TContext } it works much better, like:
TOpt extends TOptions & { context?: TContext } in this way the IDE TS autocomplete works:
without context assignation:
image
with context assignation:
image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it still correctly handle the typings for the context property of the Trans component?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so
image

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not exactly what I meant :D I meant more something like this:

image image

@marcalexiei marcalexiei deleted the feature/trans-types branch May 4, 2024 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

context values resolved differently (and incorrectly) between <Trans /> and t(...)
5 participants