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

Incorrect types for loader data that contain Record types #13058

Open
Varkoff opened this issue Feb 19, 2025 · 8 comments · Fixed by #13139
Open

Incorrect types for loader data that contain Record types #13058

Varkoff opened this issue Feb 19, 2025 · 8 comments · Fixed by #13139
Assignees
Labels

Comments

@Varkoff
Copy link

Varkoff commented Feb 19, 2025

I'm using React Router as a...

framework

Reproduction

Unfortunately Stackblitz does not support Node v20. Check reproduction code here :
https://github.com/Varkoff/react-router-type-error-with-markdoc

Since upgrading to React Router v7.2.0, I've been getting this Type 'unknown' is not assignable to type 'RenderableTreeNodes'.ts(2322) error.

Can you please take a look @pcattori ?

System Info

System:
    OS: macOS 15.3
    CPU: (10) arm64 Apple M2 Pro
    Memory: 84.78 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.9.0 - ~/.nvm/versions/node/v22.9.0/bin/node
    npm: 10.8.3 - ~/.nvm/versions/node/v22.9.0/bin/npm
    pnpm: 9.7.1 - ~/Library/pnpm/pnpm
    bun: 1.1.38 - ~/.bun/bin/bun
    Watchman: 2024.12.02.00 - /opt/homebrew/bin/watchman
  Browsers:
    Chrome: 133.0.6943.99
    Safari: 18.3
  npmPackages:
    @react-router/dev: ^7.2.0 => 7.2.0 
    @react-router/express: ^7.2.0 => 7.2.0 
    @react-router/node: ^7.2.0 => 7.2.0 
    @react-router/serve: ^7.2.0 => 7.2.0 
    react-router: ^7.2.0 => 7.2.0 
    vite: ^5.1.0 => 5.4.14

Used Package Manager

npm

Expected Behavior

I would like to not have the type error.

Actual Behavior

I am getting type errors, which prevent me to update code without adding @ts-ignore or @ts-expect-error flags. CI fails.

@Varkoff Varkoff added the bug label Feb 19, 2025
@kroofy
Copy link

kroofy commented Feb 19, 2025

I also experience something similar, for me it is related to sending data typed as Records. Didn’t manage to understand why so I am stuck at 7.1.x.

Were you able find out why this happened to you?

@pcattori
Copy link
Contributor

@Varkoff I'm getting a different error when I clone that repo and try to run npm run typecheck:

❯ npm run typecheck

> typecheck
> react-router typegen && tsc

error TS2688: Cannot find type definition file for '@remix-run/node'.
  The file is in the program because:
    Entry point of type library '@remix-run/node' specified in compilerOptions

  tsconfig.json:13:15
    13     "types": ["@remix-run/node", "vite/client"],
                     ~~~~~~~~~~~~~~~~~
    File is entry point of type library specified here.


Found 1 error.

Not sure why that's happening, haven't dug into it. Any chance you can provide a better reproduction of your issue?

@pcattori pcattori self-assigned this Feb 24, 2025
@kroofy
Copy link

kroofy commented Feb 26, 2025

I got rid of the issue, will present my findings below.

  • I was able to reproduce the issue presented by checking out Varkoff code locally on my machine
  • I was NOT able to reproduce it at Stackblitz after porting some code (makes me believe it could also be system specific?)

It all seems to come down to what is passed down to the data function. What I had to do to fix the issue:

  1. Convert an Immutable.js List using .toArray(). In previous Remix and RR versions this didn't cause any issue.
  2. Data structures returned from the loader that were previously interfaces had to be converted to types
  3. For the 2 routes (we probably got ~40 in total) where the type check from the loader didn't work I had to convert return data<MyType>({ someItems: {} }) to const myData = { someItems: {} } satisfies MyType; return data(myData). These 2 routes were the only ones where a type passed in e.g. data<MyType>().

Just to be extra clear, all three points mentioned above had to done, just doing one or two didn't help. Hope it can help someone facing the same issues as I did.

@brophdawg11
Copy link
Contributor

I'm not quite sure how but this seems to be caused by #12264. If I comment out this line in OP's repro it fixes the issue

@seanify-24
Copy link

seanify-24 commented Feb 27, 2025

Am also hitting this error, and also not quite sure what to make of this yet, but here is an isolated illustration:

// Changing import to use remix-run will fix the issue, so it breaks in react-router
// import {useLoaderData} from '@remix-run/react';
import {useLoaderData} from 'react-router';

type DataStructure = {
  title: string;
  url: string;
}

const loader = async (args) => {
  const title = "Title";
  const description = "desc...";
  const dynamicKeyObj: {[k: string]: DataStructure} = getExternalData();

  // hovering these attributes show all type values as expected
  return { title, description, dynamicKeyObj };
}

const Component = () => {
  const { title, description, dynamicKeyObj } = useLoaderData<typeof loader>();
  // cannot use dynamicKeyObj because it is unknown
}

@pcattori
Copy link
Contributor

@seanify-24 if I replicate your example, the type of dynamicKeyObj is not inferred as unknown:

Image

@pcattori
Copy link
Contributor

@Varkoff I've condensed this into a minimal repro: https://tsplay.dev/NrkKDm

So I think the fix is to remove the optionality of the internal branded type. @phryneas : heads up that I am planning to change unstable_ReactRouter_SerializesTo to be required in the next release to fix these sorts of bugs for users.

@pcattori pcattori changed the title Incorrect type returned from useLoaderData (changing RenderableTreeNodes into unknown type) Incorrect types for loader data that contain Record types Feb 28, 2025
@phryneas
Copy link
Contributor

@pcattori okay, thanks for the heads-up!

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

Successfully merging a pull request may close this issue.

6 participants