ts error when moduleResolution is "node16"

dislido commented 1 year ago

Describe the bug

I am migrating my node application from cjs to mjs. When I set "moduleResolution": "node16" in tsconfig.json, the vite.config.ts reports This expression is not callable. Type 'typeof import("/node_modules/@vitejs/plugin-react/dist/index")' has no call signatures.ts(2349). I tried to change reactPlugin() to reactPlugin.default(), the error disappears but vite build failed with error TypeError: reactPlugin.default is not a function

Reproduction

https://stackblitz.com/edit/vitejs-vite-4x7wea?file=tsconfig.json,vite.config.ts,tsconfig.node.json&terminal=dev

Steps to reproduce

set compilerOptions.moduleResolution = 'node16' in tsconfig.json

System Info

System:
    OS: macOS 13.0.1
    CPU: (8) arm64 Apple M1 Pro
    Memory: 44.64 MB / 16.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.14.0 - ~/.nvm/versions/node/v16.14.0/bin/node
    Yarn: 1.22.17 - ~/.nvm/versions/node/v16.14.0/bin/yarn
    npm: 8.19.2 - ~/.nvm/versions/node/v16.14.0/bin/npm
  Browsers:
    Chrome: 109.0.5414.119
    Firefox: 108.0.2
    Safari: 16.1
  npmPackages:
    @vitejs/plugin-react: ^3.1.0 => 3.1.0 
    vite: ^4.1.1 => 4.1.1

Used Package Manager

npm

Logs

No response

Validations

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] Make sure this is a Vite issue and not a framework-specific issue.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

justinwaite commented 1 year ago

I see you closed this, is there a resolution for this?

dislido commented 1 year ago

I see you closed this, is there a resolution for this?

The same problem occurs when I use other plugins. I closed this issue because I'm not sure if this is my project configuration problem. Now I temporarily use//@ ts-expect-error to hide the error.

I noticed that if there is a named export, this problem can be circumvented

//@vitejs/plugin-react/dist/index.d.ts

// export { BabelOptions, Options, ReactBabelOptions, viteReact as default };
export { BabelOptions, Options, ReactBabelOptions, viteReact, viteReact as default };

// import reactPlugin from '@vitejs/plugin-react';
import {viteReact} from '@vitejs/plugin-react';

It seems that I should keep this issue open

ArnaudBarre commented 1 year ago

Hi, I may look into this at one point, but I'm just really annoyed by what ts TS team has done with node16 resolution algorithm. TS 5 bundler resolution algorithm will solve this mess hopefully

rgripper commented 1 year ago

Same error for me running vite+react on Deno 1.13.3 with TS 4.9.

robwierzbowski commented 1 year ago

I had a similar issue and I believe @Sec-ant has found the issue here: https://github.com/pmndrs/zustand/issues/1381#issuecomment-1290296210

Quick summary: In node16 or nodenext module resolutions, the t.ds file must use a t.mds extension to be discovered by the module resolver.

I'd be happy to roll a PR to add a t.mds file and point to it in the package.json, if the maintainers think that won't cause any additional issues.

karlhorky commented 1 year ago

More details of the broken types for @vitejs/plugin-react over here at Are the types wrong? by @andrewbranch:

Imports of the package under the node16 module resolution setting when the importing module is ESM (its extension is .mts or .mjs, or it has a .ts or .js extension and is in scope of a package.json that contains "type": "module") resolved to CJS types, but ESM implementations.

Screenshot 2023-03-25 at 18 26 49

karlhorky commented 1 year ago

For me what fixed it was creating a patch using pnpm patch (or patch-package for Yarn / npm) with the following changes:

Edit package.json to remove the "types" line inside "exports" -> "."
Copy dist/index.d.ts to dist/index.d.mts

See example commit here: https://github.com/karlhorky/archive-webpage-browser-extension/commit/ad3a5e65b38c389a6e46d0c0d557c76d06376698

ArnaudBarre commented 1 year ago

Hi!

With the team we decided to not invest more time in the node16 resolutions issues. This IMO a bad design and this as been fixed with TS 5 bundler mode that more clearly represent how most apps and projects are built. If you're not able to migrate, we recommend keeping the old "node" resolution algorithm.

karlhorky commented 1 year ago

Just to voice the sentiment that will be certainly coming also from others: this is an unfortunate decision by your team, and is also a bit user-hostile, for those users who need to use some module resolution format for some reason.

Probably the changes to the packages amount to changes to a handful of lines of code, and then you will have proper, non-broken interop with TypeScript 4.7+.

