tatethurston / nextjs-routes

Type safe routing for Next.js
MIT License
557 stars 21 forks source link

Router query path params are incorrectly typed #117

Closed IGassmann closed 1 year ago

IGassmann commented 1 year ago

If a page is statically optimized, meaning it is pre-rendered on the server, the router's query object will be empty when the component first renders (hydration pass). Only after hydration, Next.js triggers an update to provide the route parameters in the query object.

As a consequence, the following query object should be typed as { foo: string | undefined } as opposed to { foo: string }.

import { useRouter } from "next/router";

// query is now typed as `{ foo: string }`
const { query } = useRouter<"/foos/[foo]">();

Next.js does this because React expects that there is no difference between the React tree that was pre-rendered and the React tree that was rendered during the first render (hydration) in the Browser. See React docs for more.

If a page uses getServerSideProps and getInitialProps, the query object won't be empty in the first render. See Automatic Static Optimization for more on this.

The router provides the value isReady to check if the router fields have been updated client-side and are ready for use. So ideally, checking for isReady should type narrow the query values:

import { useRouter } from "next/router";

const { query, isReady } = useRouter<"/foos/[foo]">();
const { foo } = query;
// `foo` is `string | undefined`

if (isReady) {
    // `foo` is now `string`
    functionThatExpectsStringParamater(foo);
}

I have reproduced the type error in this repository: https://github.com/IGassmann/query-type-error

If you open the file pages/[name].tsx, you won't see any type error for the function call expectStringParameter(name);, however, when you run the page, you will get a runtime error for the type of name. This happens because the name query value is incorrectly typed by nextjs-routes.

IGassmann commented 1 year ago

@tatethurston BTW, this is a great library! I'm very excited to use it after getting this fixed. The way Next.js types a path query parameter with a string array has always annoyed me. Especially considering that if the query parameter key is a path parameter, it won't ever be an array. For the page /users/[userId] and requested path /users/12udjasi?userId=asdkado, Next.js only returns query.userId as 12udjasi as opposed to ['12udjasi', 'asdkado']. The only exception is when we are speaking exclusively of a non-path parameter: /users?sortBy=age&sortBy=createdAt => query.sortBy = ['age', 'createdAt'].

We can't adopt the library now, because it would result in many bugs in our app. The app uses Firestore in the client. Firestore calls always expect its parameter not to be undefined. So using this library with this bug would make it easy for devs to cause runtime issues inadvertently.

tatethurston commented 1 year ago

@IGassmann Thanks for reporting and for the kind words.

The way Next.js types a path query parameter with a string array has always annoyed me. Especially considering that if the query parameter key is a path parameter, it won't ever be an array.

Agreed -- it's a funny API and type safety has generally seemed an afterthought in the Next.js team's designs.

tatethurston commented 1 year ago

Happy to look into this some more. The changes are relatively straightforward, but I've avoided implementing this thus far because there isn't any way to let clients "opt in" to this behavior automatically. If your page is not statically optimized, then there isn't any reason to check isReady -- this check shouldn't be forced onto clients that aren't statically optimized.

If you have any ideas about the API I'm happy to consider them.

tatethurston commented 1 year ago

Presently, the only APIs I can think of are:

  1. A configuration option for nextjs-routes to opt in to this behavior. Eg:
    
    const withRoutes = require("nextjs-routes/config")({
    strictStaticOptimizationTypes: true,
    });

module.exports = withRoutes(nextConfig);


2. An aliased import from `nextjs-routes` with augmented types. Eg: 
```ts
import { useRouterStaticallyOptimized } from 'nextjs-routes'

I'm not particularly happy with either, but 1. seems like the best option so far. 2. doesn't seem much better than the current situation of needing to remember to check isReady -- it still forces the programmer to identify call sites that will end up in a statically optimized context. The default is unsafe rather than forcing programmers to identify sites that are safe.

Do you have statically optimized and non statically optimized pages in your application? Or are they all statically optimized?

IGassmann commented 1 year ago

What about checking if the page is statically optimized or not by checking if it either export getServerSideProps or getInitialProps and then adjusting the type consequently? Although, I'm not sure how doable that is.

If the above is not possible, I think option 1 is better.

Do you have statically optimized and non-statically optimized pages in your application? Or are they all statically optimized?

Not at the moment, but probably in the future. I think this is also common for large Next.js apps to have.

IGassmann commented 1 year ago

@tatethurston I'm not sure if this can be done, but what about:

import { useRouter } from "next/router";

// query is now typed as `{ foo: string | undefined }`
const { query } = useRouter<"/foos/[foo]", { ssr: false }>();

// query is now typed as `{ foo: string }`
const { query } = useRouter<"/foos/[foo]", { ssr: true }>();

Maybe with a conditional type?

tatethurston commented 1 year ago
ssr: false

Yes, something like that could work, though it leaves it up to the programmer to correctly type every call site. It's effectively the same as a cast or non null assertion.

tatethurston commented 1 year ago

What about checking if the page is statically optimized or not by checking if it either export getServerSideProps or getInitialProps and then adjusting the type consequently? Although, I'm not sure how doable that is.

If the above is not possible, I think option 1 is better.

Do you have statically optimized and non-statically optimized pages in your application? Or are they all statically optimized?

Not at the moment, but probably in the future. I think this is also common for large Next.js apps to have.

So initially I thought this wouldn't work, because of useRouter usage in components and hooks outside of the page. Eg if ComponentA uses useRouter and is used by PageA and PageB where PageA and static optimized an PageB is not.

But, when generating the types only PageA dynamic segments would be typed as string | undefined, so in the example above ComponentA would be correctly typed for both PageA and PageB.

tatethurston commented 1 year ago

So nextjs-routes will need to detect the following

tatethurston commented 1 year ago

This will be published in nextjs-routes v2.0.0

tatethurston commented 1 year ago

@IGassmann This has been published in v2