tatethurston / nextjs-routes

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

Next13 app directory #170

Closed tatethurston closed 1 month ago

tatethurston commented 1 year ago

This issue is to house all discussion of Next13's app directory. To date, there have been a number of issues inquiring about the app directory:

nextjs-routes only supports the pages directory today. Next.js 13.2 includes statically typed links for the app directory: https://nextjs.org/blog/next-13-2#statically-typed-links.

If you find deficiencies with Next.js' approach and have ideas for a better experience, let me know and I’m happy to consider adding support. Until I hear from folks, I'm operating under the impression that Next 13's statically typed links meet everyone's needs.

Kovbo commented 6 months ago

@tatethurston Thank you for adding support for the app router. How to use it with the app router? Dynamic href in this format is not supported now:

  href={{
    pathname: "/foos/[foo]",
    query: { foo: "bar" },
  }}

I've been trying to use Next.js' types, but run into a limitation with dynamic routes. Having some dynamic routes (/foos/[foo]/bar and /foos/[foo]/baz) we built a component that should accept only the last part of the dynamic routes pattern (bar or baz) and construct the full route itself.

Your package provides a convenient DynamicRoute interface that makes it possible to create a derivative type that can extract bar and baz:

type ExtractFooRoutes<T> = T extends DynamicRoute<`/foos/[foo]${infer Rest}`, infer Params>
  ? { path: Rest; query: Omit<Params, "foo"> & Record<string, string | undefined> }
  : never;

export type FoosRoute = ExtractSlugRoutes<Route>;

It looks like it is not possible to do the same with Next.js' types. The route they provide import { type Route } from "next"; gives access to static routes only.

tatethurston commented 6 months ago

@Kovbo nextjs-routes doesn’t directly support the app router because of the nextjs change to only accept strings for the href. You may be able to use route for your use case https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime

caioaao commented 6 months ago

@Kovbo nextjs-routes doesn’t directly support the app router because of the nextjs change to only accept strings for the href. You may be able to use route for your use case https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime

Apparently it's not possible now with nextjs14 as it throws a runtime error saying:

Error: Dynamic href `bla` found in <Link> while using the `/app` router, this is not supported. Read more: https://nextjs.org/docs/messages/app-dir-dynamic-href

I think the short-term solution would be giving a escape-hatch config for allowing strings in Link href prop. An idea would be to use template literal types in the future for not allowing just any strings, but idk how that'd play out with the route fn as it spits strings.

tatethurston commented 5 months ago

@Kovbo nextjs-routes doesn’t directly support the app router because of the nextjs change to only accept strings for the href. You may be able to use route for your use case https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime

Apparently it's not possible now with nextjs14 as it throws a runtime error saying:

Error: Dynamic href `bla` found in <Link> while using the `/app` router, this is not supported. Read more: https://nextjs.org/docs/messages/app-dir-dynamic-href

I think the short-term solution would be giving a escape-hatch config for allowing strings in Link href prop. An idea would be to use template literal types in the future for not allowing just any strings, but idk how that'd play out with the route fn as it spits strings.

The route function from https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime returns a string.

caioaao commented 5 months ago

@Kovbo nextjs-routes doesn’t directly support the app router because of the nextjs change to only accept strings for the href. You may be able to use route for your use case https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime

Apparently it's not possible now with nextjs14 as it throws a runtime error saying:

Error: Dynamic href `bla` found in <Link> while using the `/app` router, this is not supported. Read more: https://nextjs.org/docs/messages/app-dir-dynamic-href

I think the short-term solution would be giving a escape-hatch config for allowing strings in Link href prop. An idea would be to use template literal types in the future for not allowing just any strings, but idk how that'd play out with the route fn as it spits strings.

The route function from https://github.com/tatethurston/nextjs-routes?tab=readme-ov-file#what-if-i-need-a-runtime returns a string.

but the project overrides next's route type and doesn't allow us to pass a string to the component, so we can't use the route function directly so we have to call route(...) as unknown as Route for it to work

lern905 commented 2 months ago

Hello, first of all, thank you for this great library!

Regarding what you mention:

If you find deficiencies with Next.js' approach and have ideas for a better experience, let me know and I’m happy to consider adding support. Until I hear from folks, I'm operating under the impression that Next 13's statically typed links meet everyone's needs.

