[@react-router/dev/vite] typecheck fails when skipLibCheck: false and moduleResolution: nodenext

[AviVahl]

I'm using React Router as a...

framework

Reproduction

mkdir react-router-dev-type-issue
cd react-router-dev-type-issue
npm init -y
npm i typescript react-router @react-router/dev
npx tsc --init --moduleResolution nodenext --module nodenext --skipLibCheck false
echo "import '@react-router/dev/vite';" > index.ts
npx tsc --noEmit

yields:

node_modules/@react-router/dev/dist/vite.d.ts:5:36 - error TS2503: Cannot find namespace 'Vite'.

5 type ReactRouterVitePlugin = () => Vite.Plugin[];
                                     ~~~~

This also happens when importing @react-router/dev/config', as it uses Vite's resolution options interface as well.

System Info

Irrelevant. Type-issue that happens on all systems.

Used Package Manager

npm

Expected Behavior

Pass with all typescript modern resolution modes, not only 'bundler'.

Actual Behavior

@react-router/dev is not marked as "type": "module", so *.d.ts files are treated as commonjs in this mode.
The import to Vite therefor receives the deprecated cjs version of vite's runtime/types (separate .d.ts that says "any").

[AviVahl]

I could try to create a PR that converts the building of @react-router/dev to output a native esm package, and it would work with all modern (v7) react-router projects out there.

That seems like an infrastructure thingy you would want to handle yourselves, but please let me know.

[timdorr]

I'm not sure why you would want to check our types for validity. We already do that, so it's redundant work. And it significantly slows down type checking in your own project. It's also recommended to have it on: https://www.typescriptlang.org/tsconfig/#skipLibCheck

We currently have some temporary measures in place to support peer deps without issues: #12437 That means our packaging is not quite as simple as add "type": "module" and call it a day. We have the complexity of also getting various packages of mixed types (CJS, ESM, and other fun formats) to resolve to the exact same module so that instance variables are the same (such as createContext()). So, it may not be possible to resolve this right now or even in the near future.

[AviVahl]

Skipping lib check isn't an option for us. It skips typechecking of all .d.ts files, including ones we have in our projects and augment other stuff. We actually resolved some real typescript-behavior-in-IDE issues that were masked by skipping the .d.ts files.

I realize it's not just a matter of adding "type": "module" and calling it a day. Fortunately, I'm pretty well versed in CJS/ESM/TypeScript/Node and all the different combination of tsconfig module/moduleResolution options, so I might be able to figure it out faster than most. The real question is whether you guys want a pure native esm project, or just the @react-router/dev package, or what... I've noticed the project is using wireit/tsup, so it might even be easier than usual.

[pcattori]

@AviVahl could you change your .d.ts files into .ts files (and add an empty export or something to make them true modules)? That way skipLibCheck wouldn't ignore them.

I realize this adds a bit of tedium on your end to do that conversion, but conceptually is there some reason you couldn't use .ts files in place of .d.ts files (or some reason you greatly prefer .d.ts over .ts)?

I commiserate as originally I wanted our typegen to produce .d.ts files (mostly for aesthetic reasons), but then they would get ignored by skipLibCheck, so I changed them to .ts files and added a no-op import (import "react-router") to them so that TS would treat them as proper modules.

The reality for us is that supporting skipLibCheck: false is untenable as any of our dependencies could have random little inconsequential type errors that pop up any time they make a new release.

[pcattori]

Currently, we don't have plans to support skipLibCheck so this is working as intended so I'm closing this issue.

That said, I'm open to hearing reasons why we should reconsider, so feel free to keep commenting on this and we can reopen if we decide to tackle this.

[AviVahl]

I cannot turn all the .d.ts files in our projects to .ts, as some are ambient module declarations. I already have another workaround from my end. It's super ugly, but allows me to keep checking everything and ensuring no unexpected any comes in.

The way I see it, the issue is not about my compilation type-checking the .d.ts in node_modules, it's about node_modules/@react-router/dev/dist/vite.d.ts having an import * as Vite from 'vite'; that resolves to node_modules/vite/index.d.cts in some tsconfig.json configurations (when "module" is set to node16/node18/nodenext). That vite/index.d.cts file contains:

/**
 * @deprecated The CJS build of Vite's Node API is deprecated. See https://vite.dev/guide/troubleshooting.html#vite-cjs-node-api-deprecated for more details.
 */
declare const module: any

export = module

Meaning, under these conditions, the react-router vite plugin essentially resolves to the type () => any[] (instead of () => Vite.Plugin[]).

Skipping the lib check masks the resolution error, but the any type is still the one getting resolved (whether I'm skipping it or not). It will, for example, also get caught by typescript-eslint's no-unsafe-return rule.

Now, there's the question: do you guys want to officially support "module: "node16"/"node18"/"nodenext"?

Under the above modes, module .ts (and .d.ts) files that sit under package.json without a "type": "module" are considered commonjs. Their imports will resolve to "require" export condition rather than the "import" condition.
node_modules/@react-router/dev/dist/vite.d.ts is one such .d.ts file.

[pcattori]

@AviVahl ah ok, I see. I think the issue might be with how our exports conditions for the @remix-run/dev package are setup.

Now that Node v18 is EOL all major Node versions support require(esm) so might be time to ship @remix-run/dev as an ESM package. I think that could also fix the issue.

Now, there's the question: do you guys want to officially support "module: "node16"/"node18"/"nodenext"?

Yes, we strive to support NodeNext though its a shifting landscape since NodeNext can change at anytime. So far so good, but I can't rule out that NodeNext might change in the future in a way that would make some of the things we do untenable.