typescript error using page-based routing

mrose commented 3 weeks ago

Describe the bug

Trying to create a list of messages where each message has a clickable link. The code below does compile and produce the intended link, but displays the following typescript error in Visual Studio Code: Type 'string' is not assignable to type 'RouteImpl<RouteImpl>'. ts(2322) Error occurs on the <Link> line

const MessageList = async () => {
  const messages = await getAllMessages();
  return (
    <>{
    messages.map(({id, monthAndYear}) => {
      const to = `/messages/?id=${id}`;
      return (
        <li className="list-none" key={id}>
          <Link to={to}>{monthAndYear}</Link>
        </li>
      );
    })
  }</>
  );
};

// messages are typed as:
export type message = {
  id: number;
  monthAndYear: string;
  message: string;
};

export type messages = message[];

Reproduction

No response

Steps to reproduce

install react-server latest create a basic page with page based routing create some code to fake the api which returns a json of messages include the code above into the page

System Info

gitpod /workspace/react-server-play (experiment-with-rsc) $ pnpx envinfo --system --npmPackages '{@lazarv/react-server,@lazarv
/react-server-*,react,react-dom,react-server-dom-webpack,vite}' --binaries --browsers
Packages: +1
+
Progress: resolved 1, reused 0, downloaded 1, added 1, done

  System:
    OS: Linux 6.1 Ubuntu 22.04.4 LTS 22.04.4 LTS (Jammy Jellyfish)
    CPU: (16) x64 AMD EPYC 7B13
    Memory: 28.21 GB / 62.79 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 20.17.0 - ~/.nvm/versions/node/v20.17.0/bin/node
    Yarn: 1.22.22 - ~/.nvm/versions/node/v20.17.0/bin/yarn
    npm: 10.8.2 - ~/.nvm/versions/node/v20.17.0/bin/npm
    pnpm: 9.9.0 - ~/.nvm/versions/node/v20.17.0/bin/pnpm
  npmPackages:
    @lazarv/react-server: 0.0.0-experimental-a37c63c-20241025-ab08c953 => 0.0.0-experimental-a37c63c-20241025-ab08c953 
    vite: 6.0.0-alpha.18 => 6.0.0-alpha.18

Used Package Manager

pnpm

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] Check that this is a concrete bug. For Q&A open a GitHub Discussion.
[X] The provided reproduction is a minimal reproducible example of the bug.

lazarv commented 3 weeks ago

Hi @mrose!

There are two issues with the code above, here:

const to = `/messages/?id=${id}`;
return (
  <li className="list-none" key={id}>
    <Link to={to}>{monthAndYear}</Link>
  </li>
);

When you assign the value of to, the type inferred by TypeScript will be a simple string which is not assignable to the to prop of Link, which expects the type __react_server_routing__.RouteImpl<__react_server_routing__.RouteImpl<T>>. This type checks the inferred type against the available routes in the router. If the type of T doesn't match any available routes, then it will be a never, so a type error will be raised.

The correct type of to would be __react_server_routing__.RouteImpl<__react_server_routing__.RouteImpl<`/messages?id=${number}`>>. But I consider this extremely verbose, so I would suggest using the value directly on the Link.

return (
  <li className="list-none" key={id}>
    <Link to={`/messages?id=${id}`}>{monthAndYear}</Link>
  </li>
);

Another issue with the URL is that it contains a trailing slash, which is not part of the correct path of the messages page.

If you don't need this type checking, then just remove ".react-server/**/*.ts" from the include list in your tsconfig.json.

{
  // ...
-  "include": ["**/*.ts", "**/*.tsx", ".react-server/**/*.ts"],
+  "include": ["**/*.ts", "**/*.tsx"], 
  // ...
}

What I could consider as an enhancement is a new configuration option for the router to generate loose types and allow any string as a route to only use the route types for suggesting available routes.

Another enhancement could be a type alias for the lengthy RouteImpl<T> type, importable from @lazarv/react-server/navigation, but even then the T generic needs to be provided for type checking.

mrose commented 3 weeks ago

Thanks very much for taking the time to comment so thoroughly - much appreciated! After this post I rewrote the urls to (e.g.) /messages/1 since I'm using the builtin router, now with [id].tsx in the messages folder, and also assigned the type to the string. (I have a preference to use intermediate expressions to help with debugging)

Regarding the trailing slash, I expected /messages/ to resolve to the index page and wonder whether that could be configured in the http configuration. In the olden days we did it that way on Apache.... Personally I would not be in favor of the first enhancement you suggest since I (and likely others) would just continue to get it wrong. The latter enhancement is more appealing and I'm guessing other routers do it like that.

lazarv commented 3 weeks ago

Sorry, I forgot to mention another solution for the typing: using as const.

const to = `/messages?id=${id}` as const;
return (
  <li className="list-none" key={id}>
    <Link to={to}>{monthAndYear}</Link>
  </li>
);

About the trailing slash, the old approach with two routes only with a trailing slash difference can cause more unexpected behavior and should be avoided. This framework auto-redirects URLs with a trailing slash to the non-trailing slash URL and treats pages more like endpoints than directories.

Although I like how TanStack Router gives the developer control over this, see https://tanstack.com/router/latest/docs/framework/react/api/router/RouterOptionsType#trailingslash-property, if you need this, please create an enhancement issue for it.

lazarv / react-server