Otherwise you are putting this burden on all of your consumers - either to maintain patches for your broken types or to somehow special-case their environments for Vite plugins.

I would strongly suggest bringing this back to the team and reconsidering this decision.

ArnaudBarre commented 1 year ago

What is the motivation for using node16 instead of bundler?

robwierzbowski commented 1 year ago

@ArnaudBarre Thanks for the message — I understand if supporting all module resolutions isn’t as important as other features, especially if TS 5 also solves the problem.

For motivation — I would like my runtime and TS check to use the same module resolution rules, to align on one way of managing modules and file names. This reduces the number of concepts people need to understand when working on and maintaining the project.

I spent some time debugging why my TS build was failing with nodenext and node16. What do you think about adding a note in the readme that these module resolution types are not supported, to help others understand that the issue is not something in their local code?

You mentioned “if you’re not able to migrate…”. Is there an upgrade path to TS 5 with Vite currently?

ArnaudBarre commented 1 year ago

This is exactly why the TS team added the bundler resolution, so that TS resolutions is more close (and maybe totally identical when using pure esm with explicit extensions) to how bundler like Vite resolve modules.

So yeah you can now update TS to v5, and then update your TS config to use this new resolution, opt-in to allowImportingTsExtensions, remove forceConsistentCasingInFileNames that is now true by default.

Here is my config:

    "target": "ESNext",
    "lib": ["DOM", "DOM.Iterable", "ESNext"],
    "module": "ESNext",
    "skipLibCheck": true,

    /* Bundler mode */
    "moduleResolution": "bundler",
    "allowImportingTsExtensions": true,
    "resolveJsonModule": true,
    "isolatedModules": true,
    "noEmit": true,
    "jsx": "react-jsx",

    /* Linting */
    "strict": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true,
    "noPropertyAccessFromIndexSignature": true,
    "noFallthroughCasesInSwitch": true

I will add a note in the readme to advice using this resolution algorithm.

robwierzbowski commented 1 year ago

Thank you! Fantastic support @ArnaudBarre, appreciate it.

karlhorky commented 1 year ago

There are other reasons to support Node16 and the other module resolution options. Interop is important for a variety of reasons which I don't have the resources on hand currently for - there is a lot of literature (and horror stories) about tooling not being interoperable with some module system.

From what I remember why we chose Node16 (and use it in all of our projects), it reduces the ambiguity of the module resolution algorithm must go through. Instead of searching through multiple files, there is only one file that it could possibly look for. This also positively affects performance, if I remember correctly.

But I'm not the best one to talk about the benefits here - I'm sure there is more that I'm leaving out.

I'm just mentioning that Vite and related packages should try to be interoperable with the module systems and ecosystem. It should not have errors on Are the types wrong?. To be against interop like this makes me lose confidence in the robustness and maintenance of Vite and its ecosystem.

ArnaudBarre commented 1 year ago

For the ambiguity and performance point, bundler mode + allowImportingTsExtensions is way better.

Vite goal is to help the ecosystem use ESM tools. We also don't support tools or libraries that do crazy things with cjs requires that webpack is able to support.

andrewbranch commented 1 year ago

👋 Hello from the TS team. There seems to be some confusion here about when it’s appropriate to use different moduleResolution settings. What we’re talking about here is a dependency (@vitejs/plugin-react) that gets loaded from a vite.config.js file by Node. While I’m pleased to see the enthusiasm for --moduleResolution bundler, users can’t just pick the resolution mode they like better and check with that, and library authors can’t reasonably refuse to make their types compatible with resolution modes that reflect runtimes where their runtime code is compatible. A user’s decision on what mode to use should be based on the facts of what module resolver is going to be used at runtime. And for a vite.config.js file, the answer is not a bundler—it’s Node¹. Consequently, the node16/nodenext modes are most appropriate.

Libraries have been explicitly discouraged from relying on --moduleResolution bundler:

On the other hand, if you’re writing a library that’s meant to be published on npm, using the bundler option can hide compatibility issues that may arise for your users who aren’t using a bundler. So in these cases, using the node16 or nodenext resolution options is likely to be a better path.

The configuration published now is incorrect, full stop. Any library that can be successfully imported by modern versions of Node must be compatible with --moduleResolution nodenext to claim TypeScript compatibility.

I'm just really annoyed by what ts TS team has done with node16 resolution algorithm. TS 5 bundler resolution algorithm will solve this mess hopefully

