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

Restrict interpolation types based on format; Include typing for formatParams #1999

Closed

Conversation

nicegamer7
Copy link

Currently, it's possible to pass any variable as an interpolation, which isn't great since it can be error prone. Take the following as an example:

// Poorly named variable
const username = { name: 'some_user', ... };

// ...

// Assuming key fallback
t("Hello {{username}}", { username });
// -> "Hello [object Object]"

The above is not completely addressed in this PR. Instead, I'm first fixing a subset of this problem, which is formatted interpolations. Since the built-in interpolations must conform to the Intl formatters that they are passed to, this PR makes it so that any interpolation that is being formatted is restriced to the type that the corresponding Intl formatter expects. This PR also adds typing for formatParams.

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: 92.36%. remained the same when pulling 666d724 on nicegamer7:feature/formatparams-types into 815cd83 on i18next:master.

@stale
Copy link

stale bot commented Aug 7, 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 Aug 7, 2023
@adrai adrai removed the stale label Aug 7, 2023
@adrai
Copy link
Member

adrai commented Sep 7, 2023

@pedrodurek and also here?

@stale
Copy link

stale bot commented Sep 17, 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 Sep 17, 2023
@adrai adrai removed the stale label Sep 17, 2023
@stale
Copy link

stale bot commented Oct 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 Oct 15, 2023
@adrai adrai removed the stale label Oct 15, 2023
@adrai
Copy link
Member

adrai commented Nov 28, 2023

I'm not sure we should enforce the format typings... what if someone uses the old formatting or custom formats?

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
@adrai
Copy link
Member

adrai commented Feb 17, 2024

@marcalexiei what is your opinion on this?

@stale stale bot removed the stale label Feb 17, 2024
Copy link
Member

@marcalexiei marcalexiei left a comment

Choose a reason for hiding this comment

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

I tried to port the on master branch.
(This PR is very old: InterpolationMap is inside t.d.ts files, here is inside index.d.ts 😅)

Right now tests are failing:

test errors image

The possible breaking change is that someone could have already declared formatter with these names but with different parameters like @adrai pointed out.

Simple solution

This computation should be performed only when _JSONFormat is set to v4 like in PluralSuffix.

type PluralSuffix = _JSONFormat extends 'v4'
? 'zero' | 'one' | 'two' | 'few' | 'many' | 'other'
: number | 'plural';

However if someone is using a custom parser with JSONFormat set to v4 the issue will remain, so the only workaround I could think of is to add a new boolean flag under CustomTypeOptions to enable this kind of behaviour.

I personally don't find any of this two solutions quite appealing because I consider them a workaround.

Complex solution

The best, but more difficult, solution is to add a way to define formatters inside CustomTypeOptions so every user is able to customise them and they are inferred by the TFunction type when the key has them.
Basically something like what is already in place with Resources.

Unfortunately that is a lot of type gymnastic.


Besides that I think that more tests should be added to test the behaviour of TFunction when providing a formatter using Intl API.

@adrai
Copy link
Member

adrai commented Feb 17, 2024

ok, will close this for now... if someone has time and motivation to take this over feel free to reopen or create a new PR

@adrai adrai closed this Feb 17, 2024
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.

None yet

5 participants