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

fix: Use Typescript 5 types with Typescript 5+ #2108

Merged
merged 1 commit into from Jan 4, 2024

Conversation

Gr3q
Copy link
Contributor

@Gr3q Gr3q commented Jan 4, 2024

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

Tests are still passing because the v5 types are also valid. I'm not sure how should I write a test to check if the type file used matches the typescript version.

@coveralls
Copy link

Coverage Status

coverage: 92.045%. remained the same
when pulling 442a7c2 on Gr3q:fix-typescript-type-selection
into aa74c03 on i18next:master.

@adrai
Copy link
Member

adrai commented Jan 4, 2024

Why is this change necessary?
Do you have an example repo that shows an issue?
the types value was kept for v4 usage... v5 should use the exports types, right?

@Gr3q
Copy link
Contributor Author

Gr3q commented Jan 4, 2024

It seems it doesn't.

I can create an example repo, but, if you do this in a folder:

npm init
npm i typescript
npm i i18next

your index.ts file:

import { ParseKeys } from "i18next";
  1. Open the folder in VSCode or anything that has "Go to Definition" function
  2. in the index.ts "Go to Definition" for ParseKeys
  3. If it goes to t.v4.d.ts it failed.

Edit: Fine I'll give you an example repo

@adrai
Copy link
Member

adrai commented Jan 4, 2024

That's probably because of the moduleResolution in the tsconfig...

@adrai
Copy link
Member

adrai commented Jan 4, 2024

I'm not a TS expert... what do you think @marcalexiei ?

@Gr3q
Copy link
Contributor Author

Gr3q commented Jan 4, 2024

  1. Clone this repo https://github.com/Gr3q/i18next-incorrect-vtype-selection
  2. npm ci
  3. npx tsc --listFilesOnly

if you have the v4 types it's a problem

@adrai
Copy link
Member

adrai commented Jan 4, 2024

like assumed... setting the moduleResolution to NodeNext, uses v5 types...
The question is which group of people do you want to make angry ;-)

@adrai
Copy link
Member

adrai commented Jan 4, 2024

Those who want TS v4 or v5?

@adrai
Copy link
Member

adrai commented Jan 4, 2024

I have a proposal: We change that to prefer TS v5...
if there are new gh issues regarding that change, you can discuss with them, ok?

@Gr3q
Copy link
Contributor Author

Gr3q commented Jan 4, 2024

The map of used types

Without this change

ModuleResolution: Node

  • Typescript 4 - index.v4.d.ts
  • Typescript 5 - index.v4.d.ts

ModuleResolution: Node16 - recommended for Typescript 5

  • Typescript 4 - doesn't work (as expected)
  • Typescript 5 - index.d.ts

With this change

ModuleResolution: Node

  • Typescript 4 - index.v4.d.ts
  • Typescript 5 - index.d.ts

ModuleResolution: Node16 - recommended for Typescript 5

  • Typescript 4 - doesn't work (as expected)
  • Typescript 5 - index.d.ts

Which part of this is a problem? Or maybe I'm missing something?

Edit: I did these checks on the example repo changing the module resolution and installing different versions of typescript, then run npx tsc --listFilesOnly

@adrai
Copy link
Member

adrai commented Jan 4, 2024

If I test in the examples/typescript project with your change I get:

ModuleResolution: Node

  • Typescript 4 - index.d.ts
  • Typescript 5 - index.d.ts

but only via navigation in the editor

@Gr3q
Copy link
Contributor Author

Gr3q commented Jan 4, 2024

I'll investigate why the editor (and probably a build) differs from what tsc reports as included files then

@marcalexiei
Copy link
Member

I'm not a TS expert... what do you think @marcalexiei ?

In our app we use "moduleResolution": "bundler" so we haven't had an issue with "types": "./index.v4.d.ts" since with that moduleResolution ts prefers exports property.


Now that you point out that types was referring to index.v4.d.ts I found it a little strange since there is a override in place inside typesVersion.

Here there are a few considerations:

  1. types inside package json can be considered a legacy property since all the ecosystem is now moving to ESM and using exports, so from my point of view if someone is using ts TS 5 should also have a "recent" moduleResolution (like bundler) to get rid of this issue since it will use exports properties.
  2. At some point TS 4 support will be dropped and I hope to get rid of all v4 files and the related content... but for now I understand that some people might have broken types if v5 definitions are exposed from package.json and they are using TS 4 like @adrai pointed out.

Conclusions

The only compromise I see is that:

  • TS v5 users try to use a more recent moduleResolution.
  • TS v4 users take account to the fact that sometime the ts v4 will be dropped.

Clearly there isn't a right solution that fits all, so this is the best compromise I could think of.

@Gr3q
Copy link
Contributor Author

Gr3q commented Jan 4, 2024

Then I'm fine as long as the documentation clearly states which combinations of ModuleResolution+Typescript+i18next versions the types are incorrect (well...older) for.

@adrai
Copy link
Member

adrai commented Jan 4, 2024

I just verified... @Gr3q is right... with tsc the correct types are used... only the editor seems to navigate to the wrong definition... but imo this is ok...
So we can merge this.

@adrai adrai merged commit 941178c into i18next:master Jan 4, 2024
10 checks passed
@adrai
Copy link
Member

adrai commented Jan 4, 2024

will wait for another pr to be merged, and then I'll create a new version

@adrai
Copy link
Member

adrai commented Jan 4, 2024

@Gr3q v23.7.16 has just been released

@Gr3q Gr3q deleted the fix-typescript-type-selection branch January 4, 2024 11:33
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

4 participants