It’s understandable and correct to be annoyed by trying to use node16 to check bundled app code, and that is indeed what the introduction of bundler was for! But hopefully it’s becoming clear from my answer that node16 is not our design—it reflects the realities of, well, Node 16, which is much stricter than most bundlers these days. But the code you have here is not intended to be loaded by a bundler; it’s intended to be loaded by Node. And moreover, as a popular library with types, you have a responsibility not to impose arbitrary TypeScript configuration restrictions on your users. This situation is analogous to shipping types that only compile with strictNullChecks: false: it may compile in your config, but your users may have good reason to compile under stricter settings, so as a library, it’s best practice to ensure you’re compatible with the strictest settings possible. --moduleResolution nodenext is the --strict of module resolution, and should be the baseline target for all Node-compatible libraries.

¹ The answer is somewhat complicated by the fact that Vite instruments Node with a custom ESM loader that makes module resolution more forgiving—I’m guessing @esbuild-kit/esm-loader. The presence of this loader does make --moduleResolution bundler an ok choice for processing vite.config.js files, provided they are written in ESM. But it should not be a requirement that users check their config files with this mode, particularly since it’s an implementation detail that Vite is overriding the resolution under the hood. Users should be able to make the reasonable assumption that --moduleResolution nodenext is the right mode to use for a file running in Node, and indeed, that mode will not lead them to write code that will fail at runtime.

ArnaudBarre commented 1 year ago

As your note shows, the reality is a lot more complex than "the user config is running in node". Most of people will use a TS config nowadays, and this file is clearly bundled and transformed before being executed.

From what I remember (and what annoys me, correct me if I'm wrong), the node16 resolution forces you to import ./utils.ts as utils.js a file that will never exist... This why for me node16 is only making sense when compiling files one by one with the TS compiler, a thing that esbuild is making less common and is clearly not the reality of Vite config.

andrewbranch commented 1 year ago

node16 is compatible with the new allowImportingTsExtensions, and that combo is what you’d want to use if @esbuild-kit/esm-loader did not arbitrarily override Node’s module resolution algorithm the way it does. (I really wish someone would make such an esbuild loader that only transpiles without affecting resolution.) But this is orthogonal to @vitejs/plugin-react knowingly shipping incorrect typing configurations. The truth is that the plugin can be imported by vanilla Node, and therefore should ship typings that reflect that reality. Particularly when community members like @karlhorky have already done the hard work of figuring out how to fix them 🙂

ArnaudBarre commented 1 year ago

node16 is compatible with the new allowImportingTsExtensions

That good news, I missed that

I really wish someone would make such an esbuild loader that only transpiles without affecting resolution.

Yep clearly something that would be needed once node loaders become stable. The fact TS imports are not ban will really make this simple to write.

the plugin can be imported by vanilla Node

This true but not clearly not an expected usage.

figuring out how to fix them

Removing entries from the export map feels like going backward. Would this not break bundler resolution ?

andrewbranch commented 1 year ago

Removing entries from the export map feels like going backward. Would this not break bundler resolution ?

Every TypeScript resolution mode supports extension matching to sibling files. If every index.mjs has a sibling index.d.mts and every index.cjs has a sibling index.d.cts and every index.js has a sibling index.d.ts, then nothing in package.json needs to explicitly reference the declaration files. TypeScript can always figure it out by following the implementation file config and then looking for a sibling.

If the types are not colocated with the implementation, or you just want to be explicit in the export map, you can use nested conditions:

"exports": {
  "import": {
    "types": "./types/index.d.mts",
    "default": "./esm/index.mjs"
  },
  "require": {
    "types": "./types/index.d.cts",
    "default": "./cjs/index.cjs"
  }
}

Just note that this does not make it possible to reuse a single .d.ts for both the CJS and ESM entrypoint—the file extension and package.json "type": "module" (or lack thereof) is the only thing that determines the module kind here, so a nested config like

"exports": {
  "import": {
    "types": "./types/index.d.ts", // <-- .d.ts
    "default": "./esm/index.mjs"
  },
  "require": {
    "types": "./types/index.d.ts", // <-- .d.ts
    "default": "./cjs/index.cjs"
  }
}

is equivalent to what is published today. In other words, just because you reach a file via the import condition in an export map during a given resolution does not automatically make it an ES module; rather, a file’s module kind status is statically determined by information from the file system alone (its file extension and package.json "type"). And again, let me stress that these are not our rules, they’re Node’s. We just apply Node’s algorithm to declaration files such that when each implementation file has a corresponding declaration file, we can predict what Node will do (which is obviously necessary to type check across multiple files). Don’t shoot the messenger 😅

vitejs / vite-plugin-react