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

Broken types using node16+ moduleResolution strategy with ESM #2010

Closed
me4502 opened this issue Aug 3, 2023 · 8 comments
Closed

Broken types using node16+ moduleResolution strategy with ESM #2010

me4502 opened this issue Aug 3, 2023 · 8 comments
Assignees

Comments

@me4502
Copy link

me4502 commented Aug 3, 2023

馃悰 Bug Report

Using TypeScript with moduleResolution set to node16 or nodenext, module set to any ESM module (such as esnext), and "type": "module" within the package.json, TypeScript is unable to correctly determine types for i18next imports due to it inferring the provided index.d.ts file as CJS when the underlying import is ESM, due to the single index.d.ts file being used for both ESM & CJS.

This is further explained on this page, https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md

To Reproduce

Reproduction steps:

  • Using TypeScript with a tsconfig containing "moduleResolution": "node16", "module": "esnext", and a package.json containing "type": "module", the following code produces type errors
import i18next from 'i18next';

// Property 'language' does not exist on type 'typeof import("/path/to/node_modules/i18next/index")'.
console.log(i18next.language);

Expected behavior

There should be an index.d.ts file per entrypoint, so that it's correctly inferring CJS or ESM based on the types, and the types therefore work.

import i18next from 'i18next';

// The `language` property is correctly seen as a string by TypeScript
console.log(i18next.language);

Your Environment

  • runtime version: Node 18.17.0
  • i18next version: 23.4.1
  • os: Mac
  • any other relevant information: TypeScript version 5.1.6
@adrai
Copy link
Member

adrai commented Aug 3, 2023

Is this really true?
So how should this be addressed?

@me4502
Copy link
Author

me4502 commented Aug 3, 2023

The cause is that the index.d.ts file has a corresponding index.js file alongside it, which is CJS. So that type definition is assumed to be CJS by TypeScript. The fix would be to have a type definition generated for both the ESM & CJS files, and use the "exports" entry in the package.json to point to the correct one depending on CJS/ESM, or just omitting them and allowing typescript to find a .d.ts for the CJS/ESM files given. The "arethetypeswrong" website has a lot of information about these kinds of issues, https://arethetypeswrong.github.io/?p=i18next%4023.4.1 (output for this library)

There are a few examples of how to fix this on their page for the issue, https://github.com/arethetypeswrong/arethetypeswrong.github.io/blob/main/docs/problems/FalseCJS.md#common-causes

For this library, the likely simplest solution would be to have an i18next.d.ts file in both the ESM & CJS folders, and the "types" entry from the "exports" removed allowing TypeScript to find those files

@adrai
Copy link
Member

adrai commented Aug 3, 2023

Would you like to try a PR?

@me4502
Copy link
Author

me4502 commented Aug 3, 2023

I'm happy to give a PR a go, this issue also affects the other i18next libraries so I'll do this one first and follow with others

@JounQin
Copy link
Contributor

JounQin commented Aug 3, 2023

The is no type: module defined in i18next's package.json, so i18next/index.d.ts is recognized as commonjs by TypeScript when using type: module + Node16

If you want to support ESM + CommonJS + TypeScript all configs, it would be a bit complex. Types for ESM and CommonJS must be separately like:

{
  "exports": {
    ".": {
      "import": {
        // a `package.json` with `type: "module"` is also required
        "types": "./dist/esm/index.d.ts",
        "default": "./dist/esm/index.js"
      },
      "require": {
        "types": "./dist/cjs/index.d.ts",
        "default": "./dist/cjs/index.js"
      }
    }
  }
}

See also https:github.com/sheremet-va/dual-packaging#using-tsc

JounQin added a commit to JounQin/i18next that referenced this issue Aug 3, 2023
@JounQin
Copy link
Contributor

JounQin commented Aug 3, 2023

See #2012

I've tested on my local machine

image image

It should both be working at the same time now.

@stale
Copy link

stale bot commented Aug 12, 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.

@adrai adrai added stale and removed stale labels Aug 12, 2023
@stale stale bot added the stale label Aug 12, 2023
@stale stale bot closed this as completed Sep 17, 2023
@adrai adrai removed the stale label Sep 17, 2023
@adrai adrai reopened this Sep 17, 2023
JounQin added a commit to JounQin/i18next that referenced this issue Oct 10, 2023
@adrai adrai closed this as completed in 78cc3bd Nov 10, 2023
@adrai
Copy link
Member

adrai commented Nov 10, 2023

please try with v23.7.3

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.

4 participants