I believe that the NextJS typed links still lack a whole lot that this library has to offer - to mention just a few:

  1. NextJS typed routes have no typed params, while this library does. While this does not seem like a big problem per-se with short routes, this starts to become a bigger problem as you have longer routes (building one of these with template literals is, in my opinion, a big pain point and error-prone).
  2. This library provides utils to build routes and also type utils to receive know which params a Page might receive - I'm not aware of something like this for the NextJS counterpart.

I have taken a quick look, and as far as I am aware, I believe a few tweaks might make this library still pretty useable with the app directory:

  1. Omit the whole declare module "next/link" declaration when there is an app directory or on a certain nextjs version. That way, this can be used even with the new Link version. I'm guessing this could also be opt-in through a next.config nextRoutes param. Because the package changed for the new router, removing the next/router declaration might not be required.
  2. OPTIONAL: changing the GetServerSidePropsContext and GetServerSideProps to another name considering these no longer exist for the app directory. This, however, is not really required in order to make this library work with the /app directory but just a nice to have.

If you know of a way to avoid these NextJS typed links shortcomings, please let me know!

tatethurston commented 2 months ago

Hey @lern905 thanks for the write up. Now that the app router has some time to settle, I'll take a look at what support looks like. I think in order of preference my thoughts are:

support for both app and page usage in a single project > support for either app or page (but not both) in a singe project > support for only the app router

w.r.t to the name change for GetServerSidePropsContext and GetServerSideProps, is theer a use case where you'd like to consume these types in an app router project?

lern905 commented 2 months ago

Hey @lern905 thanks for the write up. Now that the app router has some time to settle, I'll take a look at what support looks like. I think in order of preference my thoughts are:

support for both app and page usage in a single project > support for either app or page (but not both) in a singe project > support for only the app router

I totally agree with those preferences. In fact, I think it would be the correct approach considering that this way the library would remain backwards compatible.

However, TBH, I'm not entirely sure what would be the best way to accomplish this. The way I see it, NextJS dropped the dynamic Link props support only when the app router is used, which means that having the correct Link declaration for both cases does not seem plausible in a project with both the pages and app router.

For a router app project, using something such as the following would be ideal:

<Link
  href={route({
    pathname: "/some-test-path/[withParam]",
    query: { withParam: "1" },
  })}
>
  Test
</Link>

Which currently fails because the types declarations favor the following:

<Link
  href={{
    pathname: "/some-test-path/[withParam]",
    query: { withParam: "1" },
  }}
>
  Test
</Link>

Allowing a string type for the Link href would solve the problem, but at the same time, type-safety would be lost for both the pages and app router.

I'll try to think if there's any good solution for this.

w.r.t to the name change for GetServerSidePropsContext and GetServerSideProps, is theer a use case where you'd like to consume these types in an app router project?

It's just a naming detail and not sure if it's really required to make changes on it TBH. I just think these are pretty useful for having the typed params for a Page component. For example:

export default function Page({params}: {params: RoutedQuery<"/test-path/[testParam]">;}) {
  return <div>{params.testParam}</div>;
}

That way you can have the typed params according to the routed query, and if this route does change, it will help since either the RoutedQuery type or the params.paramName call will fail.

I'm just thinking out loud here, but this could be augmentated on each project with something such as type PageProps<T> = RoutedQuery<T>; so I don't think it's really necessary.

tatethurston commented 2 months ago

@lern905 I published 2.2.2-rc.3 to support what we've discussed. LMK your thoughts. A few outstanding items:

lern905 commented 2 months ago

Hey @tatethurston,

I've just taken a look at the new version, and I must say - wow, it's looking pretty good! I see you've managed to sort out the return type check with the RouteLiteral type, which was a really great idea.

I have tested this locally, and found no issues at all!

Thank you!!

sleepdotexe commented 2 months ago

Hi @tatethurston,

Firstly just wanted to say thanks for authoring this package and for adding support for the app directory. For my use case, I chose this package over experimental.typedRoutes since I've found the Next.js typedRoutes flag is still quite buggy – particularly with parallel routes and dynamic routes. It would often work during next dev, but wouldn't generate types for some routes during next build, causing builds to fail since now some <Link> elements failed type checks. Plus, the added benefit of having type inference for route parameters with nextjs-routes is awesome too.

I just wanted to raise a couple of things I've come across while trying to implement the package:


1. ref is missing from <Link>

When nextjs-routes.d.ts is generated, the new type for Link does not seem to include a property for ref. The original typing for <Link> has the following:

(property) RefAttributes<HTMLAnchorElement>.ref?: LegacyRef<HTMLAnchorElement> | undefined

