tatethurston / nextjs-routes

Type safe routing for Next.js
MIT License
571 stars 23 forks source link

Use with getServerSideProps, params may be undefined #126

Closed slhck closed 1 year ago

slhck commented 1 year ago

I am using:

I have the following file:

pages/automator-configs/[label].tsx:

import { GetServerSideProps, InferGetServerSidePropsType } from 'next/types';
import { getAutomatorConfig } from '../../ansibleAdapter/automatorConfig';
import type { RoutedQuery } from 'nextjs-routes';

export const getServerSideProps: GetServerSideProps<{}, RoutedQuery<'/automator-configs/[label]'>> = async (
  context,
) => {
  const { label } = context.params;
  return { props: { automatorConfig: getAutomatorConfig()[label] } };
};

export default function AutomatorConfig({ automatorConfig }: InferGetServerSidePropsType<typeof getServerSideProps>) {
  return <pre className="max-h-96">{JSON.stringify(automatorConfig, null, 2)}</pre>;
}

However, this shows several eslint and TypeScript errors:

For the first generic in GetServerSideProps, eslint shows: “Don't use {} as a type. {} actually means "any non-nullish value"”. I guess this can be ignored for now.

context.params is resolved as:

(property) params?: ({
    label: string;
} & Query) | undefined

so I cannot do const { label } = context.params;, because it shows:

Property 'label' does not exist on type '({ label: string; } & Query) | undefined'.ts(2339)

In my nextjs-routes.d.ts, I see:

declare module "nextjs-routes" {
  export type Route =
    | StaticRoute<"/404">
    | DynamicRoute<"/automator-configs/[label]", { "label": string }>
    | StaticRoute<"/">;

  interface StaticRoute<Pathname> {
    pathname: Pathname;
    query?: Query | undefined;
    hash?: string | null | undefined;
  }

  interface DynamicRoute<Pathname, Parameters> {
    pathname: Pathname;
    query: Parameters & Query;
    hash?: string | null | undefined;
  }

  interface Query {
    [key: string]: string | string[] | undefined;
  };

  export type RoutedQuery<P extends Route["pathname"]> = Extract<
    Route,
    { pathname: P }
  >["query"];

  export type Locale = undefined;

  /**
   * A typesafe utility function for generating paths in your application.
   *
   * route({ pathname: "/foos/[foo]", query: { foo: "bar" }}) will produce "/foos/bar".
   */
  export declare function route(r: Route): string;
}

So it makes sense that the query would also possibly be undefined. However, this is not what's shown in the docs.

What would be the recommended way to solve this issue?

slhck commented 1 year ago

I see that I can fix the issue with the first generic by manually specifying the expected type of props. I previously didn't have to do this, as it would be inferred automatically. No big issue, maybe just worth explaining somewhere? (I am definitely not that experienced with Next.js types …)

tatethurston commented 1 year ago

Hey @slhck nextjs-routes doesn't have a pattern for typing getServerSideProps. If there is sufficient interest in this, I'm happy to look into this further.

tatethurston commented 1 year ago

The approach here would be to import type { GetServerSideProps } from "nextjs-routes"; instead of import type { GetServerSideProps } from "nextjs"; and nextjs-routes would need to provide a more refined type for GetServerSideProps, similar to the approach already in use for Link and router.

slhck commented 1 year ago

Oh, I see, I thought it should work because the README says:

This is useful as the context type parameter inside getStaticProps and getServerSideProps

I assumed that the shown code would compile without problems for the latter case. It would definitely be useful to have this feature, but I don't know how much effort it would be.

In any case, I can work around it with runtime checks, so it's not a big issue for now!

tatethurston commented 1 year ago

@slhck Yeah the issue is that the GetServerSideProps type comes from next, which will always type context.query with SomeType | undefined. The RoutedQuery type from nextjs-routes can refine the query type, but can't remove the undefined union.

Without defining a new GetServerSideProps type, the best we can do is something like this:

const label = context.params?.label ?? '';
slhck commented 1 year ago

That's essentially what I'm doing now, yep!

slhck commented 1 year ago

This is great, thanks! Already using the new feature.