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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

TFunction casts no longer work - remove 4.1 typescript workaround? #1811

Closed
rosskevin opened this issue Jul 30, 2022 · 11 comments 路 Fixed by #1775
Closed

TFunction casts no longer work - remove 4.1 typescript workaround? #1811

rosskevin opened this issue Jul 30, 2022 · 11 comments 路 Fixed by #1775

Comments

@rosskevin
Copy link
Collaborator

rosskevin commented Jul 30, 2022

馃悰 Bug Report

A workaround was introduced in i18next/react-i18next#1227 by pedrodurek
due to bug i18next/react-i18next#1222 reported by @jigsawye.

The underlying issue microsoft/TypeScript#41406 has been fixed.

I noticed because we were doing some very latent updates and when using react-i18next (not i18next directly), casting no longer typechecks as is tested in https://github.com/i18next/i18next/blob/master/test/typescript/t.test.ts#L16-L22

To Reproduce

function returnCasts(t: TFunction) {
  const s: string = t('friend'); // same as <string>
  const s2: string = t`friend`;
  const o: object = t<object>('friend');
  const sa: string[] = t<string[]>('friend');
  const oa: object[] = t<object[]>('friend');
}

Expected behavior

The same code tested in i18next should work in react-i18next.

I believe this can be satisfied by removing the additional types for 4.1, but I haven't been keeping up with all the changes, so I could be wrong.

Your Environment

  • runtime version: browser
  • react-i18next version: 11.18.3
  • i18next version: 21.8.16
  • os: Mac
  • typescript: 4.7.4
@adrai
Copy link
Member

adrai commented Jul 30, 2022

@pedrodurek you need to decide here

@rosskevin
Copy link
Collaborator Author

FWIW - I substituted the root types into the 4.1 types file and my local build succeeded. I know that is anecdotal, but I think it is safe to say that it is broken with the workaround at this point - I'm running into additional errors in the codebase I am updating. I'd prefer we revert the workaround then go from there.

I'm not a committer any more so I'll defer to @adrai

@adrai
Copy link
Member

adrai commented Aug 2, 2022

@pedrodurek ?

@pedrodurek
Copy link
Member

Hey @rosskevin, actually, they didn't fully fix it. They simply increased the number of instances before throwing the error. I'll remove it for now, and let's see how it goes.

@pedrodurek
Copy link
Member

Hey @rosskevin, could clarify what you are trying to achieve?
In 4.1 types, we changed the order of the generic to assign a default value for the key (string) for people who opt out of using type augmentation to infer the keys and return type automatically.
For your example, it should look like this:

function returnCasts(t: TFunction) {
    const s: string = t("friend"); // same as <string>
    const s2: string = t`friend`;
    const o: object = t<string, object>("friend");
    const sa: string[] = t<string, string[]>("friend");
    const oa: object[] = t<string, object[]>("friend");
  }

@rosskevin
Copy link
Collaborator Author

Wow, I just realized I filed this in the wrong repo - this should be filed in react-i18next, not here, but it is the same cast of characters, my apologies though for this oversight.

@pedrodurek I suggest it is a mistake to make this change only in react-i18next, and it should be done in i18next - so that both libraries use the same TFunction. We have some light enhancements on i18next that are used in our in-house libraries, both node-based and react-based. Since there was a change to only react-i18next so it no longer uses TFunction, and our libraries depend on the shared use of TFunction (again, both node and react), it broke.

So perhaps propagate the breaking change/improvement to i18next and revert react-i18next to import and use TFunction again (and fix the i18next tests to allow for this new casting code)?

I don't mind a breaking change, but I do care that the raw code that works in i18next no longer works with react-i18next.

Does that make sense?

@pedrodurek
Copy link
Member

Hey @rosskevin, yes it does, I'm actually moving all types to i18next.

@stale

This comment was marked as outdated.

@stale stale bot added the stale label Sep 20, 2022
@rosskevin

This comment was marked as outdated.

@stale stale bot removed the stale label Sep 21, 2022
@stale

This comment was marked as outdated.

@stale stale bot added the stale label Oct 1, 2022
@rosskevin rosskevin removed the stale label Oct 2, 2022
@stale

This comment was marked as outdated.

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

Successfully merging a pull request may close this issue.

3 participants