Would it be possible to have this property added back into the types? Currently I am just working around this with:

<Link
  // @ts-ignore
  ref={ref}
/>

Taking a look at Next's link.d.ts, the definition for Link looks like this:

export type LinkProps<RouteInferType = any> = InternalLinkProps;
declare const Link: React.ForwardRefExoticComponent<Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, keyof InternalLinkProps> & InternalLinkProps & {
    children?: React.ReactNode;
} & React.RefAttributes<HTMLAnchorElement>>;

...and nextjs-routes generated file seems to be importing LinkProps to form its custom type:

import type { LinkProps as NextLinkProps } from "next/dist/client/link";
// ...
export interface LinkProps
    extends Omit<NextLinkProps, "href" | "locale">,
      AnchorHTMLAttributes<HTMLAnchorElement> {
    href: StaticRoute | RouteLiteral;
    locale?: false;
  }

So hopefully this could be a matter of tweaking the nextjs-routes LinkProps to match the Next.js implementation?


2. Types are being generated for parallel route folder

Currently it looks like types are being generated for parallel routes. For example, I have two files: app/(protected)/app/admin/page.tsx and app/(protected)/app/admin/@breadcrumbs/page.tsx. My generated file looks like this:

 export type Route =
    // ...
    | StaticRoute<"/app/admin">
    | StaticRoute<"/app/admin/@breadcrumbs">

This doesn't actually cause any issues (other than significantly inflating the number of available Route types to choose from), however I don't think it's intended behavior to be able to Route to a parallel route. This works with no type errors:

<Link href={'/app/admin/@breadcrumbs'}></Link>

...and generates a link with the href of http://localhost:3000/app/admin/@breadcrumbs, which 404s if you follow it.

Taking a look at the source, it seems like getAppRoutes is trying to filter out slots from the path, but when I inspect my copy of nextjs-routes in node_modules, core.js is missing the slots line. Did this change get lost in translation somehow or is there a way to fix this?


3. Potential gotcha: using values from next.config.js in middleware.ts

This one is probably pretty specific to my configuration, but I thought I'd mention it in case someone else is running into the same issue.

The root of the problem is that nextjs-routes uses chokidar to watch for file changes. Under the hood, chokidar relies on the stream Node API. However, this API is not supported by the Edge Runtime which must be used by middleware.ts.

In my middleware, I was setting up a CSP and was trying to reuse my images domains that I declared in next.config.js. However, by importing from next.config.js into middleware.ts, this was causing nextjs-routes to be included in the bundle which was erroring since it uses unsupported APIs.

The solution for this was to move my images declaration into a separate file that doesn't require nextjs-routes, and import that file into both next.config.js and middleware.ts.

tatethurston commented 2 months ago

Hey @sleepdotexe thanks for the write up -- I really appreciate your thoughts and the breakdown of pain points you're encountering.

  1. Yes, I'll take a look at correcting this. The intended behavior is to match Next.js' types exactly, with only some additional type safety "sprinkled on".

  2. Yes, I believe this is fixed in nextjs-routes@next, specifically 2.2.2-rc.3. Could you LMK if you're still encountering this issue on that version?

  3. Yeah nextjs-routes is set up so chockidar only ends up required in build environments and not in any runtime paths. Generally next.config.js is used to configure build-time settings and is not directly imported into runtime code. AIUI your approach of splitting any shared code into a separate module that is imported by next.config.js and any other consuming sites is the preferred approach -- that ensures you don't end up with unnecessary dependencies in your runtime. That said, I'm certainly happy to dig in deeper here if you can point me towards any examples demonstrating this as a recommended pattern. We'd need to identify a way to have chokidar and/or the whole plugin "tree shake" out the middleware JS build/bundle, I'm curious if there are any examples from other nextjs packages. The middleware runtime lacks node FS modules, which ultimately need to be present for the build-time execution.

sleepdotexe commented 2 months ago

Can confirm that nextjs-routes@next has resolved the issue with parallel routes! 🚀

For 3, I wouldn't worry about changing the implementation – I agree that separate modules is probably a better approach. I can't see a reason why the module would need to be included in middleware intentionally, so It's more just a warning in case anyone else runs into the same error.

Thanks!

tatethurston commented 2 months ago

Thanks @sleepdotexe. I just published 2.2.2-rc.4 which should fix the ref issue you reported.

tatethurston commented 1 month ago

Initial support has landed in 2.2.2