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

RR7 typegen doesn't support moduleResolution node16 #12424

Closed
octet-stream opened this issue Nov 30, 2024 · 14 comments
Closed

RR7 typegen doesn't support moduleResolution node16 #12424

octet-stream opened this issue Nov 30, 2024 · 14 comments
Assignees
Labels

Comments

@octet-stream
Copy link

I'm using React Router as a...

framework

Reproduction

https://stackblitz.com/edit/rr7-typegen-node16-support-issue?file=app%2Froutes%2Fhello.%24name.tsx

Just open the app/routes/hello.$name.tsx file and look at the loaderData type. You'll see it is correct. Then open tsconfig.json and set moduleResolution to node16, and then return to previous file. Now loaderData is undefined.

System Info

System:
    OS: macOS 15.1.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 187.33 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 23.1.0 - ~/.local/state/fnm_multishells/42293_1732447663469/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 10.9.0 - ~/.local/state/fnm_multishells/42293_1732447663469/bin/npm
    pnpm: 9.14.1 - /opt/homebrew/bin/pnpm
    bun: 1.1.34 - /run/current-system/sw/bin/bun
  Browsers:
    Chrome: 131.0.6778.86
    Safari: 18.1.1
  npmPackages:
    @react-router/dev: ^7.0.1 => 7.0.1
    @react-router/fs-routes: 7.0.1 => 7.0.1
    @react-router/node: ^7.0.1 => 7.0.1
    @react-router/serve: ^7.0.1 => 7.0.1
    react-router: ^7.0.1 => 7.0.1
    vite: ^6.0.1 => 6.0.1

Used Package Manager

pnpm

Expected Behavior

loaderData has correct types when moduleResolution is set to node16. In this case - it should be a string.

This might not be an issue for the most (I think), but for me - it is, because this is how Node.js resolves ES modules by default and this might be a problem when I have a code that is shared between Vite and Node.js, so I expect this to work.

Actual Behavior

loaderData is of type undefined when moduleResolution is set to node16

This happens because TS expects files to have a .js at the end of module specifier, same way as Node.js does. From what I can tell, the paths generated by RR's Vite plugin for parent routes are incorrect. This code always outputs path without extension:

type Module = typeof import("../${Pathe.filename(route.file)}")

So, I think to solve this problem, it should respect the moduleResolution option from tsconfig.json (and allowImportingTsExtensions to output .ts at the end of the specifier, if enabled).

@timdorr
Copy link
Member

timdorr commented Nov 30, 2024

We require Node 20 or higher, so this behavior is expected. However, we should ensure allowImportingTsExtensions and the moduleResolution are set to the right values as well.

@octet-stream
Copy link
Author

octet-stream commented Nov 30, 2024

Hey, thanks for quick response. :)

We require Node 20 or higher, so this behavior is expected.

I don't think I understand this part. What's with the Node.js version? This same behavior appears both with the Node version from Stackblitz and the one specified in the issue (it is the same version from my test project with RR7).

@gustavopch
Copy link

I had the same problem, except I'm using "moduleResolution": "nodenext". The solution is indeed to add .js to the imports in the generated files. Here's the patch:

diff --git a/node_modules/@react-router/dev/dist/cli/index.js b/node_modules/@react-router/dev/dist/cli/index.js
index 266c57f..660e362 100644
--- a/node_modules/@react-router/dev/dist/cli/index.js
+++ b/node_modules/@react-router/dev/dist/cli/index.js
@@ -600,7 +600,7 @@ function generate(ctx, route) {
     const indent = i === 0 ? "" : "  ".repeat(2);
     let source = noExtension(rel);
     if (!source.startsWith("../")) source = "./" + source;
-    return `${indent}import type { Info as Parent${i} } from "${source}"`;
+    return `${indent}import type { Info as Parent${i} } from "${source}.js"`;
   }).join("\n");
   return import_dedent.default`
     // React Router generated types for route:
@@ -610,7 +610,7 @@ function generate(ctx, route) {
 
     ${parentTypeImports}
 
-    type Module = typeof import("../${Pathe2.filename(route.file)}")
+    type Module = typeof import("../${Pathe2.filename(route.file)}.js")
 
     export type Info = {
       parents: [${parents.map((_, i) => `Parent${i}`).join(", ")}],
diff --git a/node_modules/@react-router/dev/dist/vite.js b/node_modules/@react-router/dev/dist/vite.js
index f17d1b6..abff485 100644
--- a/node_modules/@react-router/dev/dist/vite.js
+++ b/node_modules/@react-router/dev/dist/vite.js
@@ -641,7 +641,7 @@ function generate(ctx, route) {
     const indent = i === 0 ? "" : "  ".repeat(2);
     let source = noExtension(rel);
     if (!source.startsWith("../")) source = "./" + source;
-    return `${indent}import type { Info as Parent${i} } from "${source}"`;
+    return `${indent}import type { Info as Parent${i} } from "${source}.js"`;
   }).join("\n");
   return import_dedent.default`
     // React Router generated types for route:
@@ -651,7 +651,7 @@ function generate(ctx, route) {
 
     ${parentTypeImports}
 
-    type Module = typeof import("../${Pathe2.filename(route.file)}")
+    type Module = typeof import("../${Pathe2.filename(route.file)}.js")
 
     export type Info = {
       parents: [${parents.map((_, i) => `Parent${i}`).join(", ")}],

@gustavopch
Copy link

You may also need this so type augmentation of AppLoadContext works:

diff --git a/node_modules/react-router/package.json b/node_modules/react-router/package.json
index 1fbd582..373a206 100644
--- a/node_modules/react-router/package.json
+++ b/node_modules/react-router/package.json
@@ -24,7 +24,7 @@
   "exports": {
     ".": {
       "node": {
-        "types": "./dist/production/index.d.ts",
+        "types": "./dist/production/index.d.mts",
         "development": {
           "module-sync": "./dist/development/index.mjs",
           "default": "./dist/development/index.js"

Without this, the AppLoadContext that's used by typegen and the one that's augmented with declare module are not the same.

@octet-stream
Copy link
Author

Yes, your patch does the trick. Thank you! Yet it still need to be fixed in RR itself.

@pcattori
Copy link
Contributor

pcattori commented Dec 2, 2024

Using React Router as a framework means using Vite as a bundler. In this case, I think setting moduleResolution to "Bundler" is correct, which makes me think that we shouldn't try to support non-Bundler options. My understanding of this might very well be too blunt, so happy to hear pushback on this if there's something I'm missing.

What reasons do you have for preferring NodeNext/Node16 over Bundler? Is there some feature you rely on that only works with NodeNext/Node16?

@pcattori
Copy link
Contributor

pcattori commented Dec 2, 2024

Ok quick update: chatted with @gustavopch offline and learned that Node16 (and thus NodeNext) is a stricter version of Bundler. Bundler is an escape hatch built into tsc, but the spec/standard is Node16/NodeNext. Since we are guaranteed to support Bundler if we support Node16/NodeNext (and not vice versa), I'm convinced that we should support Node16/NodeNext.

@octet-stream
Copy link
Author

Hey, glad to hear your thoughts!

What reasons do you have for preferring NodeNext/Node16 over Bundler? Is there some feature you rely on that only works with NodeNext/Node16?

In my case I usually use RR with Mikro ORM, which has a CLI to manage stuff like seeding, migrations etc. This CLI supports TypeScript code via ts-node, which does not do any transformations for paths for module imports, so Node.js will have same paths as the input file. Because of this, I'll get an error trying to run ESM code if my paths have no file extension at the end. I can add extensions where needed, but then I either end up writing them everywhere or just for server code that will be run by the CLI. So, I have to remember to add extension myself every time or I'll get inconsistency in my code. So, yes, node16 will ensure I have correct paths across my project.

Copy link
Contributor

github-actions bot commented Dec 2, 2024

🤖 Hello there,

We just published version 7.0.2-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

@github-actions github-actions bot closed this as completed Dec 2, 2024
Copy link
Contributor

github-actions bot commented Dec 3, 2024

🤖 Hello there,

We just published version 7.0.2 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.28.1-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.28.1 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 7.1.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

Copy link
Contributor

🤖 Hello there,

We just published version 6.28.2-pre.0 which involves this issue. If you'd like to take it for a test run please try it out and let us know what you think!

Thanks!

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

No branches or pull requests

4 participants